Repository navigation
Conversation
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. |
f271a85 to
b6dff27
Compare
| * is indexed, so this only scans the rows for attachments created by editing an image, | ||
| * and it avoids searching the serialized attachment metadata for the ID. | ||
| */ | ||
| delete_metadata( 'post', 0, '_wp_attachment_original_id', $post_id, true ); |
There was a problem hiding this comment.
true is for $delete_all
We want this because, when the original is deleted we want to clear all descendants.
See: https://developer.wordpress.org/reference/functions/delete_metadata/
There was a problem hiding this comment.
A note on deletion paths:
wp_delete_post()delegates towp_delete_attachment()for attachments, so every Core route should reachdelete_attachmentand clear the record.- From what I've traced it has the same "reachability" as Core's
_thumbnail_idcleanup, which uses the identicaldelete_metadata() - doesn't fire on trash, which I think is the current pattern (the attachment still exists )
Plugins could still short circuit deletion and do it themselves via any filter, e.g., pre_delete_attachment. That means the clean up might not happen. This isn't Core's to fix, but we should mention this in the dev note for 7.2.
| * @return int ID of the attachment the chain started from, or `$attachment_id` when the | ||
| * attachment was not created by editing another one. | ||
| */ | ||
| function wp_get_original_attachment_id( $attachment_id ) { |
There was a problem hiding this comment.
wp_get_original_image_url() and wp_get_original_image_path() return the unscaled upload of the same attachment.
wp_get_original_attachment_id() returns the first image in an edit chain of different attachments.
On an edited image those near-identical names answer different questions. What about
wp_get_root_image_id()wp_get_source_attachment_id()
Also does this need a filter?
2c60221 to
65af124
Compare
|
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. |
Editing an image via the `wp/v2/media/<id>/edit` REST endpoint saves the result as a new attachment and leaves the edited image untouched, so a site can build up a chain: an upload, a crop of it, a crop of that crop. `parent_image` records only the immediately preceding image, so finding the image a chain started from meant walking it one attachment at a time. Each attachment created by an edit now records the ID at the top of its chain in `_wp_attachment_original_id` postmeta, inheriting it from the image being edited. `wp_get_original_attachment_id()` reads it back in a single lookup, and returns the ID it was given for attachments that were uploaded rather than edited. The attachments REST controller exposes the result as an `original_attachment` field in the `edit` context only, giving editors what they need to offer a way back to the original without telling visitors which images were made from which. Deleting an attachment clears the record from any image edited from it, so nothing is left pointing at an ID that could later be reused. Records are written going forward only; images edited before this lands are not backfilled. Fixes #65987.
The field points at another attachment, and every other pointer to another entity in a media response is top level: `author`, `post`, `featured_media`. `media_details` holds the width, height, file, size and derived sizes of one image, and no references to anything else. Registering it properly also means it can be requested on its own with `_fields`, which was not possible while it was nested inside another object. Follow-up to the original commit on this branch. See #65987.
Adds tests for three cases the existing coverage left undefined. Trashing an original does not clear the record on images edited from it: `delete_attachment` only fires on permanent deletion, and keeping the record means untrashing restores the relationship intact. Editing an image whose original has been deleted starts a new chain from the image being edited, since there is no lineage left to inherit. Deleting an image from the middle of a chain leaves the images below it pointing at the start of the chain, because each one records where the chain started rather than the image directly above it. See #65987.
The field carried an `attachment_id` and `source_url` pair. Relationships in this API are bare IDs — `post`, `parent`, `featured_media` — so it is now just the ID of the original attachment. Clients that need the original's URL or dimensions get them from a `wp:original-attachment` link, which is embeddable in the same way as a featured image: `?_embed` hydrates the whole attachment record under `_embedded`. The link is added where the request is still in scope rather than in `prepare_links()`, which cannot see it, so the link stays in the `edit` context alongside the field. See #65987.
The field was left out entirely for an image that was not created by editing another one. `featured_media` reports `0` for "no featured image" rather than disappearing, so this now does the same, and clients get a field of one type that is always there in the `edit` context. The stored ID is no longer checked against the original's file before being sent. Deleting an attachment already clears the ID from everything edited from it, so the check only affected originals sitting in the trash, whose files still resolve. A client following an ID that has gone stale gets no record back, which it must handle in any case. See #65987.
The field comment said `original_attachment` is limited to the `edit` context so visitors cannot tell which images were made from which. `media_details` already exposes `parent_image` in the `view` and `embed` contexts, so that was not the reason. It is limited to `edit` because only editors need it. The schema description and the `@return` of `wp_get_original_attachment_id()` said a missing original meant the image was not created by editing another one. Images edited before this change have no record either, so both now say none is recorded. See #65987.
`wp_get_original_attachment_id()` sat next to `wp_get_original_image_path()` and `wp_get_original_image_url()`, which describe the unscaled upload of the same attachment rather than the attachment a chain of edits started from. The names now say which one they mean: - `wp_get_original_attachment_id()` -> `wp_get_edit_root_attachment_id()` - `_wp_delete_original_attachment_id()` -> `_wp_delete_edit_root_attachment_id()` - `_wp_attachment_original_id` postmeta -> `_wp_attachment_edit_root_id` - `original_attachment` REST field -> `edit_root` - `wp:original-attachment` link relation -> `wp:edit-root` No change in behaviour. See #65987.
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.
fdbe530 to
ea3620e
Compare
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing nicely for me! As discussed on the related Gutenberg PR I think this settles on a good naming structure for this feature, and I think post meta is the right place to store it as it'll enable useful features further down the track, like a UI for navigating sibling crops, etc.
One other question I had, and I don't think it's a blocker for this PR but would be good to consider, is what should happen for importers / WXR exports.
For example, I see over in the importers repo there was a PR that skips some post meta here:
I have no experience with the importers code, but one idea after this PR lands could be to update the importers to skip _wp_attachment_edit_root_id just as it skips _wp_attachment_metadata. The result would be that the lineage would be severed on WXR exports, so it would be lossy, but maybe not terrible? In any case, I think the export/import behaviour here is likely beyond the scope of this PR.
If we did want to deal with it in some way now, one idea could be to skip the post meta in the export:
wordpress-develop/src/wp-admin/includes/export.php
Lines 483 to 498 in 814279b
However, since the importer repo already handles other attachment post meta, my hunch is that the fix/handling will be better done over in the wordpress-importer repo sometime before the 7.2 release.
What do you think? No other blockers here IMO!
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.
Thanks for flagging this! I had a bit of a dig around and found something else that looks interesting:
So maybe a follow up to the simpler, skipping could be to remap in a similar way. I think your idea of skipping is probably the best, first version: it'll avoid bad relationships after import, e.g., if an attachment with the same id already exists |
Draft PR here when/if we need it: |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few notes inline but nothing major.
| * @ticket 65987 | ||
| */ | ||
| public function test_get_edit_root_attachment_id_returns_same_id_for_an_upload() { | ||
| $attachment = self::factory()->attachment->create_upload_object( self::$test_file ); |
There was a problem hiding this comment.
Rather than creating an attachment for each test, it will be quicker to create a shared fixture in wpSetUpBeforeClass().
There was a problem hiding this comment.
Ugh, so I tried this: I created the attachment once in wpSetUpBeforeClass() and shared it
across the edit-root tests, but a bunch of tests failed.
tear_down() calls remove_added_uploads(), which wipes the uploads directory between tests. I don't want to bloat this PR for several milliseconds. Maybe a follow up... not sure what it'd be.
Restore the file in set_up() when it's missing? Sounds like more hassle.
There was a problem hiding this comment.
@ramonjd Thanks, I'll keep that in mind for later reviews. Keeping this PR as is works for me, it seems like a potential test suite bug which is well outside the scope of this issue.
| * is indexed, so this only scans the rows for attachments created by editing an image, | ||
| * and it avoids searching the serialized attachment metadata for the ID. | ||
| */ | ||
| delete_metadata( 'post', 0, '_wp_attachment_edit_root_id', $post_id, true ); |
Every assertion in a test that makes more than one now says what it checks, so a failure names the behaviour that broke rather than only the line it broke on. See #65987.
`wp_get_edit_root_attachment_id()` returned the ID it was given when an attachment had no recorded edit root, so a caller had to compare the result against the ID it passed in to tell "has an edit root" from "is its own edit root". It now returns 0, which is what `get_post_thumbnail_id()` and `wp_get_post_parent_id()` do for nothing, and what the REST field already reported. A record pointing at the attachment itself resolves to 0 too, so that broken record is handled where the lookup happens rather than by each caller. `edit_media_item()` relied on the old return value to seed the first edit in a chain and now falls back to the edited attachment explicitly. The REST field is the function's result as it stands, and the link is offered when that result is non-zero. See #65987.
The block editor's image editor creates an entirely new attachment with no stable pointer back to the image the lineage started from. This change records the attachment an edit chain started from in `_wp_attachment_edit_root_id` postmeta, reads it back with `wp_get_edit_root_attachment_id()`, and exposes it as a top-level `edit_root` field on the attachment REST response: * the ID of the edit root, or, * `0` when the image was not created by editing another one. The field is included in the REST API's `edit` context only, alongside an embeddable `wp:edit-root` link so clients can hydrate the edit root with `_embed`. The link is only added when the edit root is published or the user can read it. The `edit_root` field still reports the recorded ID. Developed in #13303 Props ramonopoly, andrewserong. Fixes #65987. git-svn-id: https://develop.svn.wordpress.org/trunk@64118 602fd350-edb4-49c9-b593-d223f7449a82
The block editor's image editor creates an entirely new attachment with no stable pointer back to the image the lineage started from. This change records the attachment an edit chain started from in `_wp_attachment_edit_root_id` postmeta, reads it back with `wp_get_edit_root_attachment_id()`, and exposes it as a top-level `edit_root` field on the attachment REST response: * the ID of the edit root, or, * `0` when the image was not created by editing another one. The field is included in the REST API's `edit` context only, alongside an embeddable `wp:edit-root` link so clients can hydrate the edit root with `_embed`. The link is only added when the edit root is published or the user can read it. The `edit_root` field still reports the recorded ID. Developed in WordPress/wordpress-develop#13303 Props ramonopoly, andrewserong. Fixes #65987. Built from https://develop.svn.wordpress.org/trunk@64118 git-svn-id: http://core.svn.wordpress.org/trunk@63274 1a063a9b-81f0-0310-95a4-ce76da25c4cd
3 participants
What? Why?
When Gutenberg crops an image, Core's /edit creates 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.This PR records the attachment an edit chain started from in
_wp_attachment_edit_root_idpostmeta, reads it back withwp_get_edit_root_attachment_id(), and exposes it as a top-leveledit_rootfield on the attachment REST response: the ID of the edit root, or0when the image was not created by editing another one. The field is sent in theeditcontext only, alongside an embeddablewp:edit-rootlink so clients can hydrate the edit root with_embed.The link is only added when the edit root is published or the user can read it, the same check as the
featured_medialink. Theedit_rootfield still reports the recorded ID.Deleting an attachment clears the record from every image edited from it. Records are written going forward only, so images edited before this lands are not backfilled.
Trac ticket: https://core.trac.wordpress.org/ticket/65987
Use of AI Tools
To create the backport of WordPress/gutenberg#81803 and its tests