Make WordPress Core

Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#39696 closed enhancement (fixed)

REST API: Filter which links get embedded when passing the ?_embed query parameter

Reported by: rheinardkorf's profile rheinardkorf Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests dev-feedback
Focuses: rest-api Cc:

Description

When performing a REST API request and adding the ?_embed query parameter the API attempts to embed all resources that is linked with embeddable set to true.

In some cases we may need to only embed some resources, not all embeddable resources. I am requesting that the feature be added to allow the filtering of a resources when specifying the _embed parameter.

Example:

http://<site>/wp-json/wp/v2/posts/48?_embed=author,wp:term

If implemented, the above request should only embed the Author and the Terms. (note that in this example : will be encoded to %3A)

Starting point:
This can be implemented on the WP_REST_Server class and is likely to effect the following methods:
response_to_data, embed_links

Attachments (6)

39696.diff (927 bytes) - added by rheinardkorf 7 years ago.
39696.1.diff (4.8 KB) - added by rheinardkorf 7 years ago.
39696.2.diff (6.5 KB) - added by rheinardkorf 7 years ago.
39696.3.diff (6.6 KB) - added by rheinardkorf 7 years ago.
39696.4.diff (7.0 KB) - added by TimothyBlynJacobs 4 years ago.
39696.5.diff (7.0 KB) - added by TimothyBlynJacobs 4 years ago.

Download all attachments as: .zip

Change History (49)

@rheinardkorf
7 years ago

#1 @rheinardkorf
7 years ago

I attached my first attempt to implement this ticket and wouldn't mind some feedback or testing.

I was looking to write a unit test for it as well, but couldn't find other unit tests that specifically addressed the _embed URI parameter.

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


7 years ago

#3 @adamsilverstein
7 years ago

  • Keywords 2nd-opinion needs-refresh needs-unit-tests added

Hi @rheinardkorf. Welcome to trac and thanks for your ticket and patch!

This seems like a nice enhancement. I tested the patch and verified it works as expected. However, I am concerned about using the _embed parameter to specify the fields. The API docs on using _embed mention that you can pass nothing or "1" to enable embed data, and with your patch "1" no longer works.

https://cl.ly/3t0Z3O3y0Q1R/Global_Parameters__REST_API_Handbook__WordPress_Developer_Resources_2017-01-27_14-57-41.jpg.

Can you try adding a new parameter to define the embed resources you want to be returned?

In regard to adding unit tests, you could probably add tests in tests/phpunit/tests/rest-api/rest-posts-controller.php verifying that the embed parameter and your filtering works as expected, take a look there and let me know if you have any questions.


#4 @adamsilverstein
7 years ago

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

#5 @jnylen0
7 years ago

#35613 was marked as a duplicate.

#6 @jnylen0
7 years ago

#35613 was an older ticket about this, but I closed it and left this one open because this one has a patch.

#7 @adamsilverstein
7 years ago

@jnylen0 what do you think about using _embed vs a new parameter? My concern is that some code or clients may already be sending "1".

#8 follow-ups: @jnylen0
7 years ago

I think we should stick with _embed and make sure all of these work, with appropriate test cases:

  • ?_embed: all embeds
  • ?_embed=1: all embeds
  • ?_embed=true: all embeds
  • ?_embed=author,wp:term: as expected
  • ?_embed[]=author&_embed[]=wp:term: as expected
  • JSON { _embed: true }: all embeds
  • JSON { _embed: 'author,wp:term' }: as expected
  • JSON { _embed: [ 'author', 'wp:term' ] }: as expected

And probably POST params as well, though most of that should just fall into place if the implementation is correct.

#9 in reply to: ↑ 8 @rheinardkorf
7 years ago

Thanks for the reply. That all makes perfect sense.

@jnylen0 , can you give me an example of how a JSON request is done so that I can make sure I capture the following:

  • JSON { _embed: true }: all embeds
  • JSON { _embed: 'author,wp:term' }: as expected
  • JSON { _embed: [ 'author', 'wp:term' ] }: as expected

Cheers.

Last edited 7 years ago by rheinardkorf (previous) (diff)

#10 follow-up: @rheinardkorf
7 years ago

@adamsilverstein I missed that notice in the docs. If compatibility is a concern, I am happy to explore using an alternate field.

