WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 18 months ago

#14330 closed defect (bug) (fixed)

Improper handling of 'post_type[]=' like parameters

Reported by: loushou Owned by: scribu
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch commit
Focuses: Cc:

Description (last modified by scribu)

PLEASE READ WHOLE BUG: because it includes reasons for severity also

THE PROBLEM:

So I have a problem.... obviously, right? So if I were to do something like:

get_posts('post_type[]=post_type_1&post_type[]=post_type_2');

I would receive a list of posts that are either of post type 'post_type_1' or of 'post_type_2'. Now if I goto a url like the following:

http://www.my-site.com/index.php?post_type[]=post_type_1&post_type[]=post_type_2&name=testing-post

I should get a very very similar result, but instead, I get a 404 error page.

How can I make the later example work? I have been browsing the WP base code (like always) and found the following line in classes.php on line 277:

$this->query_vars[$wpvar] = (string) $this->query_vars[$wpvar];

Most probably don't know what this line actually means, but in short it converts all the query vars (everything after the ? in a url) to strings, even those that should be arrays (since the WP_Query object supports arrays). So instead my slightly parsed url winds up looking like:

http://www.my-site.com/index.php?post_type=Array&name=testing-post

because in PHP when performing a string type cast on an array, you get a string with the word 'Array':

$myArray = array(
  'some value',
  'another un-important value'
);
echo (string) $myArray;
// prints out 'Array'

Then later in classes.php on line 289, we have this:

// Limit publicly queried post_types to those that are publicly_queryable
    if ( isset( $this->query_vars['post_type']) ) {
      $queryable_post_types =  get_post_types( array('publicly_queryable' => true) );
      if ( ! in_array( $this->query_vars['post_type'], $queryable_post_types ) )
        unset( $this->query_vars['post_type'] );
    }

which does what it says, eliminates my now post_type=Array from the query completely because 'Array' is not only not a proper post type, but it is not 'publicly_queryable'.

So, again I ask, how do I get a url like:

http://www.my-site.com/index.php?post_type[]=post_type_1&post_type[]=post_type_2&name=testing-post

to work, assuming I do not know which post type it is?

p.s. ~ for the really high-level programmers, it is actually a little more in depth than this, but this is the basic idea of the problem, and works exactly the same way as the actual problem. for a glimpse into the big picture problem, think about how WP interprets clean urls to determine the query used to pull the list of posts to display on the current page.

IMPORTANCE VALUE:

This is marked 'major' because on of the key features of WP3.0 was better handling of custom post types. With this problem, all translations of the url where multiple post types are being queried will fail. Not only is this a post_type problem, but above should clearly explain that any query variable that the WP_Query class normally accepts in the form of an array, will be effected. This means that now translated urls like this:

http://www.my-site.com/index.php?post_status[]=publish&post_status[]=new_custom_status&post_status[]=inherit

will also not work.

So because the scope of this problem directly conflicts with the awesome, much anticipated new features of WP, on a large scale, it is very important that it gets solved quickly.

Attachments (7)

url-array-fix.patch (2.5 KB) - added by loushou 9 years ago.
Patch
better-url-array-fix.patch (1.5 KB) - added by loushou 9 years ago.
Patch
php4-url-array-fix.patch (1.5 KB) - added by loushou 9 years ago.
Patch
php4-and-coding-standards-url-array-fix.patch (1.6 KB) - added by loushou 9 years ago.
Patch with coding standards and php4 ompatibility
php4-stands-and-elegant-url-array-fix.patch (1.5 KB) - added by loushou 9 years ago.
Patch with brilliance
patch.patch (1.5 KB) - added by loushou 9 years ago.
Properly named patch that accounts for array values in url queries, instead of destroying them, compiant with PHP4 and the coding standards, with the elegant code suggested in an earlier post
14330.patch (1.4 KB) - added by hakre 9 years ago.
Secondary approach

Download all attachments as: .zip

Change History (36)

@loushou
9 years ago

Patch

@loushou
9 years ago

Patch

#1 @loushou
9 years ago

  • Owner set to loushou
  • Status changed from new to accepted

Taking over the ticket

#2 @loushou
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

So I made a patch. Then I was looking at it an I found I could simplify it. So the attached file 'better-url-array-fix.patch' is the condensed one. I tested them against my install, and it performed exactly how I described it should in the bug, without breaking any of my site. However I would like to add that my site is pretty small with only BuddyPress as a plugin. This does need to be tested on bigger sites.

