WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#40027 closed enhancement (fixed)

Tags and Categories should have a "slugs" parameter for batch fetching

Reported by: wonderboymusic Owned by: jnylen0
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests fixed-major i18n-change
Focuses: Cc:

Description

I have 8 categories but I want to query only 3:
1 taco
2 burrito
3 enchilada

/categories?slug=taco only returns one item because there is no batch slug mechanism

/categories?include[]=1&include[]=2&include[]=3 returns all 3

What I want:

/categories?slugs[]=taco&slugs[]=burrito&slugs[]=enchilada

The endpoints, by default, are way too slow to effectively be used with serial HTTP. The batching parameters are essential.

Attachments (8)

40027.diff (612 bytes) - added by MatheusGimenez 6 months ago.
40027.2.diff (3.1 KB) - added by curdin 6 months ago.
Updated functionality, Unit tests added
Screenshot from 2017-03-16 17:18:11.png (72.5 KB) - added by MatheusGimenez 5 months ago.
Screenshot from 2017-03-16 17:17:56.png (74.3 KB) - added by MatheusGimenez 5 months ago.
Screenshot from 2017-03-16 17:16:26.png (71.9 KB) - added by MatheusGimenez 5 months ago.
40027.3.diff (3.6 KB) - added by curdin 5 months ago.
Unit tests added to MatheusGimenez's patch
40027.4.diff (5.2 KB) - added by curdin 5 months ago.
Added CSV query arg tests
40027.5.diff (4.0 KB) - added by jnylen0 5 months ago.
Remove duplicate tests

Download all attachments as: .zip

Change History (35)

#1 @kadamwhite
6 months ago

  • Milestone changed from Awaiting Review to Future Release

From a design standpoint we've tried to make any list-style query parameter (include, exclude, categories, etc) behave the same... in this regard I agree that "slug" is the odd one out, because it is a filtering query that returns an array, but does not accept multiple values. I'm in favor of the change, as the benefit to clients is significant and the query overhead within WP should be significantly less of a burden than the HTTP latency or query overhead of making multiple requests in parallel or sequence

#2 @jnylen0
6 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 4.7.4

This is very similar to #38579 for posts. As Adam mentioned, we should use the same slug parameter but support arrays and comma-separated lists. Let's see if we can get it in the next minor release.

#3 @MatheusGimenez
6 months ago

  • Keywords needs-testing added; needs-patch removed

I added array support in the parameter slug in this patch.

Example to use:
SITE_URL/wp-json/wp/v2/categories/?slug[]=slug-1&slug[]=slug-2

Last edited 6 months ago by MatheusGimenez (previous) (diff)

@curdin
6 months ago

Updated functionality, Unit tests added

#4 @curdin
6 months ago

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-testing removed

Hi,

I've added unit tests. I've also noticed that the slugs parameter was previously slug. For backwards compatibility my patch contains both, slug for a single parameter, slugs for an array. Happy to update if this is not expected.

#5 @desrosj
6 months ago

  • Keywords needs-testing added

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


5 months ago

#7 @swissspidy
5 months ago

  • Owner set to jnylen0
  • Status changed from new to assigned

#8 follow-up: @jnylen0
5 months ago

  • Status changed from assigned to accepted

@curdin we want to use slug only, see comment:2 above.

#9 @danielbachhuber
5 months ago

+1 to slug handling a single or multiple values. This should be standard across all endpoints.

Some prior conversation:

#10 @curdin
5 months ago

Sure thing. I'm away for the next 3 days - I will send an updated patch on Monday.

#11 in reply to: ↑ 8 ; follow-up: @MatheusGimenez
5 months ago

Replying to jnylen0:

@curdin we want to use slug only, see comment:2 above.

My patch already do this.

#12 in reply to: ↑ 11 @curdin
5 months ago

Apologies if it does, I was under the impression it only did arrays but not strings. I'll check it out and merge my unit tests, then resubmit the patch.

