Image block: Validate attachment ID exists before treating image as local#77178
Conversation
…ocal When block markup is copied from one WordPress site to another, image blocks carry attachment IDs from the source site. The editor assumes these IDs reference local attachments and suppresses the Upload to Media Library button. This change adds a check: after the entity record resolution completes, if the attachment ID does not match a local record, the ID is cleared from the block attributes. This allows the existing external image upload flow to activate correctly. Fixes WordPress#74156 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @samvaidya! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
| const hasResolved = | ||
| id && isSingleSelected | ||
| ? select( coreStore ).hasFinishedResolution( | ||
| 'getEntityRecord', | ||
| [ | ||
| 'postType', | ||
| 'attachment', | ||
| id, | ||
| { context: 'view' }, | ||
| ] | ||
| ) | ||
| : false; |
There was a problem hiding this comment.
Thanks for the PR!
Curious, are we only interested in 404s?
I'm just wondering if getResolutionError would be more useful.
hasFinishedResolution returns true for other failed resolutions, e.g., 403, 500
const resolutionError =
id && isSingleSelected
? select( coreStore ).getResolutionError(
'getEntityRecord',
[
'postType',
'attachment',
id,
{ context: 'view' },
]
)
: null;
// Later...
useEffect( () => {
if ( ! id || ! isSingleSelected ) {
return;
}
// Only clear for confirmed 404s. apiFetch throws the Response object
// for HTTP errors (parse: false), so checking .status === 404 avoids
// incorrectly clearing the id on 403, 500, or network failures, which
// would cause data loss for valid local attachments.
if ( attachmentResolutionError?.status === 404 ) {
setAttributes( { id: undefined } );
}
}, [ id, isSingleSelected, attachmentResolutionError, setAttributes ] );
What that satisfy the requirements?
There was a problem hiding this comment.
Great catch, thanks @ramonjd! You're right -- hasFinishedResolution is too broad. A 500 or 403 would also "finish" but shouldn't trigger clearing the ID, since the attachment may exist fine and just be temporarily unreachable or permission-restricted. Clearing it in those cases would be data loss.
Switched to getResolutionError and added a ?.status === 404 check so we only clear on confirmed not-found responses. Updated in 1066e9f.
Thanks too for the learning opportunity -- this is a great example of "be specific about what you're catching." 🙏
|
+1 thanks for the PR! I noticed something while testing which isn't introduced in this PR, but if your environment supports client-side media processing, then when you click the "Upload to Media Library" button in the block toolbar, it'll fire off more notices than expected when the image uploads:
I can look into a fix for this separately, so don't let it stop you proceeding with this PR! It's otherwise testing well for me, but @ramonjd raises a valid point about explicitly checking for 404s. Other than that, I think this is a good pragmatic approach, and as you mention in the description this works well for an MVP that should hopefully capture the majority of cases, with the caveat that it's possible for an id to exist in both environments that both match a valid |
Fix here: #77218 |
Addresses @ramonjd's review: replace hasFinishedResolution with getResolutionError so we can specifically check for 404 status. This prevents the id from being cleared on transient errors (500, 403, network failures), which would cause data loss for valid local attachments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Per @andrewserong's review: note the limitation that if a different attachment with the same id exists on the destination site, the lookup will succeed and use the wrong local image. URL matching could address this in a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for testing and the feedback @andrewserong! 🙏 And thanks for taking care of the extra notices issue separately in #77218! Great suggestion on documenting the ID-collision limitation in the code comment itself. Added in 8a0c693. |
| // | ||
| // Known limitation: if a different attachment with the same id happens to | ||
| // exist on the destination site, the lookup will succeed and the wrong | ||
| // local image will be used. URL matching could address this in a follow-up. |
There was a problem hiding this comment.
This sounds like a good follow up. Not bullet-proof, but even a filename check could be helpful to alert the user (and they can manually resolve if necessary).
ramonjd
left a comment
There was a problem hiding this comment.
This is testing well for me. Cheers!
The 404 response, and the upload button are present. Upload works.
When the id isn't present in the image attributes, e.g.,
<!-- wp:image {"sizeSlug":"large"} -->
<figure class="wp-block-image size-large"><img src="https://picsum.photos/id/237/800/600" alt="" /></figure>
<!-- /wp:image -->the upload button appears as expected.
| // clearing the id on 403, 500, or network failures, which would | ||
| // cause data loss for valid local attachments. | ||
| if ( attachmentResolutionError?.status === 404 ) { | ||
| setAttributes( { id: undefined } ); |
There was a problem hiding this comment.
This can create an undo trap. A user undoes id removal, the whole effect runs again, etc. It's good practice to mark similar changes in effect as non-persistent.
I'll create a follow-up.


Description
When block markup is copied from one WordPress site to another, image blocks carry attachment IDs from the source site. The editor assumes these IDs reference local attachments and suppresses the "Upload to Media Library" button.
This change piggybacks on the existing
getEntityRecordcall (which already fires when an image block is selected) and adds ahasFinishedResolutioncheck. Once resolution confirms the attachment doesn't exist locally, the invalididis cleared from block attributes, allowing the existing external image upload flow to activate correctly.How it works
id: 107491from Site AuseSelectfiresgetEntityRecord('postType', 'attachment', 107491)- returns 404hasFinishedResolutioncheck confirms resolution is complete (not still loading)useEffectdetects: id exists, resolution finished, but no record found - clears theidisExternalImage(undefined, url)now returnstrueWhat this doesn't change
hasFinishedResolutionis a local state check (zero network cost)isSingleSelectedis true (user clicked on the specific image block)Future scope
Testing Instructions
Fixes #74156