Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#38034 closed defect (bug) (fixed)

post__in orderby not working when passed in an array to orderby

Reported by: kelvink's profile kelvink Owned by: boonebgorges's profile 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)

class-wp-query.diff (1.5 KB) - added by kelvink 8 years ago.
38034.diff (2.4 KB) - added by boonebgorges 8 years ago.
38034.2.diff (2.5 KB) - added by boonebgorges 6 years ago.
38034.3.diff (6.7 KB) - added by mgibbs189 6 years ago.
38034.4.diff (6.7 KB) - added by mgibbs189 6 years ago.
38034.5.diff (6.8 KB) - added by mgibbs189 6 years ago.
Fixed remaining PHPUnit errors
38034.6.diff (4.7 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (35)

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

#3 follow-up: @helen
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 @kelvink
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.

Last edited 8 years ago by kelvink (previous) (diff)

#5 @maccast
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 @AllysonSouza
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.

#7 @boonebgorges
8 years ago

In 40278:

Tests: Use assertSame() for WP_Query 'orderby' tests.

assertEqualSets() ignores order, so isn't much good for testing 'orderby'.

See #38034.

#8 follow-up: @boonebgorges
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.

@boonebgorges
8 years ago

#9 in reply to: ↑ 8 @maccast
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 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.

#10 @boonebgorges
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 @thefraj
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 @boonebgorges
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 @mgibbs189
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).

@boonebgorges
6 years ago

#14 @boonebgorges
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'?

#15 @mgibbs189
6 years ago

@boonebgorges Sure thing, our team will take a closer look :)

@mgibbs189
6 years ago

#16 @mgibbs189
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 @boonebgorges
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 @boonebgorges
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 @mgibbs189
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.

@mgibbs189
6 years ago

@mgibbs189
6 years ago

Fixed remaining PHPUnit errors

@boonebgorges
6 years ago

#20 @boonebgorges
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, and post_name__in in parse_orderby(), and move the clause generation to the switch block where other orderby values are handled
  • For these values of orderby, force order to an empty string. Do this in get_posts() instead of passing the values around and doing it in parse_order(), etc. Note that I'm forcing an empty string rather than respecting the value of order passed to the function: it was decided as part of #39055 that order 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 @mgibbs189
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 @mgibbs189
6 years ago

@boonebgorges Since your patch looks good, could we get this queued up for core inclusion?

#23 @boonebgorges
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 @mgibbs189
6 years ago

@boonebgorges No worries, just making sure you didn't need anything else from me :)

#25 @boonebgorges
6 years ago

  • Milestone changed from Future Release to 5.1

#26 @boonebgorges
6 years ago

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

In 44452:

Query: Standardize treatment of 'orderby' values post__in, post_parent__in, and post_name__in.

Ordering by post__in was introduced in [21776], but the code assumed that
post__in would be a comma-separated string listing post IDs. When an array
of post IDs was passed to the post__in query var, 'orderby=postin' was
not respected. This changeset changes this behavior by handling
'orderby=post
in' in the same way as most other values of 'orderby',
which ensures that arrays as well as strings can be properly parsed.

The same treatment is given to the similar post_name__in and
post_parent__in options of 'orderby', so that most query generation for
orderby clauses happens in the same place, instead of in special cases.

A slight change in the resulting SQL (related to the whitespace around
parentheses and commas) necessitates a change to an existing REST API test
that does a string comparison against the SQL query.

Props mgibbs189, kelvink.
Fixes #38034.

#27 @pento
6 years ago

In 44456:

Query: Fix some code formatting issues introduced in [44452].

See #38034.

#28 @kelvink
6 years ago

Thank you so much for resolving!

Note: See TracTickets for help on using tickets.