#39055 closed enhancement (wontfix)
REST API: order is ignored when orderby is set to include
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Query | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
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.
Attachments (8)
Change History (32)
#1
@
8 years ago
- Component changed from REST API to Query
- Keywords dev-feedback added
- Type changed from defect (bug) to enhancement
- Version trunk deleted
#2
@
8 years ago
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.
#3
@
8 years ago
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?
#4
@
8 years ago
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.
#5
@
8 years ago
- Keywords needs-patch added; dev-feedback removed
- Milestone changed from Awaiting Review to 4.8
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.
@
8 years ago
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.
#7
@
8 years ago
- Keywords needs-unit-tests added
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
.
#8
@
8 years ago
- Keywords needs-unit-tests removed
@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.
#9
@
8 years ago
- Keywords has-unit-tests needs-refresh added
- Owner set to jnylen0
- Status changed from new to accepted
@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 inwpTearDownAfterClass
. 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 ofarray (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
).
#10
@
8 years ago
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.
#11
@
8 years ago
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.
#12
@
8 years ago
@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?
#13
follow-up:
↓ 14
@
8 years ago
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 ofWP_Query
, treatDESC
differently than missing or empty. (Note we need to do this before #L2141 because we set the default order toDESC
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.
#14
in reply to:
↑ 13
@
8 years ago
@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.
#15
follow-up:
↓ 16
@
8 years ago
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 isquery/results.php
. Note this file also has plenty of fixtures already. - The new tests pass
include
andorderby => include
to WP_Query. However, this should bepost__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 ofarray ( 1, 2, 3 )
- Should be
foreach ( ... )
instead offoreach( ... )
- 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.
#16
in reply to:
↑ 15
@
8 years ago
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?
#17
@
8 years ago
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.
#18
@
8 years ago
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
#20
@
8 years ago
- Keywords needs-refresh removed
- Milestone 4.8 deleted
- Resolution set to wontfix
- Status changed from accepted to closed
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
andorder
forpost__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 examplename
,author
). - Document
RAND(x)
as well asrand
.
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.
#21
@
8 years ago
@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.
#22
follow-up:
↓ 24
@
8 years ago
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.
Agree. The
orderby
param supportspost__in
,post_parent__in
andpost_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.