REST API: order is ignored when orderby is set to include

[joshlevinson]

The API currently supports the ability orderby=include as long as you've specified an array of post IDs to include. In this scenario,order is ignored–results are always returned in the same order as the IDs of the include array.

A tester reported an expectation that if orderby is set to include and order is desc, that the results would effectively be the "opposite" of the include array.

For example:
include=5,2,1&orderby=include and
include=5,2,1&orderby=include&order=desc
currently returns results like: [ { id: 5...}, { id: 2...}, { id: 1... } ]

If order is respected, specifying order=desc
would return results like: [ { id: 1...}, { id: 2...}, { id: 5... } ] (the "opposite" of the array)

Note that this behavior was brought up in testing the REST API, but it is not specific to the API. The current behavior is present in WP_Query, so if it is deemed that this behavior should change, it should probably be changed in WP_Query rather than the API.

[swissspidy]

Note that this behavior was brought up in testing the REST API, but it is not specific to the API. The current behavior is present in WP_Query, so if it is deemed that this behavior should change, it should probably be changed in WP_Query rather than the API.

Agree. The orderby param supports post__in, post_parent__in and post_name__in, so it would need to be supported for all of these.

I'm not sure if it should be supported, as it could be seen as a minor BC break. The alternative would be improved documentation.

[jnylen0]

Another option that seems reasonable to me would be to fix it at the API level only. We already make a bunch of fixes like this - the names of the orderby parameters are a minor example.

[joshlevinson]

I think it's reasonable to fix it at the API level if it we're concerned about a BC break. However, I don't see how anyone would be relying on the current behavior (since it doesn't really *do* anything, it's just ignored).

I'm pretty green in terms of the rules around BC breaks. I assume you'd agree with me that we can't avoid BC break 100% of the time. Would you say the rule of thumb is along the lines of "developers are likely to rely on X" in deciding to forgo making a BC breaking change?

[swissspidy]

Seeing it as an enhancement, how easy would it be to add this only to the API for now? It seems like adjusting WP_Query would be more future-proof.

Considering that 4.7 should be released on Tuesday, this is more like 4.8 material and we get enough time to change this once and for all with a proper dev-note etc.

[jnylen0]

Looking back at old tickets and realized I never responded to the latest questions here.

Would you say the rule of thumb is along the lines of "developers are likely to rely on X" in deciding to forgo making a BC breaking change?

Yes, I'd agree with that.

It seems like adjusting WP_Query would be more future-proof.

I agree with that too - so let's try to fix this properly in the next release.

[sanket.parmar]

I think we need to sort the post__in array based on order. I mean if order is desc then we have to sort post__in in reverse order and when order is asc, we have to sort post__in in normal order. So if include=5,2,1&orderby=include or include=5,2,1&orderby=include&order=desc is passed then result should be [ { id: 5...}, { id: 2...}, { id: 1... } ] and when include=5,2,1&orderby=include&order=asc is passed then result should be [ { id: 1...}, { id: 2...}, { id: 5... } ]. Let me know if it is correct.

[jnylen0]

Hi @sanket.parmar - thanks for the patch. This change does need unit tests also.

I don't think we should sort the array, just reverse it if needed. Returning results in the order specified in the request should correspond to orderby=asc.

[fibonaccina]

@jnylen0 If I understand correctly the intended behavior is to return the results in the same order as the include array (even if this input array isn’t sorted), by default. For example, if include=2,5,1 we should get the posts in this order: 2,5,1. When adding order=asc, the order should be the same as that in the include array (ex. 2,5,1). If we add order=desc, the order should be the reverse of the include array (ex 1,5,2).

Do I have this right? Happy to update the tests if this is not correct! The tests are failing right now, as the posts are returned in a sorted array even if the include array isn’t sorted. As well, the default behavior (when order is omitted) mimics desc order, not asc.

[jnylen0]

@fibonaccina thanks for writing tests. I agree with your description of the intended behavior. I would make a few minor changes to your unit tests in 39055_tests.diff​:

• Create the post objects in wpSetUpBeforeClass and delete them in wpTearDownAfterClass. This avoids boilerplate in the code and overhead when running the test suite.
• Remove the @ticket annotations. git blame serves this same purpose just fine. (Do we have it documented somewhere to use these?)
• Some whitespace issues: array( 1, 2, 3 ) instead of array (1, 2, 3); align array blocks on the => using spaces.

Another direction we could take these test cases is #39079, which avoids creating carefully arranged test objects in favor of testing the SQL clauses generated by WP_Query.

