WordPress.org

Make WordPress Core

Opened 3 days ago

Last modified 2 days ago

#48451 new defect (bug)

Regression: wp_update_attachment_metadata filter now fires very often and without complete metadata

Reported by: ianmjones Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version: trunk
Component: Media Keywords: needs-patch 2nd-opinion
Focuses: Cc:
PR Number:

Description

WP5.3 has changed how often the wp_update_attachment_metadata filter is fired when an image is uploaded to the Media Library.

It fires initially with just the full (and potentially original_image) details but no sizes, and then again as each image sub-size is generated and added to the sizes array.

With wp_update_attachment_metadata now firing a minimum of 5 times (10 for "large" files) instead of once, I predict quite a resource hit from plugins processing the filter. There's plenty of image related plugins that hang off that filter to trigger optimizations and other media related processes assuming that the upload is now complete and thumbnails available.

We regularly see customers with 25-40 or more registered image sizes, that's going to mean rather a lot of database calls to update the metadata as each sub-size is generated, followed by plugins processing the image(s) each time.

Maybe the solution is for wp_update_attachment_metadata() to gain an $unfiltered param that is set to true during initial upload and sub-size generation, with a final call to the function without $unfiltered when done and dusted?

A common pattern throughout WP5.2 (and still in WP5.3) is as follows...

wp_update_attachment_metadata( $id, wp_generate_attachment_metadata( $id, $file ) );

Such as in \media_handle_upload() and friends.

So that's why a lot of plugins wait for the complete metadata to be generated (wp_generate_attachment_metadata) and then saved (wp_update_attachment_metadata) before handling the new upload.

Change History (5)

#1 follow-up: @azaozz
3 days ago

  • Keywords needs-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to 5.3

Moving to 5.3 for consideration.

At first look:

A common pattern throughout WP5.2 (and still in WP5.3) is as follows...
wp_update_attachment_metadata( $id, wp_generate_attachment_metadata( $id, $file ) );

Right, but wp_update_attachment_metadata() is also used in quite a few other cases. For example when editing an attachment post, in wp_ajax_crop_image(), to update the ID3 data for audio files in wp_ajax_save_attachment(), when editing an image (crop, rotate, ...), for custom background and header images, etc.

Seems using the same filter to detect when images have been uploaded successfully is problematic?

Until now the wp_generate_attachment_metadata filter was the (de facto) "upload is successful" hook. This is still the case in n 5.3, however when the initial upload request fails and the client makes another request to finish image post-processing wp_generate_attachment_metadata is not fired. There is a proposed "action" in (the new) wp_update_image_subsizes(), see https://core.trac.wordpress.org/attachment/ticket/47872/47872.3.diff.

Thinking that adding an $unfiltered param to wp_update_attachment_metadata() and avoid filtering would be wrong. Other plugins may be using that filter for other purposes, for example moving/updating/fixing the paths to the image sub-sizes.

It's also possible that the image attachment meta data is stored temporarily "somewhere" while image sub-sizes are being created (another post meta?), then "moved" to the proper place. At first look not liking much that workaround.

Any other ideas and/or possible fixes? :)

#2 @azaozz
3 days ago

Looking again at the proposed wp_update_image_subsizes action in https://core.trac.wordpress.org/attachment/ticket/47872/47872.3.diff.

Think the problem reported here is related and would be better to handle that on this ticket. Both are about firing a hook after an image has been uploaded and all sub-sizes were created successfully.

Perhaps it would be better to fire wp_generate_attachment_metadata and then save the meta again with wp_update_attachment_metadata() at the end of wp_update_image_subsizes()?

#3 in reply to: ↑ 1 @ianmjones
3 days ago

Replying to azaozz:

Right, but wp_update_attachment_metadata() is also used in quite a few other cases. For example when editing an attachment post, in wp_ajax_crop_image(), to update the ID3 data for audio files in wp_ajax_save_attachment(), when editing an image (crop, rotate, ...), for custom background and header images, etc.

Seems using the same filter to detect when images have been uploaded successfully is problematic?

I should clarify the problem here, it's not so much that the wp_update_attachment_metadata is used for knowing when an upload is complete, it is more used to know when the image and its sub-sizes have potentially changed.

So initially yes, on a new upload we (WP Offload Media) use it as a signal that an image has been uploaded and all of its thumbnails are present and correct and ready to be sent on their merry way to a storage provider.

However, if thumbnails are regenerated, or an image optimizer plugin batch job processes the image and updates the thumbnails, they too generally call wp_update_attachment_metadata when they are done with the Media Library item. And again, that means plugins like WP Offload Media can re-offload the images to S3/GCS/DO Spaces etc.

The problem with the new setup is that it fires wp_update_attachment_metadata a lot, and with partial metadata (e.g. no sizes array initially). Which is very different to how it has been.

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


3 days ago

#5 @ianmjones
2 days ago

  • Summary changed from Regression: wp_update_attachment_metadata filter now fires very often and without compete metadata to Regression: wp_update_attachment_metadata filter now fires very often and without complete metadata
Note: See TracTickets for help on using tickets.