#11 @jnylen0
7 years ago

If _embed is parsed as a standard parameter using the REST API parameter parsing logic, then JSON parameters should "just work".

However, looking through the current code I see that this is not the case. So I guess we can just stick with $_GET['_embed'] and not worry about JSON parsing for now.

I am strongly -1 on adding a new parameter here. We can pretty easily support both existing and new use cases with the same parameter - the ?_embed cases I outlined above should be sufficient.

#12 in reply to: ↑ 8 @adamsilverstein
7 years ago

Replying to jnylen0:

I think we should stick with _embed and make sure all of these work...

Great, thanks for the feedback.

#13 in reply to: ↑ 10 @adamsilverstein
7 years ago

Replying to rheinardkorf:

@adamsilverstein I missed that notice in the docs. If compatibility is a concern, I am happy to explore using an alternate field.

Actually, based on @jnylen0's feedback I agree _embed is fine, making sure it works as he described.

#14 @rheinardkorf
7 years ago

@jnylen0 I now have some code that will cater for the scenarios you described.

@adamsilverstein writing the unit tests to test _embed is proving to be difficult because of the use of $_GET. I don't see anywhere in the unit tests where any testing for _embed is done at all. As I started writing some unit tests I realised that there may actually be some logical issue with class-wp-rest-server.php itself. This would not be an issue if the $request object from ::serve_request() were to either be 1) set to an object property or 2) passed to ::response_to_data() (and subsequently passed to ::embed_links). The _embed variable can then be pulled from $request->get_query_params() and we can thus avoid using $_GET altogether.

Some direction here would be appreciated before I send my next patch :)

Last edited 7 years ago by rheinardkorf (previous) (diff)

@rheinardkorf
7 years ago

#15 follow-up: @rheinardkorf
7 years ago

Attached a new patch that includes the functionality as requested. Also included unit tests test_get_item__embed() that tests the scenarios.

Because of the issue mentioned earlier with the server relying on $_GET I've had to set $_GET within the tests. This is not ideal and needs to be addressed if the server gets updated (created #39727).

Could this patch please be reviewed?

Last edited 7 years ago by rheinardkorf (previous) (diff)

#16 in reply to: ↑ 15 ; follow-up: @jnylen0
7 years ago

Replying to rheinardkorf:

Because of the issue mentioned earlier with the server relying on $_GET I've had to set $_GET within the tests. This is not ideal and needs to be addressed if the server gets updated (created #39727).

I am not sure I agree with this approach. I have not tried it yet, but your earlier idea (option 2) seems to be a better solution:

This would not be an issue if the $request object from ::serve_request() were [...] passed to ::response_to_data() (and subsequently passed to ::embed_links).

In any case I'll take a closer look this coming week.

#17 in reply to: ↑ 16 @rheinardkorf
7 years ago

Replying to jnylen0:

In any case I'll take a closer look this coming week.

No problem. I'm happy to make whichever changes you require me to make. The patch for the server works, but obviously the unit test had to be hacky as there were no tests before and the use of $_GET probably has not been carefully considered. If you prefer I go with the option to pass the $request down the line to where its needed I am happy to do so... would make for a much cleaner unit test.

Looking forward to hearing back from you.

Last edited 7 years ago by rheinardkorf (previous) (diff)

#18 @jnylen0
7 years ago

If you prefer I go with the option to pass the $request down the line to where its needed

Yes I think that would be cleaner overall.

@rheinardkorf
7 years ago

#19 @rheinardkorf
7 years ago

@jnylen0 , @adamsilverstein I've added a patch now that includes the additional changes.

Unit tests now look a lot nicer and ::embed_links() and ::response_to_data() both now have an additional $request argument that is set to false by default to not break anything else.

Would be happy for this to be tested and feedback is of course most welcome!

I should add that this includes unit tests, so that flag on this ticket could possibly be removed.

Last edited 7 years ago by rheinardkorf (previous) (diff)

#20 follow-up: @jnylen0
7 years ago

  • Keywords has-patch has-unit-tests added; 2nd-opinion needs-unit-tests removed

Thanks @rheinardkorf! This is looking better.

