#38693 closed enhancement (fixed)
REST API: Audit `orderby` params and add any others
Reported by: | jnylen0 | Owned by: | jnylen0 |
---|---|---|---|
Milestone: | 4.8 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
Migrated from https://github.com/WP-API/WP-API/issues/2906. It looks like at some point we dropped the capability to order by author and taxonomy term, probably when we introduced mandatory enum
validation. What other orderby
parameters can we easily support?
Attachments (7)
Change History (41)
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#4
@
8 years ago
As discussed in github, I think the useful mising 'orderby' parameters are: 'none', 'modified', 'parent', 'rand' and 'meta_value_num'. In addition, it might be helpful to introduce a filter so developers could alter which fields were accepted.
#5
@
8 years ago
The "rest_{$this->post_type}_collection_params" filter can be used to add in additional orderby
values. To add random, you could hook in like this:
<?php add_filter( 'rest_post_collection_params', 'my_prefix_add_rest_orderby_params', 10, 1 ); function my_prefix_add_rest_orderby_params( $params ) { $params['orderby']['enum'][] = 'rand'; return $params; }
If there is a consensus on what should be added as supported defaults, I can make a patch. The hook is already available to use.
#6
@
8 years ago
- Keywords needs-patch added
- Type changed from enhancement to defect (bug)
The collection params filters support this use case really nicely - thanks for the example.
meta_value_num
requires meta_key
to be set in the query so we shouldn't add that one.
I can't think of any reason not to add all of the other parameters mentioned above ('none', 'modified', 'parent', 'rand').
Setting this back to "bug" as it's a regression from previously established behavior.
#8
@
8 years ago
So add in none
, modified
, parent
, and rand
, author
? I don't know how to test for random sorting but I will add test cases for the others.
#9
@
8 years ago
My only concern with this is rand
as that's not idempotent, so this will lead to reverse-cache issues etc. Also, rand
is pretty bad for performance too, so I'm a bit hesitant with that one.
#13
@
8 years ago
- Keywords has-patch has-unit-tests added; needs-patch removed
- Milestone changed from Awaiting Review to 4.7
Thanks for patching this, @ChopinBach. Looks good to me apart from a couple of minor notes.
A few of the new tests don't have strictly defined order:
test_get_items_orderby_author_query
test_get_items_orderby_none_query
test_get_items_orderby_parent_query
For none
it's fine to just assert that the response is a 200. For the others, you could either verify that the list of authors/parents is sorted correctly, or make sure each post has a unique author/parent.
Also, rather than copy/paste update_post_modified
from tests/query/dateQuery.php
, let's move it to the base WP_UnitTestCase
class.
@
8 years ago
Adds orderby params author, parent, none, and modified to the posts endpoint for the REST API. Unit tests revised to not rely upon secondary sorting by SQL. Line endings should be fixed as well.
@
8 years ago
This one should actually work. Adds orderby params author, parent, none, and modified to the posts endpoint for the REST API. Unit tests revised to not rely upon secondary sorting by SQL. Line endings should be fixed as well.
@
8 years ago
Created by Linux not Windows so the Line endings should be fine now. Curse you WINDOWS! Adds orderby params author, parent, none, and modified to the posts endpoint for the REST API. Unit tests revised to not rely upon secondary sorting by SQL. Line endings should be fixed as well.
#14
@
8 years ago
- Keywords commit added
Minor change in 38693.6.diff: I removed the old copy of update_post_modified
now that it lives in the base test class. This is good to commit.
Unfortunately I found another bug with orderby
while testing this → #38985
#15
@
8 years ago
Hmm reading through the patch why are we supporting none
? That doesn't make any sense to me. Who would want that, and even if we did, wouldn't orderby => null
be better?
#16
@
8 years ago
Also, why are we trying to get this in 4.7 for an enhancement after RC1? Is that defo necessary?
#17
follow-up:
↓ 20
@
8 years ago
This is not an enhancement - this is a regression from previously established behavior that at least 3 different people have asked about.
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#19
@
8 years ago
- Keywords commit removed
Removing commit
for workflow reasons - need commit
from one permanent committer and dev-reviewed
from another, please.
#20
in reply to:
↑ 17
@
8 years ago
- Keywords 2nd-opinion added
- Type changed from defect (bug) to enhancement
Replying to jnylen0:
This is not an enhancement - this is a regression from previously established behavior that at least 3 different people have asked about.
@jnylen0 I disagree. This is an enhancement, and I am not comfortable with trying to squeeze this into 4.7.
What about adding a filter for the orderby
enum params so sites can expand the supported orderby values for 4.7, and properly fixing in 4.8?
#21
@
8 years ago
What about adding a filter for the orderby enum params so sites can expand the supported orderby values for 4.7, and properly fixing in 4.8?
Given the collection params are filterable, i don't think there's any benefit to adding 2-3 values to the enum that won't actually do anything. A plugin that wants to add support can do so via the filter.
This is not an enhancement - this is a regression from previously established behavior that at least 3 different people have asked about.
I don't agree this isn't an enhancement, we haven't had these order by values for some time. IMO now is the time to punt this to 4.8, it's fine that we add it then.
#22
follow-up:
↓ 26
@
8 years ago
- Keywords 2nd-opinion removed
- Milestone changed from 4.7 to 4.8
Okay, let's take a look at adding these via a filter in a separate ticket. Thank y'all for weighing in.
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
8 years ago
#25
@
8 years ago
note: I closed #39657 which was asking about adding menu_order
to the accepted orderby
options.
#26
in reply to:
↑ 22
;
follow-up:
↓ 28
@
8 years ago
Replying to jnylen0:
Okay, let's take a look at adding these via a filter in a separate ticket. Thank y'all for weighing in.
Has this separate ticket been created? I'd be happy to see the random orderby reappear--but it was apparently decided that would be plugin territory. If that's so, would I need to code that myself?
Thanks!
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#28
in reply to:
↑ 26
@
8 years ago
- Owner set to jnylen0
- Status changed from new to accepted
Unless there are any objections, I'll plan to land these additional parameters in the next few days. 38693.6.diff is basically ready, but we can improve/simplify the tests considerably after #39079.
Replying to openhatch:
Has this separate ticket been created?
comment:5 looks pretty good as a code sample, so I'm not sure we need to do anything else regarding a filter, unless that doesn't work well for some reason.
#29
@
8 years ago
- Keywords commit added
In 38693.7.diff:
- Refresh against latest trunk
- Explicitly test
ORDER BY
clauses used by neworderby=
methods - Remove
orderby=none
(see comment:15; let's revisit this later if needed) - Update
wp-api.js
test fixtures
Joining the discussion as I'd love to see some of the
orderby
options, likerand
, being brought back to the API (background).If some of the options can't be added back, I think it'd be nice if we could add options back on a case-by-base basis, thanks to a filter.