#39696 closed enhancement (fixed)
REST API: Filter which links get embedded when passing the ?_embed query parameter
Reported by: | rheinardkorf | Owned by: | 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)
Change History (49)
This ticket was mentioned in Slack in #core-restapi by rheinardkorf. View the logs.
8 years ago
#3
@
8 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.
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.
#6
@
8 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
@
8 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:
↓ 9
↓ 12
@
8 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
@
8 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:
Cheers.
#10
follow-up:
↓ 13
@
8 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
@
8 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
@
8 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
@
8 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
@
8 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 :)
#15
follow-up:
↓ 16
@
8 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?
#16
in reply to:
↑ 15
;
follow-up:
↓ 17
@
8 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
@
8 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.
#18
@
8 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.
#19
@
8 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.
#20
follow-up:
↓ 21
@
8 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:
- This will not work for embedding objects into arrays such as a list of posts. See: https://github.com/WordPress/wordpress-develop/blob/4.7.1/src/wp-includes/rest-api/class-wp-rest-server.php#L430 - we need to come up with a way to pass the request object there too.
embed_links
is not called anywhere else in WP except fortest_link_embedding_without_links
, so I think we should go ahead and treat the$request
parameter as required. Sinceembed_links
is a protected method, the only way this could break is if a plugin is overridingWP_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
@
8 years ago
Replying to jnylen0:
- This will not work for embedding objects into arrays such as a list of posts. See: https://github.com/WordPress/wordpress-develop/blob/4.7.1/src/wp-includes/rest-api/class-wp-rest-server.php#L430 - we need to come up with a way to pass the request object there too.
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 fortest_link_embedding_without_links
, so I think we should go ahead and treat the$request
parameter as required. Sinceembed_links
is a protected method, the only way this could break is if a plugin is overridingWP_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.
8 years ago
#23
follow-up:
↓ 26
@
8 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.
8 years ago
#25
follow-up:
↓ 27
@
8 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.
#26
in reply to:
↑ 23
;
follow-up:
↓ 28
@
8 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
@
8 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
@
8 years ago
Replying to jnylen0:
So, we'll need to compute the value of this parameter (either
false
,true
, or an array) before callingresponse_to_data
, and then pass it down toembed_links
(with a default value oftrue
).
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:
↓ 30
@
8 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
@
8 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. You are not able to embed any other resources.
This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.
7 years ago
#33
@
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).
#35
@
5 years ago
- Milestone changed from Future Release to 5.4
Milestoning this for 5.4 to continue our performance improvements.
#36
@
5 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.
5 years ago
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#40
@
5 years ago
- Owner changed from jnylen0 to TimothyBlynJacobs
I uploaded a new patch that fixes an issue when doing collection embedding.
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.