Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#20143 closed defect (bug) (fixed)

Array query args on a paginated archive page creates PHP Warning, breaks page.

Reported by: ericlewis's profile ericlewis Owned by: nacin's profile nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Canonical Keywords: has-patch needs-unit-tests
Focuses: Cc:


Having query args that are elements of an array breaks in redirect_canonical.

To reproduce:

  1. Create enough posts in an install to warrant pagination of posts
  2. Go to second page and add an array element query arg eg.[bar]=true

I'm getting this PHP warning:

Warning: rawurlencode() expects parameter 1 to be string, array given in /Users/ericlewis/Sites/wp-trunk/wp-includes/canonical.php on line 283

Warning: Cannot modify header information - headers already sent by (output started at /Users/ericlewis/Sites/wp-trunk/wp-includes/canonical.php:283) in /Users/ericlewis/Sites/wp-trunk/wp-includes/pluggable.php on line 864

This stems from the fact that in redirect_canonical, the query args are parsed into an array via parse_str(), and then rawurlencode() is applied to each element of that array. In this case, that means rawurlencode is applied to an array (foo), which causes the issue.

Attachments (3)

canonical.diff (623 bytes) - added by toppa 12 years ago.
20143.patch (845 bytes) - added by SergeyBiryukov 12 years ago.
20143.diff (1.4 KB) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (10)

#1 @toppa
12 years ago

I replaced array_map on line 283 with array_walk_recursive and added a rawurlencode callback function. I put the callback function at the bottom of the file - there may be a better place for it.

12 years ago

#2 @toppa
12 years ago

  • Cc toppa added

I just uploaded a revised version with a correction to my code.

This problem is affecting one of our projects. We're doing taxonomy and meta filtering based on form submissions that have checkboxes using arrays. My patch fixes the pagination problem we're experiencing.

#3 @dd32
12 years ago

  • Component changed from General to Canonical
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.4

when you upload a new version of the patch, don't overwrite the old one, it's simpler to track changes/etc.

changes look sane, although do we really need to use the wrapping function around the url decode?

#4 @toppa
12 years ago

Thanks - I'll not overwrite next time (this is my first patch). If you try to call rawurlencode without the wrapping function you will get a PHP warning. array_walk_recursive wants the function you give it to take 2 parameters (value and key).

#5 @SergeyBiryukov
12 years ago

  • Version set to 3.3

Related: #18086

Added PHPDocs (20143.patch).

12 years ago

#6 @nacin
12 years ago

  • Keywords needs-unit-tests added

The cognitive problem with array_walk() is that it requires a callback that is particular to its use, as it needs to accept two particular arguments. This is not the case with array_map(). For when the callback does not require the key to operate, it is nicer, as you can do things like using intval, absint, or whatever else you might have. Or, in this case, rawurlencode().

Since we already have urlencode_deep(), I figure we can just add a rawurlencode_deep(). See 20143.diff. Still props toppa and Sergey for the docs as the approach is the same.

#7 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [20611]:

Run rawurlencode_deep() through the parsed query in canonical. (Introduces rawurlencode_deep().) props toppa for the initial patch. fixes #20143.

Note: See TracTickets for help on using tickets.