PATCH BREAKDOWN

Prior to casting the query variable to a string, we need to check if it is an array (since WP_Query accepts arrays as parameter values). If it is, instead of casting the entire variable to a string, we need to cycle through the array, and cast each value of the array to a string. Every other type gets processed exactly as it did before.

Then for the 'post_type' query var specifically (since it has it's own independent logic a little lower in the code), we need to modify the logic specifically targeting it. Again we check if the value of $this->query_varspost_type? is an array. If it is, we need to compare each element in the list to the publicly queryable post_types. Once again if the value is not an array, then it is processed exactly as before.

Loushou

#3 @scribu
9 years ago

  • Keywords has-patch added; post_type WP WP_Query classes.php removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

A ticket isn't fixed until the patch is commited. Please read http://codex.wordpress.org/Reporting_Bugs

#4 @scribu
9 years ago

  • Description modified (diff)

#5 @scribu
9 years ago

  • Keywords needs-patch added; has-patch removed

Note that foreach ($this->query_vars[$wpvar] as &$v) won't work in PHP4 (which still needs to be supported until 2011).

#6 @scribu
9 years ago

  • Milestone changed from Awaiting Review to 3.1

Anyway, I agree that it's a problem that needs to be solved.

#7 @loushou
9 years ago

  • Status changed from reopened to assigned

Sorry about the premature closing. I must have mis-read the Reporting_Bugs page, because now looking at it, it does say that. As far as PHP4 compatibility, I flat out forgot about keeping that in mind, as for the last 4 years (at least) have worked on PHP5. I do understand the support requirement though and I can make a new patch pretty quickly that will be compatible. Finally, thank you for not only understanding the problem I proposed, but also supporting it. It does mean a lot.

@loushou
9 years ago

Patch

#8 @loushou
9 years ago

This patch removes all 'references' in foreach loops, since they were not available in PHP4. Instead it uses the key-value method to replace the values with string casted values. Also removed the references in the foreach loop in the second section for the patch that dealt specifically with the post_type query variable.

Thanks for the notes again. :)

#9 @scribu
9 years ago

Please add spaces to the insides of language constructs and function calls, as shown in http://codex.wordpress.org/WordPress_Coding_Standards

#10 @loushou
9 years ago

Sure thing :). Thank you for being patient with me. This is my first ticket in Trac. I never make the same mistake twice. New patch soon.

#11 @scribu
9 years ago

Also, instead of

foreach ($this->query_vars['post_type'] as $key => $pType) 
    if ( ! in_array( $pType, $queryable_post_types ) )
        unset( $this->query_vars['post_type'][$key] );

it would be more elegant to use array_intersect:

$this->query_vars['post_type'] = array_intersect( $this->query_vars['post_type'], $queryable_post_types );

@loushou
9 years ago

Patch with coding standards and php4 ompatibility

#12 @scribu
9 years ago

PS: Trac will automatically rename attachments with the same name like so:

  • array-fix.diff
  • array-fix.2.diff
  • array-fix.3.diff
  • etc.

PPS: instead of the non-helpful 'patch', you might as well leave the description field blank.

#13 @scribu
9 years ago

What I'm trying to say is that you should use the description field to describe your patch, instead of the attachment name.

@loushou
9 years ago

Patch with brilliance

@loushou
9 years ago

Properly named patch that accounts for array values in url queries, instead of destroying them, compiant with PHP4 and the coding standards, with the elegant code suggested in an earlier post

#14 @loushou
9 years ago

You make an excellent point. Thanks.

#15 @scribu
9 years ago

  • Keywords has-patch added; needs-patch removed

@hakre
9 years ago

Secondary approach

#16 @hakre
9 years ago

I just ran over this ticket and skimmed throught your discussion here and I thought I throw in my inspiration in form of a second patch:

Instead of handling multiple input types, in this patch I normalize the input first, so it's more easy to deal with afterwards.

As you can see, the code size is reduced and it's less complex.

#17 in reply to: ↑ description ; follow-up: @wpmuguru
9 years ago

Replying to loushou:

PLEASE READ WHOLE BUG: because it includes reasons for severity also

THE PROBLEM:

So I have a problem.... obviously, right? So if I were to do something like:

get_posts('post_type[]=post_type_1&post_type[]=post_type_2');

That's where you are going wrong. Try

get_posts('post_type=post_type_1,post_type_2');

#18 @loushou
9 years ago