Finally it looks like our WP_Query ​sort order documentation needs some work, because it says that the default order is DESC, which is not currently the case if using include (internally post__in).

[swissspidy]

Remove the @ticket annotations. git blame serves this same purpose just fine. (Do we have it documented somewhere to use these?)

Yes, it's documented on ​https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/#groups. @ticket annotations are very helpful for quickly running tests related to a specific ticket, and way easier than using git blame. We usually add those for bug tickets, not enhancements or new tickets. In this case here I think they're absolutely fine.

[jnylen0]

The test is skipped if the ticket is still open – in these cases, we treat the test as a “known bug” that will likely fail.

Oh right, that thing. Okay, never mind my comment about the @ticket annotations. Thanks for the extra info.

[fibonaccina]

Updated unit tests with review feedback

[fibonaccina]

@jnylen0 I reviewed your work in 40037 just after I added these updated tests to the ticket. Sorry!
The SQL-oriented tests you added there are closer to the data. Relocating and adapting these tests to go with those seems to me like a more elegant solution.
I'll defer to you, whether it's better to leave the tests here or modify them to use the changes you added via ticket #39079 , which help the setup quite a bit. I suspect you'd prefer the latter, right?

[jnylen0]

No worries @fibonaccina! I also didn't link [40037] back to this ticket properly, which didn't help. Also, I missed something here: you are writing tests against WP_Query directly, whereas mine apply to the REST API posts controller. So, while a similar approach may be worth considering for WP_Query, your latest test cases look totally fine and that can be done later in another ticket.

There is an additional wrinkle which makes this change less straightforward: ​the default order of WP_Query is DESC. This is not currently respected when orderby is post__in, post_parent__in, or post_name__in.

With that in mind, here are the changes that would be needed in the WP_Query code:

• Look at the order parameter somewhere around #L1713 and use it to set a variable like $reverse_orderby_array. Unlike the rest of WP_Query, treat DESC differently than missing or empty. (Note we need to do this before #L2141 because we set the default order to DESC there.)
• If we need to reverse an array of IDs/slugs, and we are actually ordering by something that uses one of these arrays, there are three places where we need to do so: #L1945,1957,1967.

There is one more problem: the order parameter in WP_REST_Posts_Controller defaults to desc, like the rest of WP_Query. So, if we were to make the changes described above, then the default order of the results from a REST API request would end up being the reverse of the specified include array.

Given all that, and given that it's pretty easy for clients to specify the include or post__in array corresponding to the order in which they actually want to receive their results, I'm no longer convinced this change is worth it. Maybe we should just document the existing behavior instead, as it's arguably more reasonable than any of the alternatives. (I also notice that post_parent__in is missing from the orderby documentation - are there any others?)

Regardless of what we decide here, though, I am all in favor of adding test cases to improve coverage.

[fibonaccina]

@jnylen0 I see what you mean, thank you for providing this detailed background. I'm not so familiar with the codebase and I hadn't realized there is actually some complicated mapping of parameters happening in the REST API posts controller that might make the behavior different in the higher level API handling from WP_Query.

It would be ideal to keep the behavior of WP_Query in sync with that of the API posts controller. Cases where these diverge could cause documentation omissions like the one you caught.

As the changes required involve picking out the 3 outliers (post__in, post_parent__in, post_name__in) and adding custom handling for them, I second your position to not make this change, but in any case to set users' expectations correctly through documentation.

I've changed the tests ( 39055_tests_current_behavior.diff ) to reflect the current behavior. Hopefully my comments there convey to a prospective reader that if the 'include' array is not sorted when provided as input, we do sort it in the results. The tests are all passing.

[fibonaccina]

Tests changed to reflect current behavior

[jnylen0]

Hi @fibonaccina - in doing a more detailed review of 39055_tests_current_behavior.diff​ I found a couple more problems:

• The new tests live in tests/phpunit/tests/query/search.php which tests the search parameters specifically, but a better place is query/results.php. Note this file also has plenty of fixtures already.
• The new tests pass include and orderby => include to WP_Query. However, this should be post__in - include is only meaningful in the WP REST API, as part of the remapping done there to make parameter names more user-friendly (ref).

So, we need to change all instances of include to post__in in your new test cases, revise the logic so that they pass, and move them to query/results.php.

There are also some spacing issues in your patch:

