Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#51722 closed enhancement (fixed)

Allow embedded links in preloaded api request

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

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 (20)

#1 @TimothyBlynJacobs
6 years ago

  • Keywords good-first-bug added
  • Milestone Awaiting Review → 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
6 years 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
6 years 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.


6 years ago
#4

  • 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 years ago

  • Milestone Future Release → 5.7

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

​lukaspawlik commented on ​PR #788:


6 years ago
#6

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

​TimothyBJacobs commented on ​PR #788:


6 years ago
#7

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.


6 years ago
#8

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
6 years ago

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

#10 @TimothyBlynJacobs
6 years 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
6 years 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.

​spacedmonkey commented on ​PR #879:


6 years ago
#12

@TimothyBJacobs I added unit tests now.

#13 @hellofromTonya
6 years 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.


6 years ago

#15 @TimothyBlynJacobs
6 years ago

  • Owner set to TimothyBlynJacobs
  • Resolution → fixed
  • Status new → closed

In 50005:

REST API: Support embedding links in rest_preload_api_request().

Props lpawlik, spacedmonkey.
Fixes #51722.

​TimothyBJacobs commented on ​PR #879:


6 years ago
#17

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

#18 @TimothyBlynJacobs
6 years 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.

This ticket was mentioned in ​PR #2340 on ​WordPress/wordpress-develop by ​jsnajdr.


5 years ago
#19

Suppose a preloaded request includes a _fields query param that asks that only selected response fields are returned. You can add such a request with a filter:
{{{php
function add_filtered_index_request( $preload_paths ) {

array_push( $preload_paths, '/?_fields=home,title' );
return $preload_paths;

}
add_filter( 'block_editor_rest_api_preload_paths', 'add_filtered_index_request' );
}}}
However, you will discover that the _fields param is not respected and all fields are returned.

This is because the rest_preload_api_request doesn't call rest_filter_response_fields on the response object. For other requests this call happens inside the rest_post_dispatch filter, but this filter is not called when preloading responses.

This patch adds the rest_post_dispatch filter call after every preloaded response. This filter also calls the rest_send_allow_header function to add the Allow header, so we can also remove an explicit call when handling an OPTIONS request.

Calling the rest_post_dispatch filter should be perfectly fine because it's already called in all similar circumstances:

  • when serving a normal REST request in WP_REST_Server::serve_request
  • when performing a batched request in WP_REST_Server::serve_batch_request_v1
  • when embedding links in WP_REST_Server::embed_links

One extra effect of this patch is that preloaded responses for GET requests will also include headers, while previously they would only include body, and headers would be present only for OPTIONS responses. Before:

{
  '/': {
    body: { home, title }
  },
  'OPTIONS': {
    '/': {
      body: { home, title },
      headers: { Allow }
    }
  }
}

After:

{
  '/': {
    body: { home, title },
    headers: { Allow }
  },
  'OPTIONS': {
    '/': {
      body: { home, title },
      headers: { Allow }
    }
  }
}

Including these headers is a good and desirable thing because it allows us to get rid of redundant OPTIONS request where we already have GET response. The GET response would now already contain the desired info about permissions. See also this discussion on a Gutenberg PR: ​https://github.com/WordPress/gutenberg/pull/38901#issuecomment-1044340843

In principle this patch is similar to what @TimothyBJacobs did some time ago for embeds: https://core.trac.wordpress.org/ticket/51722

Note: See TracTickets for help on using tickets.