Make WordPress Core

Opened 8 years ago

Last modified 14 months ago

#35593 reviewing defect (bug)

Metadata from wrong file in media_handle_upload

Reported by: normanrz's profile normanrz Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch needs-testing
Focuses: administration Cc:

Description (last modified by swissspidy)

In media_handle_upload and media_handle_sideload the attachment metadata is generated after inserting the attachment. For that the same file name is used. However, if a plugin changes the filename in an add_attachment hook the wrong file name will be used. Thus the wrong metadata is written or it is cleared.

See wp-admin/includes/media.php:

368	        // Save the data
369	        $id = wp_insert_attachment($attachment, $file, $post_id);
370	        if ( !is_wp_error($id) ) {
371	                wp_update_attachment_metadata( $id, wp_generate_attachment_metadata( $id, $file ) );
372	        }

In my patch I query the filename again before updating the metadata.

Attachments (1)

media.diff (779 bytes) - added by normanrz 8 years ago.

Download all attachments as: .zip

Change History (12)

@normanrz
8 years ago

#1 @swissspidy
8 years ago

  • Description modified (diff)
  • Keywords has-patch needs-testing added
  • Summary changed from Metadata from wronge file in media_handle_upload to Metadata from wrong file in media_handle_upload

#2 @obenland
8 years ago

  • Version trunk deleted

#3 @joemcgill
8 years ago

  • Keywords reporter-feedback added
  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi @normanrz,

Thanks for the submission. I can see how this would indeed cause unexpected behavior.

However, it seems like a plugin that is firing on an action hook to change the filename should also take responsibility to update the metadata using the wp_update_attachment_metadata hook rather than adding an extra DB hit for everyone. Any reason why that wouldn't be a workable solution?

#4 @joemcgill
8 years ago

  • Keywords close added

Hi @normanrz,

Curious if you were able to look into my previous suggestion about updating the metadata using the wp_update_attachment_metadata hook?

Thanks,
Joe

#5 @normanrz
7 years ago

Hi @joemcgill,
thanks for looking into this and sorry for the long wait.
I think the problem is that when I use the drag&drop feature to upload files the following is happening:

  • media_handle_upload is invoked and creates the $file variable
  • media_handle_upload calls wp_insert_attachment which then calls the add_attachment hook
  • now my plugin changes filename (and in fact calls wp_update_attachment_metadata)
  • the plugin returns to wp_insert_attachment and that returns to media_handle_upload
  • finally media_handle_upload invokes wp_update_attachment_metadata again, but with the stale $file object (because my plugin actually changed some of the metadata) which it created before

It seems to me that I can't handle this in the plugin because any metadata will be later overriden by the stale information from media_handle_upload.

Thanks
Norman

#6 @hellofromTonya
3 years ago

@joemcgill What do you think of @normanrz last comment? Is this ticket still a close candidate?

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#8 @joemcgill
3 years ago

  • Keywords close removed
  • Owner joemcgill deleted

@hellofromTonya sounds like this could use further investigation to reproduce and fix this issue. I don't want to be a bottle neck further on this if someone else wants to investigate.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 years ago

#10 @hellofromTonya
3 years ago

  • Keywords reporter-feedback removed
  • Milestone set to Awaiting Review
  • Owner set to hellofromTonya

Thanks @joemcgill. Assigning this ticket to me as owner. Moving to Awaiting Review for further investigation.

#11 @hellofromTonya
14 months ago

  • Owner hellofromTonya deleted

Removing myself as owner to avoid bottleneck.

Note: See TracTickets for help on using tickets.