Make WordPress Core

Opened 6 years ago

Closed 8 weeks ago

Last modified 8 weeks ago

#43439 closed defect (bug) (fixed)

&_embed only embedding first 10 categories

Reported by: manyourisms's profile manyourisms Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.9.4
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:

Description

Normally our posts don't have more than 10 categories, but I did notice that when adding more than 10 to a post, a query using &_embed still only embedding the first 10. After searching through the class-wp-rest-server.php code, it doesn't seem like that should be the case, but alas! I'm too unfamiliar with how it is building the links for a post behind the scenes, but surely it must be happening there if I was to guess.

Hitting the category link href at wp-json/wp/v2/categories?post=<postId> also only returns 10 categories.

Attachments (3)

10-categories.PNG (31.6 KB) - added by manyourisms 6 years ago.
Response showing it only coming back with 10 categories
many-categories.PNG (28.6 KB) - added by manyourisms 6 years ago.
Edit post page showing much more than 10 categories for the post.
43439-unit-test-update.diff (2.9 KB) - added by Mamaduka 2 months ago.

Download all attachments as: .zip

Change History (29)

@manyourisms
6 years ago

Response showing it only coming back with 10 categories

@manyourisms
6 years ago

Edit post page showing much more than 10 categories for the post.

#1 follow-up: @kadamwhite
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hitting the category link at wp-json/wp/v2/categories?post=<postId> and receiving 10 responses is expected; collections maintain standard pagination behavior when using query parameters like post=, so you can see the next page of responses with &page=2.

Only permitting up to 10 records to be embedded, however, is unexpected. Good catch! The origin of the issue appears to be that we use the REST API internally to embed links collections, and these requests obey the REST API's standard 10-post collection response limit. I'd definitely consider changing this to work in a more predictable way.

#2 in reply to: ↑ 1 @manyourisms
6 years ago

Wunderbar! Thanks for the clarification on the categories endpoint, sorry to have missed that.

Replying to kadamwhite:

Hitting the category link at wp-json/wp/v2/categories?post=<postId> and receiving 10 responses is expected; collections maintain standard pagination behavior when using query parameters like post=, so you can see the next page of responses with &page=2.

Only permitting up to 10 records to be embedded, however, is unexpected. Good catch! The origin of the issue appears to be that we use the REST API internally to embed links collections, and these requests obey the REST API's standard 10-post collection response limit. I'd definitely consider changing this to work in a more predictable way.

#3 follow-up: @rmccue
6 years ago

@kadamwhite What sort of changes did you have in mind for making this more predictable?

#4 in reply to: ↑ 3 @manyourisms
6 years ago

Replying to rmccue:

@kadamwhite What sort of changes did you have in mind for making this more predictable?

I would think the page parameter would apply only to the main resource you are requesting, IE posts in this case. I would expect all embeds for those 10 resources to come back (unless there was an additional paging param for the embeds for some reason).

Last edited 6 years ago by manyourisms (previous) (diff)

#5 @kadamwhite
6 years ago

I would also feel like we'd get back all embeds. If I have a resource that includes a link to that resource's categories, I would expect to be able to follow it and get a comprehensive list.

This obviously raises the question of how to handle situations (rare, but they happen) where one post may have 100+ tags or categories. I think to start we should hard-cap in that edge-case, but otherwise return a full list when embedded.

#6 @TimothyBlynJacobs
3 years ago

  • Keywords needs-unit-tests added

A naive solution to this would be to always append per_page=100 in WP_REST_Server::embed_links(). However that wouldn't be ideal for single routes, and we have no guarantee that collection routes actually implement their per_page cap at 100.

What we could do is inside of ::embed_links() use the new ::match_request_to_handler() and ::respond_to_request() methods to reimplement ::dispatch() but to first look if a per_page arg is registered and if so, find it's maximum value and apply it to the request.

@lpawlik are you interested in this one?

#7 @lpawlik
3 years ago

@TimothyBlynJacobs sure thing, I'll add it to my list 👍

This ticket was mentioned in PR #672 on WordPress/wordpress-develop by lukaspawlik.


3 years ago
#8

  • Keywords has-patch added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/43439

Allow _embed to include all embedded object such as taxonomies, comments etc.

lukaspawlik commented on PR #672:


3 years ago
#9

@TimothyBJacobs @spacedmonkey this is an initial commit for https://core.trac.wordpress.org/ticket/43439. It seems to be solving reported issue however, I am concerned about the fact that it's using \WP_REST_Server::match_request_to_handler that is modifying the state of passed request object, and the same method is called later via dispatch method. Is it our concern that it's called second time? Do you have any other ideas to address this issue?

#10 @TimothyBlynJacobs
3 years ago

Hey @lpawlik! Just following up on our conversation in Slack about how we could alleviate the duplicate matching issue.

#11 @lpawlik
3 years ago

Hey @TimothyBlynJacobs - thanks for the ping. I'll be working on this, later this week and I hope to have something ready for the review early next week :)

#12 @spacedmonkey
4 months ago

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

This ticket was mentioned in PR #5758 on WordPress/wordpress-develop by @spacedmonkey.


4 months ago
#13

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

#14 @spacedmonkey
2 months ago

  • Milestone changed from Future Release to 6.5

#15 @spacedmonkey
2 months ago

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

In 57623:

REST API: Set maximum 'per_page' for embedded REST API requests.

This enhancement refines the REST API server to automatically establish the maximum 'per_page' value for embedded objects, adhering to the endpoint's schema when not explicitly defined in the request. This adjustment elevates the limit from the default of 10 items to 100 items, significantly improving the likelihood of receiving the complete dataset of embedded objects.

Props manyourisms, lpawlik, spacedmonkey, kadamwhite, TimothyBlynJacobs.
Fixes #43439.

@spacedmonkey commented on PR #5758:


2 months ago
#16

Committed.

#17 @Mamaduka
2 months ago

I believe r57623 caused a regression for the navigation-fallback endpoint. The embedded object no longer returns the `raw` property values.

The regression was missed by the test_embedded_navigation_post_contains_required_fields test since it doesn't match actual usage. It makes separate requests with the embed context instead of using the _embed=true parameter.

More details in Gutenberg issue - https://github.com/WordPress/gutenberg/issues/58987.

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


2 months ago

#19 @Mamaduka
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I am reopening the ticket since regression needs to be resolved before we proceed with a committed solution.

@youknowriad commented on PR #6133:


2 months ago
#21

@TimothyBJacobs @spacedmonkey would you be able to help validate the fix here. This is a regression that happened during the 6.5 cycle, therefor, it would be good to land this fix on the same release.

@TimothyBlynJacobs commented on PR #6133:


2 months ago
#22

This fix looks good to me as well.

I'd love if we could have unit tests that are covering this specifically as part of the WP_REST_Server tests, instead of the incidental coverage we are getting as part of the navigation controller. But I don't think that needs to block this from getting merged considering the timing.

#23 @swissspidy
2 months ago

  • Keywords commit added
  • Owner changed from spacedmonkey to swissspidy
  • Status changed from reopened to assigned

#24 @swissspidy
8 weeks ago

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

In 57659:

REST API: Pass correct context to embedded items.

Fixes a regression introduced in [57623] where navigation embed items were missing raw property values.

Props mamaduka, swissspidy, youknowriad, timothyblynjacobs.
Fixes #43439.

Note: See TracTickets for help on using tickets.