Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#40826 closed enhancement (fixed)

Allow items queried in batch via "slug" to maintain order

Reported by: wonderboymusic's profile wonderboymusic Owned by: kadamwhite's profile kadamwhite
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7.4
Component: REST API Keywords:
Focuses: Cc:

Description

When querying endpoints that have items, it is possible to pass an array of values to the slug param. This will filter the result set to only include slugs that match. This works for posts, pages, media, users, categories, and tags.

Often, the reason this is happening is so that a remote system can collect all relevant queries before sending one HTTP request that contains them all. This was formalized in #40027.

One caveat: the endpoints do not currently have a proper orderby value that ensures the items are returned in the same order they requested.

The best convention I could come up with is: orderby=slugs. Patch incoming.
This is essential for tools like Dataloader https://github.com/facebook/dataloader#batching

Attachments (5)

slugs-patch.diff (12.1 KB) - added by wonderboymusic 7 years ago.
40826.diff (12.5 KB) - added by wonderboymusic 7 years ago.
40826.2.diff (12.7 KB) - added by kadamwhite 7 years ago.
40826.3.diff (12.0 KB) - added by kadamwhite 7 years ago.
40826.4.diff (12.9 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (25)

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


7 years ago

#2 @jnylen0
7 years ago

  • Keywords has-unit-tests needs-refresh added
  • Milestone changed from 4.7.6 to 4.8.1

This change seems fine for a 4.8.x release, but I have a few specific points of feedback.

  1. I think included_slugs is a better name for this new orderby parameter. It's very similar to the existing include functionality that deals with IDs, so the word include should appear in the name somewhere. slugs is too similar to the existing slug which just means "order by slug".
  1. The new functionality added to WP_Term_Query should follow the naming convention of similar functionality in the rest of the WP_*_Query classes: fieldname__in (slug__in in this case).
  1. At least for posts, you can use the $this->assertPostsOrderedBy() helper to make for more precise tests and avoid having to create a bunch of test fixtures. With wider use, this should speed up the test suite pretty significantly.

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


7 years ago

#4 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

@wonderboymusic
7 years ago

#5 @wonderboymusic
7 years ago

  • Keywords needs-refresh removed

40826.diff is a refresh

#6 @joehoyle
7 years ago

I don't want to bike shed the naming, so feel free to ignore. We have include for "order by the IDs passed", which is not past tense, so I'd be +1 for include_slugs.

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


7 years ago

#8 @kadamwhite
7 years ago

  • Owner set to kadamwhite
  • Status changed from new to accepted

I agree with @joehoyle on preferring include_slugs; I'll review this over the weekend and we should be able to get it in for 4.9. Thanks, @wonderboymusic!

@kadamwhite
7 years ago

#9 @kadamwhite
7 years ago

Added 40826.2.diff to change included_slugs to include_slugs, there should be no other differences between the two

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


7 years ago

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


7 years ago

@kadamwhite
7 years ago

#12 @kadamwhite
7 years ago

Fixed an issue with the previous patch I uploaded (had a stray test from another ticket), and added a @since note for the slugin addition to the WP_Term_Query constructor per feedback from @ocean90

@boonebgorges
7 years ago

#13 @boonebgorges
7 years ago

40826.4.diff is a refresh of 40826.3.diff that uses sanitize_title_for_query() rather than sanitize_title(). See the linked Slack discussion, as well as #19292 and [19444].

sanitize_title() is incorrectly used elsewhere in WP_Term_Query in a similar way. I'll open a separate ticket for that.

#14 @kadamwhite
7 years ago

  • Keywords commit added

#15 @kadamwhite
7 years ago

  • Type changed from defect (bug) to enhancement

#16 @kadamwhite
7 years ago

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

In 41760:

REST API: Support ordering response collection by listed slugs.

Adds an "include_slug" orderby value for REST API collections to permit returning a collection filtered by slugs in the same order in which those slugs are specified.
Previously, the order of slugs provided with the ?slug query parameter had no effect on the order of the returned records.

Props wonderboymusic, ocean90, boonebgorges.
Fixes #40826.

#17 @pento
7 years ago

  • Keywords has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[41760] is causing Travis failures.

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


7 years ago

#19 @SergeyBiryukov
7 years ago

In 41769:

REST API: After [41760], use correct query variable in WP_Term_Query::parse_orderby() when 'include_slugs' => 'slug__in' is passed.

See #40826.

#20 @pento
7 years ago

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

[41769] fixed the test error.

Note: See TracTickets for help on using tickets.