Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#38693 closed enhancement (fixed)

REST API: Audit `orderby` params and add any others

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

rest_post_orderby.diff (9.4 KB) - added by ChopinBach 8 years ago.
Patch for rest post orderby parameters.
rest_post_orderby.2.diff (10.2 KB) - added by ChopinBach 8 years ago.
Fixed the test in this patch.
rest_post_orderby.3.diff (10.4 KB) - added by ChopinBach 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.
rest_post_orderby.4.diff (5.4 KB) - added by ChopinBach 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.
rest_post_orderby.5.diff (5.4 KB) - added by ChopinBach 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.
38693.6.diff (6.4 KB) - added by jnylen0 8 years ago.
Avoid duplicating update_post_modified helper
38693.7.diff (8.6 KB) - added by jnylen0 8 years ago.
Test ORDER BY clauses explicity; remove orderby=none

Download all attachments as: .zip

Change History (41)

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


8 years ago

#2 @helen
8 years ago

  • Type changed from defect (bug) to enhancement

#3 @jeherve
8 years ago

Joining the discussion as I'd love to see some of the orderby options, like rand, 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.

#4 @adamsilverstein
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.

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

#5 @ChopinBach
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 @jnylen0
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.

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

#7 @jnylen0
8 years ago

Oh, also author, from the GitHub issue linked in the OP.

#8 @ChopinBach
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 @joehoyle
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.

#10 @jnylen0
8 years ago

Leaving rand as plugin territory and adding the others seems reasonable to me.

@ChopinBach
8 years ago

Patch for rest post orderby parameters.

#11 @ChopinBach
8 years ago

I have to revise that patch one of the tests doesn't work right.

@ChopinBach
8 years ago

Fixed the test in this patch.

#12 @ChopinBach
8 years ago

The second patch should be good.

#13 @jnylen0
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.

@ChopinBach
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.

@ChopinBach
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.

@ChopinBach
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.

@jnylen0
8 years ago

Avoid duplicating update_post_modified helper

#14 @jnylen0
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 @joehoyle
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 @joehoyle
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: @jnylen0
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 @helen
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 @rachelbaker
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 @joehoyle
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: @jnylen0
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

#24 @adamsilverstein
8 years ago

#39657 was marked as a duplicate.

#25 @adamsilverstein
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: @openhatch
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 @jnylen0
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.

@jnylen0
8 years ago

Test ORDER BY clauses explicity; remove orderby=none

#29 @jnylen0
8 years ago

  • Keywords commit added

In 38693.7.diff:

  • Refresh against latest trunk
  • Explicitly test ORDER BY clauses used by new orderby= methods
  • Remove orderby=none (see comment:15; let's revisit this later if needed)
  • Update wp-api.js test fixtures

#30 @jnylen0
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 40605:

REST API: Add author, modified, and parent sort order options for posts.

These (and a few others that can be revisited later if needed) were present in
beta versions of the WP REST API but were removed during the merge to WP 4.7.

Props ChopinBach, jnylen0.
Fixes #38693.

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-restapi by schlessera. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-editor by aduth. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.