A few notes on 39696.2.diff:

  • embed_links is not called anywhere else in WP except for test_link_embedding_without_links, so I think we should go ahead and treat the $request parameter as required. Since embed_links is a protected method, the only way this could break is if a plugin is overriding WP_REST_Server. I am honestly not sure if we should worry about this or not - maybe we can just add a check into the code like this: $embed = ( empty( $request ) ? false : $request->get_param( '_embed' ) ).
  • We might as well go ahead and use $request->get_param or $request['_embed'] - this should give us JSON and POST support for free.

#21 in reply to: ↑ 20 @rheinardkorf
7 years ago

Replying to jnylen0:

Good spot! There are 2 possible ways to address this. 1) Create a custom mapping object in which we can pass the request object. 2) Drop array_map and do a traditional foreach. For the sake of this problem I'm inclined to go the latter.

  • embed_links is not called anywhere else in WP except for test_link_embedding_without_links, so I think we should go ahead and treat the $request parameter as required. Since embed_links is a protected method, the only way this could break is if a plugin is overriding WP_REST_Server. I am honestly not sure if we should worry about this or not - maybe we can just add a check into the code like this: $embed = ( empty( $request ) ? false : $request->get_param( '_embed' ) ).

Which method would you like to implement the check on, ::embed_links() or ::response_to_data() ?

  • We might as well go ahead and use $request->get_param or $request['_embed'] - this should give us JSON and POST support for free.

Sorry I do not follow this last one. Where would you like me to do this?

Thanks for the feedback. If you could answer those questions for me I could work on another patch a bit later. Got some client work first today.

Cheers.

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


7 years ago

@rheinardkorf
7 years ago

#23 follow-up: @rheinardkorf
7 years ago

@jnylen0 , I believe I've now addressed your feedback from the previous patch. Let me know if there is anything else you'd like me to do here.

Cheers.

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


7 years ago

#25 follow-up: @westonruter
7 years ago

On a related note, it would be great if in addition to indicating a embed subsetting if we could use a similar construct to indicate that resources should be embedded recursively along a specified tree. For example, if I wanted to get the recent posts along with their comments embedded and the embedded comments' own authors also embedded a third level down, maybe I could do so with a request like:

/wp/v2/posts?_embed[]=comments/author

I understand this is maybe encroaching on GraphQL territory, but just wanted to mention recursive embedding as the flip side of being able to limit embedding. This is also encroaching on batch requests as well I suppose.

Last edited 7 years ago by westonruter (previous) (diff)

#26 in reply to: ↑ 23 ; follow-up: @jnylen0
7 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner changed from adamsilverstein to jnylen0
  • Version changed from 4.7.1 to 4.4

Replying to rheinardkorf:

I believe I've now addressed your feedback from the previous patch. Let me know if there is anything else you'd like me to do here.

After looking at this again, I think we can improve the approach further. Instead of adding a new $request parameter, let's change the $embed parameter to response_to_data as follows:

- @param bool $embed Whether links should be embedded.
+ @param bool|array $embed Which links to embed (none, all, or a specific list).

So, we'll need to compute the value of this parameter (either false, true, or an array) before calling response_to_data, and then pass it down to embed_links (with a default value of true).

This should be a cleaner and more backwards-compatible way of making the change here.

We can also make some further improvements to the test cases:

  • Split up into multiple test methods
  • Set parameters using $request->set_query_param, $request->set_json_param etc., for all cases I listed in comment:8
  • Potentially use a PHPUnit data provider to get rid of boilerplate in the tests

I'm also happy to handle any of these further improvements later.

Finally, you had asked about getting this change into 4.7.3. For this release we are targeting fixes with potential backwards-compatibility implications, as these need to be made sooner rather than later. Since this is a pretty clean enhancement, and it's not quite ready yet, I think we can hold off until 4.8 or a potential 4.7.4.

If you need this functionality in the meantime, then I would recommend creating a plugin. Here is a potential starting point for such a plugin.

#27 in reply to: ↑ 25 @jnylen0
7 years ago

Replying to westonruter:

On a related note, it would be great if in addition to indicating a embed subsetting if we could use a similar construct to indicate that resources should be embedded recursively along a specified tree. For example, if I wanted to get the recent posts along with their comments embedded and the embedded comments' own authors also embedded a third level down, maybe I could do so with a request like:

/wp/v2/posts?_embed[]=comments/author

