-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Attachments/media endpoint: Add an embeddable post as a link in attachments REST API response #10027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
@rmccue and @spacedmonkey just adding you as reviewers for this one as I know you've both done a lot of REST API work in the past. I've had a bit of a break from contributing directly to core this year, so apologies for the ping if you're not the right folks to ping on these sorts of PRs (and of course let me know if there's anyone else I should request a review from). Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Thanks for the tests. I had a question about the post type some folks might be able to verify.
if ( ! empty( $post->post_parent ) ) { | ||
$links['post'] = array( | ||
'href' => rest_url( rest_get_route_for_post( $post->post_parent ) ), | ||
'embeddable' => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at other controllers, embeddable links often include additional attributes. Example:
https://github.com/ramonjd/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php#L1193Looking at other controllers, embeddable links often include additional attributes
Would it be useful to know the post type before embedding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! That could also be useful for requests that aren't embedding, but that would like to fetch the linked resource in a separate request, as they'd know the post type 👍
I'm curious for feedback on this, as we'd need to add a call to get_post
to grab the post type, but overall I like the idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like to fetch the linked resource in a separate request, as they'd know the post type
A use case could be interacting with the wordpress/core-data
package, or directly linking to an editor instance, e.g., /wp-admin/site-editor.php?p=/page/3&canvas=edit
.
Access to the post type and id is required.
Having said that, it's something that can be added later so unless there's a perceived and medium-term need for it, I think it's okay to leave out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good points, I'm happy to update it, I think it'll make the link far more useful to include the post type 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php
Outdated
Show resolved
Hide resolved
…achments REST API response
…ontroller.php Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
cb08d80
to
770e64e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM

This is a small change and (in my opinion) non-controversial.
It makes sense to be able to embed an attachment's attached post.
There is talk about allowing multiple parents for attachments, e.g.,
They are 12+ years old though, so I think any changes to many-to-many relationships for these post types can be dealt with if it ever gets in.
Thanks for the review! I'll just ping another couple of folks for visibility in case anyone else has opinions on the shape of this change. FYI: @swissspidy and @adamsilverstein in case this touches upon any media stuff y'all are working on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look fine to me, thanks @andrewserong!
Thanks everyone! I'll look to commit this today |
In the attachments controller (i.e. for the
media
endpoint) addpost
to the links in the API response, that provides a link to the API call to get the linked post — i.e. the parent, or the post that the media item is attached to.The purpose is to make it easier to fetch linked posts or pages that media items are attached to, via the REST API. This is part of some of the work I've been exploring for the new media library (proposed in WordPress/gutenberg#55238). When listing (or providing UI to make changes to) the "uploaded to" field (or "attached to" or whatever it should be called), it'll be easier to do so if we can embed the post in the API response for media items.
Trac ticket: https://core.trac.wordpress.org/ticket/64034
Testing instructions
If you'd like to test this from within the block editor:
undefined
as the request hasn't been resolved yet):post
in the list of_links
.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.