WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

Last modified 5 months ago

#51722 closed enhancement (fixed)

Allow embedded links in preloaded api request

Reported by: spacedmonkey Owned by: TimothyBlynJacobs
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.0
Component: REST API Keywords: good-first-bug has-patch
Focuses: rest-api Cc:

Description

Currently the rest_preload_api_request function doesn't embed links / respect the ?_embed= url param. This means that embedded links in the REST API can not be prefetched.

Change History (18)

#1 @TimothyBlynJacobs
8 months ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

This will require changing rest_preload_api_request to instead call $server->response_to_data() instead of just calling $response->get_data() directly. And pull _embed from $query_params and apply rest_parse_embed_param() to it.

#2 @technosailor
7 months ago

I'm looking to jump in on this ticket but I'm trying to grok how/where this method is used for testing reasons. But as an FYI... I am looking to pick this ticket up.

#3 @TimothyBlynJacobs
7 months ago

@technosailor great! In WordPress Core it is used to bootstrap the Block Editor. You can see that in action here: https://github.com/WordPress/wordpress-develop/blob/c869ef617e03677279ff64b68630da823275988e/src/wp-admin/edit-form-blocks.php#L79

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


7 months ago

  • Keywords has-patch added; needs-patch removed

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

@spacedmonkey please find attached implementation of your idea from mentioned Trac ticket.

Please let me know if you would like me to modify this in any different way.

cc @TimothyBJacobs

#5 @TimothyBlynJacobs
6 months ago

  • Milestone changed from Future Release to 5.7

Thanks for picking this up @lukaspawlik! The approach looks right to me, would you be interested in adding tests for this?

#6 @prbot
5 months ago

lukaspawlik commented on PR #788:

@TimothyBJacobs Accepted your suggestion. This seems to be ready.

#7 @prbot
5 months ago

TimothyBJacobs commented on PR #788:

This looks great, thanks @lukaspawlik! Would you be interested in writing a test for this?

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


5 months ago

Adding a simple fix for 51722. The response_to_data method to resolve the links and embed fields.

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

#9 @spacedmonkey
5 months ago

@TimothyBlynJacobs I have created a simpler patch. If you seem to think that valid course, I will add some simple tests.

#10 @TimothyBlynJacobs
5 months ago

Thanks for picking it up @spacedmonkey. Both approaches look pretty similar to me. Missing in the second PR is a call to rest_parse_embed_param. I do like that in @lpawlik's patch we work off of the already parsed request object.

#11 @spacedmonkey
5 months ago

For what is worth, we are currently using my patch on a very popular plugin called web stories. See our plugin. https://github.com/google/web-stories-wp/pull/5149

This was test and code reviewed by me and @swissspidy . There is a benefit to using existing code that others are using.

#12 @prbot
5 months ago

spacedmonkey commented on PR #879:

@TimothyBJacobs I added unit tests now.

#13 @hellofromTonya
5 months ago

  • Keywords needs-testing-info added

Scheduling this ticket for Testing Scrub, but need more information for the testers to QA it:

  • What are the steps to test the PR?
  • Are there any testing dependencies such as a plugin or script?
  • What is the expected behavior after applying the patch?

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


5 months ago

#15 @TimothyBlynJacobs
5 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 50005:

REST API: Support embedding links in rest_preload_api_request().

Props lpawlik, spacedmonkey.
Fixes #51722.

#17 @prbot
5 months ago

TimothyBJacobs commented on PR #879:

Thanks for working on this, I combined this with @lukaspawlik's previous PR in https://github.com/WordPress/wordpress-develop/commit/bb395706f4ebe6efeca2a951fad69ba4d98fb799.

#18 @TimothyBlynJacobs
5 months ago

  • Keywords needs-testing needs-testing-info removed

@hellofromTonya I've removed needs-testing-info since this is purely a developer facing change that wouldn't have any user-facing impact and I think we have good test coverage on the change. Feel free to re-add though if you feel differently.

Note: See TracTickets for help on using tickets.