Replying to MatheusGimenez:

Replying to jnylen0:

@curdin we want to use slug only, see comment:2 above.

My patch already do this.

#13 follow-up: @curdin
5 months ago

@MatheusGimenez - yep, sorry, my mistake.

I am adding patch now using your functional changes, and adding slug array unit tests to the category and tag controller tests.
This may be a question for another day but in https://github.com/WP-API/WP-API/issues/2065 there is discussion around posts and users also having slug filters. Should these also be amended to receive arrays?

@curdin
5 months ago

Unit tests added to MatheusGimenez's patch

#14 in reply to: ↑ 13 @jnylen0
5 months ago

Replying to curdin:

This may be a question for another day but in https://github.com/WP-API/WP-API/issues/2065 there is discussion around posts and users also having slug filters. Should these also be amended to receive arrays?

The posts endpoint already supports filtering by multiple slugs, see #38579. If the users endpoint doesn't, then yes, we should add that too. Want to create a separate ticket for that?

I would like to see one of the new test_get_items_slug_array_arg tests changed to test_get_items_slug_csv_arg with the slugs passed as a comma-separated string like taco,burrito,enchilada. Otherwise 40027.3.diff looks good to me and I will test and commit it later this week.

@curdin
5 months ago

Added CSV query arg tests

#15 @curdin
5 months ago

@jnylen0 Updated patch with CSV unit tests for both category and tags added above.

From my testing the user endpoint does not appear to accept an array or csv of slugs. I have opened #40213 to address this.

Last edited 5 months ago by curdin (previous) (diff)

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


5 months ago

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


5 months ago

#18 @swissspidy
5 months ago

From what I can tell, the latest patch works as intended.

#19 @jnylen0
5 months ago

  • Keywords commit added; needs-testing removed

In 40027.5.diff I removed some duplicate tests and made some minor changes to the test logic. This is looking good to me - I'll commit after the test suite finishes running.

@jnylen0
5 months ago

Remove duplicate tests

#20 @jnylen0
5 months ago

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

In 40376:

REST API: Allow fetching multiple terms at once via the slug parameter.

This matches a similar change previously made for posts (#38579) and an upcoming change for users (#40213).

Props wonderboymusic, MatheusGimenez, curdin.
Fixes #40027.

#21 @jnylen0
5 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 4.7 branch.

#22 @jnylen0
5 months ago

I missed something in [40376] - really this commit should include a string change for consistency with the posts endpoint:

- Limit result set to terms with a specific slug.
+ Limit result set to terms with one or more specific slugs.

Last I heard, we don't do string changes in minor releases. With the longer release cycle for 4.8, should we leave this rule in place, or relax it?

#23 @jnylen0
5 months ago

Per Slack discussion this is not expressly forbidden, and we already have at least one other string change in 4.7.4. I'm going to go ahead and fix this.

#24 @jnylen0
5 months ago

In 40377:

REST API: Update description string of terms endpoint slug parameter.

As a follow-up to [40376], and for consistency with the posts endpoint, we should indicate in the description that the slug filter parameter can accept multiple values.

See #40027.

#25 @ocean90
5 months ago

  • Keywords i18n-change added

Feel free to commit the string change, it doesn't necessary need to be in 4.7.4 though. Note that the other ticket isn't yet back-ported.

#26 @swissspidy
4 months ago

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

In 40427:

REST API: Allow fetching multiple terms at once via the slug parameter.

This matches a similar change previously made for posts (#38579) and an upcoming change for users (#40213).

Props wonderboymusic, MatheusGimenez, curdin.
Fixes #40027.

Merges [40376] to the 4.7 branch.

#27 @swissspidy
4 months ago

Note that I merged this to the 4.7 branch without the string change as none of the other string changes from the mentioned tickets have made it into this release. Better to keep it that way, given that we now release much earlier than originally planned.

Note: See TracTickets for help on using tickets.