Hakre - excellent suggestion. Looks really good and does the same exact thing. Wish I would have thought of that.

WpmuGuru - while that may work for the 'get_post' function, it will not work for a regular url query string. In classes.php on line 224, while the url params are getting looped over, we do not have a statement that will explode each param on ',', however it does make use of:

parse_str($query, $perma_query_vars);

which makes my query work. That being said, perhaps that notation should be added to the patch as well. What does everyone think?

#19 @loushou
9 years ago

Hakre - On second thought, I may have reacted a little too quickly. Your solution does solve the main problem; however, now ALL the params are getting turned to an array. First I am not sure that the extra logic for every param being an array is being accounted for with you solution (as there is plenty of logic below the for loop that deals with each param individually) and second, I am pretty sure that WP_Query only accepts arrays for certain params, not all params. For instance, had we included a date and time in our URL, the hour, minute, second, month, day, and year params all require single values, not arrays. Same for the name param. This solution is not as complete as I originally thought at first glance. :(

#20 in reply to: ↑ 17 @scribu
9 years ago

  • Keywords commit added
  • Severity changed from major to normal

Replying to wpmuguru:

That's where you are going wrong. Try

get_posts('post_type=post_type_1,post_type_2');

Yes, this works, but it doesn't fix the problem. Suppose one wants to implement a form that queries multiple post types:

<form method="get" action="">
  <input type="checkbox" name="post_type[]" value="post_type_1">
  <input type="checkbox" name="post_type[]" value="post_type_2">

...
</form>

The resulting URL will be post_type[]=post_type_1&post_type[]=post_type_2, which is a perfectly reasonoable way to send query variables.

Because of the current bug, you would have to add further code (either JavaScript or a 'request' hook) to transform it into 'post_type=post_type_1,post_type_2', which is a waste.

Still, since this is something that can be worked around and doesn't affect any part in Core, I think it's severity is 'normal'.

#21 @scribu
9 years ago

  • Summary changed from Multiple Post Type in Page Query Problem to Improper handling of 'post_type[]=' like parameters

#22 @scribu
9 years ago

Note: the patch I think is commit worthy is patch.patch

#23 @mikeschinkel
9 years ago

  • Cc mikeschinkel@… added

#24 @scribu
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [15737]) Proper handling of array query variables. Props loushou. Fixes #14330

#25 @nacin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Add an Image popup > Media Library tab:

Catchable fatal error: Object of class stdClass could not be converted to string in /Users/nacin/Sites/3.1/wp-includes/classes.php on line 284

Call Stack:
    0.0005     689064   1. {main}() /Users/nacin/Sites/3.1/wp-admin/media-upload.php:0
    0.4897   28972104   2. do_action() /Users/nacin/Sites/3.1/wp-admin/media-upload.php:120
    0.4897   28973624   3. call_user_func_array() /Users/nacin/Sites/3.1/wp-includes/plugin.php:395
    0.4897   28973656   4. media_upload_library() /Users/nacin/Sites/3.1/wp-includes/plugin.php:0
    0.4897   28973968   5. wp_iframe() /Users/nacin/Sites/3.1/wp-admin/includes/media.php:752
    0.4980   29005416   6. call_user_func_array() /Users/nacin/Sites/3.1/wp-admin/includes/media.php:338
    0.4980   29005752   7. media_upload_library_form() /Users/nacin/Sites/3.1/wp-admin/includes/media.php:0
    0.5679   29142016   8. wp_edit_attachments_query() /Users/nacin/Sites/3.1/wp-admin/includes/media.php:1846
    0.5689   29150032   9. wp() /Users/nacin/Sites/3.1/wp-admin/includes/post.php:923
    0.5689   29150080  10. WP->main() /Users/nacin/Sites/3.1/wp-includes/functions.php:1512
    0.5689   29150080  11. WP->parse_request() /Users/nacin/Sites/3.1/wp-includes/classes.php:519

The object is a post object of the first image in the library.

#26 @scribu
9 years ago

  • Owner changed from loushou to scribu
  • Status changed from reopened to reviewing

#27 @scribu
9 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

(In [15793]) Don't attempt to convert objects to strings in WP->parse_request(). Fixes #14330

#28 @scribu
9 years ago

Related: #15112

#29 @dd32
18 months ago

In 42419:

WP: Don't attempt to convert multiple-nested arrays to a string in WP->parse_request().

See #14330.
Fixes #42752.

Note: See TracTickets for help on using tickets.