WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#30753 closed defect (bug) (wontfix)

wp_parse_str() improperly casts boolean querystring values as strings

Reported by: r-a-y Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.2.1
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

If you use a querystring in wp_parse_args(), wp_parse_str() is used.

Let's say we do something like this:

$r = wp_parse_args( 'this_should_be_bool_false=false&this_should_be_bool_true=true', array() );

$r['this_should_be_bool_false'] equals the string 'false' instead of boolean false.

Attached patch uses wp_validate_boolean() to properly cast string values of 'true' or 'false' to their boolean equivalents and includes a unit test.

This may be a wontfix since it's more of a PHP issue with parse_str(), but I thought I'd post this anyway.

Attachments (2)

30753.01.patch (1.4 KB) - added by r-a-y 7 years ago.
30753tests.diff (814 bytes) - added by voldemortensen 7 years ago.

Download all attachments as: .zip

Change History (9)

@r-a-y
7 years ago

#1 @dd32
7 years ago

Although the change makes sense on the surface, what you're really doing here is literally passing a string value of 'false', which is a completely acceptable string to be used here.

In many themes and even some places in core in the past, we've used bool_attribute=0 to designate false-y.

This change unfortunately has the downside that it could cause a bug in anything that expects the string "false" as a value.

#2 @r-a-y
7 years ago

Totally agree. Like I said, it's probably a wontfix, but I thought I'd post it and see what everyone's thoughts were.

#3 @nacin
7 years ago

  • Version changed from 4.1 to 2.2.1

Yeah, I don't think we should change this. But it's good to have documented here in a ticket.

I'd prefer to discourage query string usage. At the same time I'd also like to not break it. And this will surely break something that someone is doing somewhere. wp_parse_args() is an ubiquitous, low-level function.

Thanks for including a unit test! Perhaps we should instead add a unit test that expects this to produce the string 'false', and link to this ticket, in order to enforce the design decision here. That way it's documented in an even better way.

#4 @voldemortensen
7 years ago

  • Keywords has-patch added

Patch adds unit tests to make sure that 'true' or 'false' passed in query args are checked as strings.

#5 @SergeyBiryukov
7 years ago

In 31310:

Add a unit test that expects wp_parse_args() to treat 'true' and 'false' in a query string as strings.

props voldemortensen for initial patch.
see #30753.

#6 @SergeyBiryukov
7 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing as wontfix, per comment:3.

#7 @SergeyBiryukov
7 years ago

In 31311:

Remove redundant parameter.

see #30753.

Note: See TracTickets for help on using tickets.