Conversation
|
Size Change: 0 B Total Size: 7.92 MB |
c0aadcb to
f99043d
Compare
|
Flaky tests detected in d01832e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/33146216658 Should save the changes in
|
Notes on the path to WordPress CoreThe implementation here is hook-driven, but the sequence maps to Core in the following way:
Because the plugin records lineage on every edit that goes through Two editing paradigms: editor vs. media libraryIt's worth noting the two image-editing models.
The two paradigm work in their own contexts, and shouldn't conflict:
|
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
40b51f8 to
75c1d2e
Compare
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
75c1d2e to
d01832e
Compare
| return $response; | ||
| } | ||
|
|
||
| $data['media_details']['original_attachment'] = array( |
There was a problem hiding this comment.
See this comment over the in core patch: https://github.com/WordPress/wordpress-develop/pull/13303/changes#r3878648433
I don't believe there's a precedent for media_details with registered context.
The original_attachment sub-property has context: edit.
media_details is usually treated as a bare, typeless object with no properties at all.
The alternative is a top-level property.
There was a problem hiding this comment.
Hrm, this is an interesting one! What's the trade-off between placing this value in media_details versus making it a root-level property in the output, i.e. so that it's effectively a field just like alt_text, post or source_url? Would it simplify things if were a separate field, so that we don't bump into the other data in media_details?
I'm always wondering if we'll want to be able to query by "give my all the derivates / crops for attachment id 10" which might be easier if it's a field 🤔
There was a problem hiding this comment.
What's the trade-off between placing this value in media_details versus making it a root-level property in the output, i.e. so that it's effectively a field just like alt_text, post or source_url?
You know what, the only reason I chucked it in media_details was because parent_image was already in the post meta and I reasoned the two belonged together 😄 But "post" is top level in the REST response.
Also media_details in the REST response, from what i can gather, is filled with props that describe the image, e.g., width/height.
Our original_attachment describes relationship to a different post.
I don't see any cost to moving it up. Let's do it. Thanks for raising it.
I'm always wondering if we'll want to be able to query by "give my all the derivates / crops for attachment id 10" which might be easier if it's a field 🤔
Maybe the query would be easier to build, if we make it queryable by original_attachment, that is, a new collection param on /wp/v2/media but it'd still have to translate to a meta_query on _wp_attachment_original_id. Would be a good follow up in any way.
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the update, I like it at the root level 👍
Just left a small comment about the shape of the data: could it be a simple integer, or do we need source_url embedded?
Another question re: naming is, in terms of REST API shape, would original_media feel more consistent with the endpoint? I.e. since the endpoint is called media and we have media_type rather than attachment_type and so on 🤔
| $data['original_attachment'] = array( | ||
| 'attachment_id' => $original_id, | ||
| 'source_url' => $source_url, | ||
| ); | ||
| $response->set_data( $data ); |
There was a problem hiding this comment.
Do we need both the attachment_id and the source_url? I.e could this be a simple integer field and it's up to the client to request the other attachment if they want to?
One other benefit if we register this as a field, is that I think we might be able to use links / embed the linked item in the response if consumers wish to.
There was a problem hiding this comment.
Do we need both the attachment_id and the source_url?
Yes and no. 😆
So it's terrifically convenient for loading the image into the editor. See: https://github.com/WordPress/gutenberg/pull/81805/changes#diff-6742eddff5146f6b37df0497c25f09e886dfa83a40d41e91e0d3d217cf806e0dR120
But yeah, the alternative is to enable embed like you say. It's a great idea, and I guess it would be consistent with the parent post id?
What do you think?
There was a problem hiding this comment.
What do you think?
So it's terrifically convenient for loading the image into the editor. See:
Ah, I see! Yes for that use case it's very convenient. In principle, though, I think the consumer probably should be doing a little more work there. Either via an embedded response in the original request, or a subsequent request to grab the full attachment via a second API call either before or after someone clicks the restore button.
My thinking is this: if we're making changes to the REST API to accommodate the feature, it's worth it to build it out as a field and how we think it should work in the long run. That leans me toward a simple integer value because eventually we might want to query by it, and it seems neater if its an integer in all cases (as a query param and in the response object).
All that said, we don't need to build the full field right now, we just need the data to be in a shape we want to commit to. And that points me toward a simple integer value, just as we have for post, as it seems the most consistent with how we shape the API endpoints.
A caveat, though: this is not a strongly held opinion, just my thoughts as I think about the change here. I can be easily swayed 😄
There was a problem hiding this comment.
Oh, edit to add: one of the reasons this is on my mind is that once you've got your button feature in, I'd love to have a play with the idea of a dropdown that shows all available existing crops, so that's why the query idea resonates for me!
I.e. you open the cropper and it requests to see if a parent crop exists, or if the current image is a parent of other crops and it lists them all so you can navigate between them.
There was a problem hiding this comment.
My thinking is this: if we're making changes to the REST API to accommodate the feature, it's worth it to build it out as a field and how we think it should work in the long run. That leans me toward a simple integer value because eventually we might want to query by it, and it seems neater if its an integer in all cases (as a query param and in the response object).
I'm sold. I'm going to do it! Thank you!
I'd love to have a play with the idea of a dropdown that shows all available existing crops, so that's why the query idea resonates for me!
💯 Great idea
Nothing against changing it. You're right it definitely sounds more consistent in terms of the route, but I made it So we have
|
Ah good point! I'd been thinking of existing keys used elsewhere like For a tiny bit of extra context: the reason I'm nitpicking the naming here is that I wound up having to rename the attached-to field and relation a bit when trying to get that change into the REST API last year in core, so I imagine these sorts of questions will get asked at the core merge stage. But I think you've argued well for |
Worth thinking about it now. Thanks for raising it. As said, happy to change it whatever makes sense. I think the more folks we ask, the more opinions we'll get 😄 Core might be a ripe place to poke at it. Cheers! |
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
The 8.0.0 release moved trunk's Unreleased section to a published heading, and the rebase merged the #81803 entry into it. The PR is unmerged, so its entry belongs under Unreleased.
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
The 8.0.0 release moved trunk's Unreleased section to a published heading, and the rebase merged the #81803 entry into it. The PR is unmerged, so its entry belongs under Unreleased.
7e969ea to
f7a61fc
Compare
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
Add a "Restore original image" button to the Crop sidebar tab. It appears when the edited attachment exposes media_details.original_attachment (the lineage root from the backend primitive). Clicking loads the original into the cropper as a dirty preview: - Save with no further crop repoints the block at the original attachment — no /edit request, no new attachment. - Save after cropping the restored original runs /edit against the original's id, creating a fresh child off the root. - Cancel discards the restore via the existing discard-confirm. The canvas takes an optional srcOverride so the cropper can show the original; swapping the source resets the cropper's dirty baseline, so a bare restore is tracked by a separate isOriginalRestored flag OR'd into hasChanges. The original's natural dimensions come from a cached getEntityRecord on its id (the field itself carries only id + url). Based on #81803 (the original_attachment lineage primitive).
|
This is still in scope for 7.2. Backport here: |
When an image is edited via /wp/v2/media/{id}/edit, core creates a new
child attachment with no stable pointer back to the image the lineage
started from. Record that root id in a dedicated postmeta key
(_wp_attachment_original_id) at edit time and expose it in the REST
response as media_details.original_attachment (edit context only).
- gutenberg_get_original_attachment_id() resolves the root, or returns
the attachment's own id when it has no edit lineage.
- The wp_edited_image_metadata hook records the root once at write time,
so reads are a single meta lookup with no chain walking.
- delete_attachment clears the pointer from descendants via an indexed
(meta_key, meta_value) query.
Forward-only: tied to the unreleased media editor cropper, so no
backfill for attachments edited before this lands.
Part of #78077.
…ype for edited images
Root level rather than inside media_details because it describes a relationship between attachments (like post, the parent post), not a property of the media file.
Mirror featured_media: the field is the original attachment's id, with
an embeddable wp:original-attachment link for hydrating the full
record via _embed, instead of an { attachment_id, source_url } object.
rest_prepare_attachment fires twice per attachment (once from the posts controller, again from the attachments controller wrapping it), so add_link() appended a duplicate wp:original-attachment link; only add it when absent. Drop the wp_get_attachment_url() reachability check: it cost an uncached meta query per derivative, delete_attachment cleanup keeps the meta accurate, and a consumer fetching a dangling id simply gets no record.
The feature targets the WordPress 7.2 release (wordpress-develop PR 13303), so it belongs in the compat directory rather than lib/experimental. Move the test to the phpunit root alongside the other compat feature tests.
The field now carries a schema (integer, edit context, readonly), so it appears in the OPTIONS /wp/v2/media response, works with _fields filtering, and core strips it outside the edit context automatically. A registered field is always present in its contexts, so attachments with no lineage now return 0 (matching featured_media) instead of omitting the key. The rest_prepare_attachment filter remains only to add the embeddable wp:original-attachment link, which register_rest_field cannot do.
The 8.0.0 release moved trunk's Unreleased section to a published heading, and the rebase merged the #81803 entry into it. The PR is unmerged, so its entry belongs under Unreleased.
Rename the lineage helpers, postmeta key, REST field and link relation so they no longer read like core's original_image / wp_get_original_image_* APIs, which describe the unscaled upload of the same attachment: - gutenberg_get_original_attachment_id() -> gutenberg_get_edit_root_attachment_id() - gutenberg_record_original_attachment_id() -> gutenberg_record_edit_root_attachment_id() - _wp_attachment_original_id -> _wp_attachment_edit_root_id - REST field original_attachment -> edit_root - Link relation wp:original-attachment -> wp:edit-root The remaining helpers, the meta key constant, the compat and test file names and the test class follow suit. No behaviour change.
360784e to
c803dfb
Compare
Adds a Restore original image action to the media editor's More options menu, shown when the attachment exposes the edit_root field recorded by its lineage (#81803). - Restore loads the root attachment and its editable details, and discards pending edits to the derivative. - Saving an untouched restore repoints the block without writing an attachment; transforming first runs /edit against the root. - Metadata fields are disabled while a restore is pending; detail saves retarget the root. - Image and Cover restore the previous attachment with snackbar Undo. Squashed end state of the restore-original branch, ported onto the session cropper engine after the snapshot-history refactors landed on trunk.
| $rel = 'https://api.w.org/edit-root'; | ||
| $links = $response->get_links(); | ||
| if ( ! isset( $links[ $rel ] ) ) { | ||
| $response->add_link( |
There was a problem hiding this comment.
Just a very minor nitpicky question as I re-test this: do we need to handle skipping adding the link for requests that specify specific fields they want in the response (i.e. limiting the response)?
Here's a couple of examples from core, in case it helps!
The Posts controller: https://github.com/WordPress/wordpress-develop/blob/5aac49a2d6bf28944a3c9ed5827a622e02696649/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2150-L2152
This might be an example we could copy from the REST server class: https://github.com/WordPress/wordpress-develop/blob/5aac49a2d6bf28944a3c9ed5827a622e02696649/src/wp-includes/rest-api/class-wp-rest-server.php#L1404-L1414
(And as always, apologies if I'm overthinking this one!)
There was a problem hiding this comment.
Oh yes, very probably. Thanks for flagging that. I'll take a look.
Makes sense that we'd only want it when folks explicitly call for embed or the links field.
Core builds its own links only when a field-limited request includes _links or _embedded, so adding ours unconditionally handed a _fields-limited response a _links member core intended to omit. Bail before add_link() using the same rest_is_field_included() idiom as core, treating a present _embed parameter as a request for links.
Adds a Restore original image action to the media editor's More options menu, shown when the attachment exposes the edit_root field recorded by its lineage (#81803). - Restore loads the root attachment and its editable details, and discards pending edits to the derivative. - Saving an untouched restore repoints the block without writing an attachment; transforming first runs /edit against the root. - Metadata fields are disabled while a restore is pending; detail saves retarget the root. - Image and Cover restore the previous attachment with snackbar Undo. Squashed end state of the restore-original branch, ported onto the session cropper engine after the snapshot-history refactors landed on trunk.
A response limited with `_fields` carries no `_links` member unless the request asks for `_links` or `_embedded`, because the posts controller builds no links at all in that case. The edit root link was added outside that check and keyed off the response field, so `_fields=id,edit_root` came back with a `_links` member holding this one link alone, and `_fields=id,_links` came back without it while every other link was there. The link is now gated on the same check the parent controller uses for its own links, and reads the edit root itself rather than the prepared field, so asking for links no longer depends on asking for the field. Follows WordPress/gutenberg#81803. The plugin also treats a bare `_embed` parameter as a request for links. Core does not: `get_fields_for_response()` drops `_embedded` from a `_fields` list that omits it, and core's own links stay out of that response too. See #65987.
andrewserong
left a comment
There was a problem hiding this comment.
Re-testing and this is working nicely for me!
✅ edit_root key is correctly populated with the root attachment id
✅ links appear when expected (and the relevant url is correct)
✅ On an attachment that has no edit_root its value is 0
✅ Confirmed that deleting an attachment also clears out the post meta in the db
I reckon this is a good place to get it in, we've had a few rounds of discussing naming and I think we've landed on a decent structure here and we haven't changed our minds over the past few days 😄
LGTM!
Tiny nit from another round with Claude (non-blocking): Should we guard registration behind a check to ensure we don't run all this twice if core's backport is present?
Oh boy, excellent point. I'll do that. E.g., |
Core's backport (wordpress-develop PR 13303) defines wp_get_edit_root_attachment_id() in post.php, loaded before any plugin, so its presence means core already records the meta, exposes the field and link, and cleans up on delete. Consolidate the four registrations under that check. The functions stay defined either way, since the test suite calls them directly.
Adds a Restore original image action to the media editor's More options menu, shown when the attachment exposes the edit_root field recorded by its lineage (#81803). - Restore loads the root attachment and its editable details, and discards pending edits to the derivative. - Saving an untouched restore repoints the block without writing an attachment; transforming first runs /edit against the root. - Metadata fields are disabled while a restore is pending; detail saves retarget the root. - Image and Cover restore the previous attachment with snackbar Undo. Squashed end state of the restore-original branch, ported onto the session cropper engine after the snapshot-history refactors landed on trunk.
Match core's guard on the featured_media link: only advertise the wp:edit-root link when the root attachment is published or the requester can read it. A stale meta id (the root deleted out of band, or the id recycled) resolves to do_not_allow and the link is skipped; the edit_root field itself still carries the stored id, since the link is a promise that ?_embed can hydrate it and the field is not.
The `wp:edit-root` link promises that `_embed` can fetch the edit root, but the recorded ID is not checked, so an edit root that no longer exists, or one the user cannot read, was still offered as a link. The link now uses the same check as the `featured_media` link: it is added only when the edit root is published or the user can read it. The `edit_root` field still reports the recorded ID as is. Follows WordPress/gutenberg#81803. See #65987.
What?
Adds lineage tracking for attachments created by editing an image via
/wp/v2/media/{id}/edit. Records the root/original attachment id on each derivative and exposes it in the REST response.Part of #78077. Supersedes #78458.
Why?
When Gutenberg crops an image, Core's
/editcreates an entirely new attachment with no stable pointer back to the image the lineage started from.parent_imagerecords only the immediate source; there is no root/original reference across crop-of-crop chains.That means we can't navigate a crop back to its original in a performant way, that is, without getting each post up the change where
parent_imageexists.How?
_wp_attachment_edit_root_idstores the root id, written once at edit time on thewp_edited_image_metadatahook. The new child inherits its parent's root, or the parent itself when the parent has no lineage, so reads are a single meta lookup, no chain walking.gutenberg_get_edit_root_attachment_id( $id )resolves the root, or returns$idwhen there's no lineagerest_prepare_attachmentfilter adds a root-leveledit_rootfield holding the original's id, with an embeddablewp:edit-rootlink (mirrorsfeatured_media),editcontext only, and omits it when the original is unreachable (deleted/missing file). Root level because it describes a relationship between attachments, likepost.delete_attachmentclears the pointer from descendants via ameta_key-indexed queryScope / forward-only
Applies to every edit that goes through
/edit— including the image block's crop tools in the post editor — on any site running the plugin; the media editor modal is the first consumer of the read side. No backfill: attachments edited before this lands are intentionally untracked.Test plan
No manual tests here — to test in the browser, use the follow-up UI PR: #81805
Summary by CodeRabbit
New Features
Documentation
Tests