#38034 closed defect (bug) (fixed)
post__in orderby not working when passed in an array to orderby
Reported by: | kelvink | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | Query | Keywords: | has-patch |
Focuses: | Cc: |
Description
Currently order by post__in
only works if you pass it as a string. If you pass it inside an array, it won't orderby post__in
at all.
To reproduce:
<?php $query = new WP_Query( array( 'post__in' => array( 1,2,3,4,5,6,7,8 ), 'orderby' => array( 'post__in' => 'desc', 'title' => 'asc' ) ));
Attachments (7)
Change History (35)
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#3
follow-up:
↓ 4
@
8 years ago
- Keywords reporter-feedback added
- Version changed from trunk to 4.6
Hi @kelvink - thanks for your report. Could you also provide the query set up that does work for you?
Not new to trunk, so moving back to 4.6, though it would be something that's existed longer than that I imagine.
#4
in reply to:
↑ 3
@
8 years ago
Replying to helen:
Hi @kelvink - thanks for your report. Could you also provide the query set up that does work for you?
Not new to trunk, so moving back to 4.6, though it would be something that's existed longer than that I imagine.
Works:
<?php $query = new WP_Query( array( 'post__in' => array( 19, 21, 27, 16 ), 'orderby' => 'post__in' ));
Does NOT work:
<?php $query = new WP_Query( array( 'post__in' => array( 19, 21, 27, 16 ), 'orderby' => array( 'post__in' ) ));
My fix makes post__in
orderby work if you pass it in an array. So you can use the post__in
orderby in conjunction with other ordering parameters.
I can submit a patch for version 4.6 as well, if needed.
#5
@
8 years ago
Not sure if this has gotten worse or if I'm experiencing a different issue, but I have a site where the post__in
sort order is not working anymore when it was in the past. For me it's doesn't work even if i use a string or an array. It was working in the past and I only noticed the issue after upgrading to Wordpress 4.7.2.
Here is my code for reference:
$query_images_args = array( 'post__in' => explode(',',$include), 'post_parent' => $thePost->ID, 'post_type' => 'attachment', 'post_mime_type' =>'image', 'post_status' => 'inherit', 'orderby' => 'post__in' ); $featuredPosts = new WP_Query( $query_images_args );
Another possible difference is I'm getting the gallery attachments associated with the post, but my $include
contains the a comma seperatedstring of the gallery image_ids in the order I want them sorted in.
#6
@
8 years ago
I'm also having this problem, ordeby post__in
not work even in string. That's my code:
<?php $membros = array( 156, 159, 153 ); $args = array( 'post_type' => 'time', 'posts_per_page' => -1, 'post__in' => $membros, 'orderby' => 'post__in', ); ?>
The posts are displayed in the default admin list order, ignoring the post__in
.
#8
follow-up:
↓ 9
@
8 years ago
- Keywords needs-patch added; reporter-feedback removed
- Milestone changed from Awaiting Review to Future Release
I'm unable to reproduce the problems described by @AllysonSouza and @maccast. It's possible that they indicate some other issue.
What @kelvink describes is, indeed, a problem, and the approach in the patch looks correct. I'm about to attach a patch that also contains a test demonstrating the issue.
However, the patch causes some other orderby tests to fail. The reason is this. Once you start treating post__in
as a "regular" orderby value instead of a special case, it means that you automatically start inheriting the order
parameter too. And since the default order
param is DESC
, WP_Query( array( 'orderby' => 'post__in' ) )
starts returning results in reverse. This is too big a backward compatibility break. So we need to add some logic somewhere that indicates that when you pass orderby=post__in
but don't specify order
, you intend the order to be ASC
.
#9
in reply to:
↑ 8
@
8 years ago
Thanks of checking this out. Is there possibly some additional information or details I could provide to help determine why @AllysonSouza and I are seeing a different but similar issue to the one being described here?
Replying to boonebgorges:
I'm unable to reproduce the problems described by @AllysonSouza and @maccast. It's possible that they indicate some other issue.
What @kelvink describes is, indeed, a problem, and the approach in the patch looks correct. I'm about to attach a patch that also contains a test demonstrating the issue.
However, the patch causes some other orderby tests to fail. The reason is this. Once you start treating
post__in
as a "regular" orderby value instead of a special case, it means that you automatically start inheriting theorder
parameter too. And since the defaultorder
param isDESC
,WP_Query( array( 'orderby' => 'post__in' ) )
starts returning results in reverse. This is too big a backward compatibility break. So we need to add some logic somewhere that indicates that when you passorderby=post__in
but don't specifyorder
, you intend the order to beASC
.
#10
@
8 years ago
@maccast The first thing I'd check is the SQL that's being generated in your case by WP_Query
($this->request
).
#11
@
8 years ago
Hi All,
I came across this thread when I realised that the posts-in operator actually doesn't seem to work at all in the latest versions of Wordpress.
Below is the sort of code I was using, it will not produce any results under any conditions that I can see (even if pages by this Id exist). I also tried casing using foreach ($posts as $p) : ... ... blah blah but have not been able to get any results back
<?php $args = array( 'post__in' => array(25199, 27, 1448, 6509) ); $query = new WP_Query( $args ); ?> <?php if ( $query->have_posts() ) : ?> <?php while ( $query->have_posts() ) : $query->the_post(); ?> <h2><?php the_title(); ?></h2> <?php echo the_content(); ?> <?php endwhile; ?> <?php endif; ?> <?php wp_reset_postdata(); ?>
#12
@
8 years ago
@thefraj post__in
is definitely working more generally. See, for instance, the post__in
tests in tests/phpunit/tests/query/results.php. In your case, you're probably not passing the proper post_type
or post_status
to WP_Query
. If you have done further debugging and determined that it's definitely a core bug, please post your full analysis, including post type definition and the SQL generated by your WP_Query
instance (and, ideally, automated tests demonstrating the failure).
#13
@
6 years ago
- Keywords has-patch added; needs-patch removed
@boonebgorges could we revisit this one? Your included patch looks good (and thanks to @kelvink for first reporting).
#14
@
6 years ago
- Keywords needs-patch added; has-patch removed
@mgibbs189 Yes, I think we could revisit, but my comment above about 'order' still holds. 38034.2.diff updates the patch for current trunk, and adds another test that demonstrates the failure. Can someone work on a patch that will account for the proper default value of 'order' when 'orderby=postin'?
#16
@
6 years ago
- Keywords has-patch added; needs-patch removed
@boonebgorges The new patch accounts for the proper order
when using post__in
, and the order/orderby code itself has been tweaked a bit for clarity.
#17
@
6 years ago
Thanks, @mgibbs189. Am I correct that lines 1644-1647 (right side) of 38034.3.diff are the key changes from 38034.2.diff ? I ask because we generally don't mix general code factoring with bug fixes, so they'll have to go in separately. (and in fact, we shy away from doing general code refactoring without cause, as it risks introducing bugs. That said, your changes certainly look ok at a glance :) )
#18
@
6 years ago
Additionally, I'm seeing some test failures:
1) Tests_Query_Results::test_query_orderby_post_parent__in_with_order_desc Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( - 0 => 'child-three' - 1 => 'child-four' - 2 => 'child-one' - 3 => 'child-two' + 0 => 'child-one' + 1 => 'child-two' + 2 => 'child-three' + 3 => 'child-four' /home/bgorges/sites/wpdev/tests/phpunit/tests/query/results.php:619 2) Tests_Query_Results::test_query_orderby_post__in_with_order_desc Failed asserting that Array &0 ( 0 => 1626 1 => 1628 2 => 1627 ) is identical to Array &0 ( 0 => 1627 1 => 1628 2 => 1626 ). /home/bgorges/sites/wpdev/tests/phpunit/tests/query/results.php:679 3) Tests_Query_Results::test_query_orderby_post_name__in_with_order_desc Failed asserting that Array &0 ( 0 => 'parent-three' 1 => 'parent-one' 2 => 'parent-two' ) is identical to Array &0 ( 0 => 'parent-two' 1 => 'parent-one' 2 => 'parent-three' ). /home/bgorges/sites/wpdev/tests/phpunit/tests/query/results.php:714
I haven't looked into these in detail, but they appear to be related. Could you please run grunt copy:files && phpunit --group=post,query
to have a look?
#19
@
6 years ago
@boonebgorges Yes and no. Some of the logic was moved to parse_order(), such as handling of rand
and other edge cases.
The code for supporting an array-based orderby
had many overlaps to that of a space-separated string orderby
(e.g. orderby => 'title menu_order'), so combining it leaves fewer places that need sanitization.
Another patch incoming.
#20
@
6 years ago
Thanks, @mgibbs189. 38034.5.diff feels like a lot of changes for what seems like a fairly small bit of logic. 38034.6.diff is another approach:
- Remove special handling for
post__in
,post_parent__in
, andpost_name__in
inparse_orderby()
, and move the clause generation to theswitch
block where otherorderby
values are handled - For these values of
orderby
, forceorder
to an empty string. Do this inget_posts()
instead of passing the values around and doing it inparse_order()
, etc. Note that I'm forcing an empty string rather than respecting the value oforder
passed to the function: it was decided as part of #39055 thatorder
would be ignored in these cases (a decision I agree with). See https://core.trac.wordpress.org/ticket/39055#comment:13 and [40056] - Fix a REST API test that was sensitive to the whitespace in the generated SQL clause
Could you have a look to see whether this approach makes sense to you, and whether it works?
#21
@
6 years ago
@boonebgorges Yes, I like how you moved everything into parse_orderby()
, it flows better that way. Thanks for all your help and quick responses thus far.
I do wish that this code received a little more TLC, but I understand the hesitation to refactor working code.
#22
@
6 years ago
@boonebgorges Since your patch looks good, could we get this queued up for core inclusion?
#23
@
6 years ago
Hi @mgibbs189 - I've got this ticket on my list of things to look at, but please be patient - it's only been Saturday and Sunday since last update :-D
#24
@
6 years ago
@boonebgorges No worries, just making sure you didn't need anything else from me :)
Related: https://core.trac.wordpress.org/changeset/34836