• Should be array( 1, 2, 3 ) instead of array ( 1, 2, 3 )
• Should be foreach ( ... ) instead of foreach( ... )
• Multiline arrays should be indented with one tab, rather than matching the level of indentation of the opening (

See the ​PHP coding standards, in particular the sections on ​indentation and ​spacing.

I'm happy to either review an updated patch, or to make these revisions myself, as you prefer.

[fibonaccina]

Thanks a lot for the feedback @jnylen0 !

It seems my IDE (PhpStorm) was adding some unexpected indentation. Your link was very useful, I hadn't stumbled onto the WordPress-specific PHP coding standards yet, this will be very helpful going forward. I believe I've fixed the indentation, please let me know if anything doesn't look right. The newest version is 39055_tests_current_behavior_refresh.diff

As well, I wavered quite a bit whether to place the tests in search.php or results.php. Admittedly in the end I made a more or less educated guess, but especially now understanding the issue much better, there isn't a doubt in my mind that results.php is the correct place. Are there any resources I might have missed for guidance on where to place tests, or is just a matter of figuring it out on a case by case basis?

[fibonaccina]

Relocate tests and fix indentation

[jnylen0]

I think we are getting close here 😅

However, these tests don't match the expected behavior discussed earlier in the ticket, and they also fail on my WP install. It looks like you still have 39055.diff​ from earlier in the thread applied to your WP checkout.

Are there any resources I might have missed for guidance on where to place tests, or is just a matter of figuring it out on a case by case basis?

I'm not aware of any - I noticed on a closer look that everything in search.php was testing the search parameter, and results.php contained more generic tests.

[fibonaccina]

I was sure I was hallucinating. You're right, the patch was still applied. After removing the patch and reviewing the test results, they are as you concluded: (1) no order, and order=asc return posts in the same order as the input array, (2) order=desc also returns results in the same order (though it shouldn't), and we'll fix that with documentation instead of a code change. Latest patch is 39055_tests_current_behavior_refresh_take2.diff

[fibonaccina]

Correct test results

[jnylen0]

In 40056:

WP_Query: Add tests for the combination of orderby=post__in and order.

This commit adds test cases for the interaction (or more accurately, lack of
interaction) between orderby=post__in and the order parameter.

Props fibonaccina.
See #39055.

[jnylen0]

Thanks for sticking with this one, @fibonaccina. I've committed the new tests.

I've also updated the WP_Query sort order docs with ​a few changes resulting from a review of the docs vs the code and the inline documentation:

• Document the (lack of) interaction between orderby and order for post__in and similar parameters. This is a bit counter-intuitive but I believe it to be the best design overall, for reasons discussed above.
• Document the fact that post_* is accepted as well as * for many parameters (for example name, author).
• Document RAND(x) as well as rand.

With that done, and some test cases added, I'm going to close out this ticket.

Finally, I'd also accept similar test cases for post_name__in and post_parent__in, whether attached to this ticket or a new one.

[fibonaccina]

@jnylen0 Sorry for the delay - I agree we should have tests for post_name__in and post_parent__in for coverage as well as for contributors' reference, since the translation magic happening in WP_REST_Posts_Controller may confuse things a bit. So I went ahead and attached the remaining tests.

I noticed that a test for the post_name__in query with no order specified already exists in tests/post/query.php via ticket # 36515. So I added two more tests, for the post_name__in query with order 'asc' and 'desc'. The behavior is the same as with post_in - no difference in returned order between the two. I added the tests in tests/post/results.php but could also relocate them to be grouped with the already existing one in tests/post/query.php, let me know what you think.

As for the post_parent__in query, there is a test in tests/query/results.php which specifies order 'asc'. I added the remaining test with order 'desc' in the latest diff.

[fibonaccina]

Additional tests for 'post_namein' and 'post_parentin'

[jnylen0]

Fixed whitespace issues

[jnylen0]

The new tests look good, I uploaded 39055.2.diff​ to fix a few whitespace issues (trailing whitespace on one line, and spaces mixed with tabs inside the array indentations on a few other lines).

Compare the two patches on Trac and you can see some of this visually; the rest, I can do with cat -A patch.diff but I think these flags vary a bit by OS.

[jnylen0]

In 40237:

WP_Query: Add missing tests for combinations of orderby and include parameters.

This commit adds some missing test cases for combinations of orderby and other parameters (post_parent__in and post_name__in).

Followup to [40056] for orderby and post__in.

The interaction of these parameters is perhaps counterintuitive because orderby does not affect the returned results. This is overall probably the best design, and it's now better tested and documented.

Props fibonaccina.
See #39055.

[fibonaccina]

I missed those :( Thank you!