Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

#66056 closed defect (bug) (fixed)

Media: prepare the attachment finalize response from the current post row

Reported by: adamsilverstein Owned by: adamsilverstein
Priority: normal Milestone: 7.1.1
Component: Media Version: 7.1
Severity: normal Keywords: has-patch has-unit-tests dev-reviewed
Cc: Focuses:

Description

Follow up to the client-side media processing endpoints (the sideload and finalize routes on WP_REST_Attachments_Controller), needed alongside the Gutenberg package update planned for 7.1.1.

Claude wrote this one up after working through the Gutenberg side:

finalize_item() fetches the attachment post at the top of the method, then applies the wp_generate_attachment_metadata filter and writes the metadata, and finally prepares its response from the $post it fetched at the start. Anything a callback on that filter writes to the attachment's post row - an optimizer changing post_mime_type once it has converted the file, for instance - is missing from the response.

That mattered less while the editor threw the finalize response away and refetched the attachment. With ​https://github.com/WordPress/gutenberg/pull/81947 the editor stores the finalize response as its copy of the attachment record and no longer refetches, so whatever the response carries is what the block keeps for the session. That change fixes ​https://github.com/WordPress/gutenberg/issues/81844, where a URL-keyed host cache answered the refetch with the pre-finalize record and the Image block lost its Resolution control, and it is a candidate for the 7.1.1 package update. The server side needs to land with it.

The fix is small: re-read the post with $this->get_post() after the metadata is written and prepare the response from that. When the re-read fails, because a callback rejected the file and deleted the attachment, return the WP_Error rather than a 200 built from the stale row, so the editor reports the upload as failed instead of storing a record for an attachment that is gone.

Three unit tests cover the finalize response carrying the generated sub-sizes, reflecting a post row rewritten by the filter, and failing when the attachment was deleted.

PR incoming.

Change History (9)

This ticket was mentioned in ​PR #13412 on ​WordPress/wordpress-develop by ​@adamsilverstein.


4 weeks ago
#1

Companion to ​https://github.com/WordPress/gutenberg/pull/81947, which fixes ​https://github.com/WordPress/gutenberg/issues/81844 and is a candidate for the 7.1.1 package update. Both halves need to ship together.

_Claude did the port and the write-up:_

finalize_item() prepares its response from the $post fetched at the top of the method, so anything a wp_generate_attachment_metadata callback writes to the post row is missing from it. The editor is about to keep that response as the attachment record instead of refetching, so the response has to be prepared from the row as it stands after the metadata is written.

This re-reads the post before preparing the response. If the re-read fails, because a callback deleted the attachment, the WP_Error is returned rather than a 200 built from the stale row.

Three tests cover the response carrying the generated sub-sizes, reflecting a post row rewritten by the filter, and failing when the attachment was deleted. The same change and tests are in the Gutenberg controller override in the PR above.

## AI Use

Code and description both written with 🤖 Claude Code. I will review and test.

🤖 Generated with ​Claude Code

​https://claude.ai/code/session_01Ga8xRGdztewVMVJ1rZpUJw

This ticket was mentioned in ​Slack in #core by adamsilverstein. ​View the logs.


4 weeks ago

#3 @adamsilverstein
4 weeks ago

  • Owner set to adamsilverstein
  • Status new → assigned

This ticket was mentioned in ​Slack in #core by adrianduffell. ​View the logs.


4 weeks ago

#5 @adamsilverstein
4 weeks ago

  • Resolution → fixed
  • Status assigned → closed

In 63564:

Media: Prepare the attachment finalize response from the current post data.

finalize_item() prepares its response from the $post fetched before the wp_generate_attachment_metadata filter runs, so anything a callback writes to the post row is missing from the response. The editor now stores that response as the attachment record instead of refetching, so re-read the post after the metadata is written and return the WP_Error if a callback deleted the attachment. See ​https://github.com/WordPress/gutenberg/issues/81844 and ​https://github.com/WordPress/gutenberg/pull/81947.

Props gregbenz, andrewserong.
Fixes #66056.

#6 @adamsilverstein
4 weeks ago

  • Keywords dev-feedback added
  • Resolution fixed
  • Status closed → reopened

Reopening to consider for the 7.1 branch.

This ticket was mentioned in ​Slack in #core-committers by adamsilverstein. ​View the logs.


4 weeks ago

#8 @joedolson
4 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Approved for merge to the 7.1 branch.

#9 @adamsilverstein
4 weeks ago

  • Resolution → fixed
  • Status reopened → closed

In 63572:

Media: Prepare the attachment finalize response from the current post data.

finalize_item() prepares its response from the $post fetched before the wp_generate_attachment_metadata filter runs, so anything a callback writes to the post row is missing from the response. The editor now stores that response as the attachment record instead of refetching, so re-read the post after the metadata is written and return the WP_Error if a callback deleted the attachment. See ​https://github.com/WordPress/gutenberg/issues/81844 and ​https://github.com/WordPress/gutenberg/pull/81947.

Reviewed by joedolson.
Merges [63564] to the 7.1 branch.

Props gregbenz, andrewserong.
Fixes #66056.i

Note: See TracTickets for help on using tickets.