I understand this is maybe encroaching on GraphQL territory, but just wanted to mention recursive embedding as the flip side of being able to limit embedding. This is also encroaching on batch requests as well I suppose.

I'm not totally sold on this, like you said this sort of thing feels like a better fit for a full-fledged query language, rather than a stopgap measure for one specific case.

Another potential drawback of this approach is if there is a way to embed a very large number of objects in a single request. We would need to limit this somehow, otherwise we would be providing a convenient way to take up a bunch of server resources with a single request.

#28 in reply to: ↑ 26 @rheinardkorf
7 years ago

Replying to jnylen0:

So, we'll need to compute the value of this parameter (either false, true, or an array) before calling response_to_data, and then pass it down to embed_links (with a default value of true).

That sounds like a reasonable approach.

We can also make some further improvements to the test cases:
...
I'm also happy to handle any of these further improvements later.

I'm happy with later too.

Finally, you had asked about getting this change into 4.7.3. For this release we are targeting fixes with potential backwards-compatibility implications, as these need to be made sooner rather than later. Since this is a pretty clean enhancement, and it's not quite ready yet, I think we can hold off until 4.8 or a potential 4.7.4.

If you need this functionality in the meantime, then I would recommend creating a plugin. Here is a potential starting point for such a plugin.

That is a great help. I might work on it via a plugin for now and that is a good starting point thanks.

I'm going to keep this ticket in my to do pile, but seeing that we're only talking a potential 4.8 I think I might focus on a plugin to add this functionality.

Thanks @jnylen0, thats been a great help.

#29 follow-up: @rmccue
7 years ago

One other use case we need to support: ?_embed without a value. Agree with @jnylen0 on the other points here generally speaking.

Also worth noting: the value of this parameter needs to be a filter on things that are embeddable (i.e. an intersection of embeddable and the _embed value), and we need to be careful not to try and embed things that don't have the embeddable flag set to true.

#30 in reply to: ↑ 29 @rheinardkorf
7 years ago

Replying to rmccue:

One other use case we need to support: ?_embed without a value.

Current patch already supports that too.

Also worth noting: the value of this parameter needs to be a filter on things that are embeddable (i.e. an intersection of embeddable and the _embed value), and we need to be careful not to try and embed things that don't have the embeddable flag set to true.

The patch as it is will only filter "out" that which is already marked as embeddable and not supplied in the query. You are not able to embed any other resources.

Last edited 7 years ago by rheinardkorf (previous) (diff)

#31 @rmccue
7 years ago

Great, was mainly noting for posterity's sake. :)

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


7 years ago

#33 @greatislander
7 years ago

As per @jnylen0 in Slack, I believe this patch would also ensure that requests routed through rest_do_request() respect the _embed parameter if set (currently they don't).

#34 @TimothyBlynJacobs
6 years ago

#43956 was marked as a duplicate.

#35 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Future Release to 5.4

Milestoning this for 5.4 to continue our performance improvements.

#36 @TimothyBlynJacobs
4 years ago

  • Keywords dev-feedback added; needs-refresh removed

Uploaded a refresh patch mostly following @jnylen0's latest outline of functionality.

Currently, the patch continues to check for the $_GET variable instead of pulling it from the WP_REST_Request object. I think that is a bit safer back-compat wise.

I'm not strongly opposed to changing it to pull from the WP_REST_Request object if we think the impact will be low enough and it provides a meaningful benefit. But right now, even if we pulled from the request object, embeds still wouldn't work for internal requests.

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


4 years ago

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


4 years ago

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


4 years ago

#40 @TimothyBlynJacobs
4 years ago

  • Owner changed from jnylen0 to TimothyBlynJacobs

I uploaded a new patch that fixes an issue when doing collection embedding.

This ticket was mentioned in PR #149 on WordPress/wordpress-develop by TimothyBJacobs.


4 years ago
#41

#42 @TimothyBlynJacobs
4 years ago

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

In 47224:

REST API: Introduce selective link embedding.

Previously the _embed flag would embed all embeddable links in a response even if only a subset of the links were necessary. Now, a list of link relations can be passed in the _embed parameter to restrict the list of embedded objects.

Props rheinardkorf, adamsilverstein, jnylen0, cklosows, chrisvanpatten, TimothyBlynJacobs.
Fixes #39696.

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


3 years ago

Note: See TracTickets for help on using tickets.