Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#22985 closed defect (bug) (fixed)

Edit thumbnail image only - loses all sub sizes in attachment meta

Reported by: lewismcarey Owned by: markoheijnen
Milestone: 3.5.1 Priority: high
Severity: critical Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

I have several additional image sizes in my theme. When I crop an image and save over only the thumbnail, all references to the various sub-sizes are lost and only the new thumbnail is referenced in _wp_attachment_metadata.

  1. Add an image to the media library.
  2. Output _wp_attachment_metadata.
  3. Edit the image (crop).
  4. Save only the thumbnail.
  5. Output the relevant _wp_attachment_metadata.

This may be an issue with wp_save_image() not re-genarating subsizes from the original.
I believe this to be different from Ticket #19889 as it appears to remove references to default sizes from media settings screen.

Attachments (2)

22985.diff (529 bytes) - added by markoheijnen 5 years ago.
using array_merge() inside wp_save_image()
media_tests.patch (2.9 KB) - added by bananastalktome 5 years ago.

Download all attachments as: .zip

Change History (23)

#1 @shaunbent
5 years ago

Whilst testing for this issue I have noticed that when using the 'Apply to all image sizes' option that custom image sizes are excluded here as well - so this issue doesn't seem to be limited to just the thumbnail only option.

#2 @lewismcarey
5 years ago

The main diff I can see between image-edit.php 3.4 & 3.5 is

695     $meta['sizes'] = $img->multi_resize( $_sizes );


596    if ( $resized )
597        $meta['sizes'][$size] = $resized;
598    else
599        unset($meta['sizes'][$size]);

before wp_update_attachment_metadata($post_id, $meta); is called on success.

#3 @markoheijnen
5 years ago

  • Milestone changed from Awaiting Review to 3.5.1
  • Owner set to markoheijnen
  • Status changed from new to assigned

I will look into this today. I can see the issue with the code lewis send but I need to look into the context of that since for what I think that get called when a new image getting uploaded. So if that is true that code is unrelated

#4 @markoheijnen
5 years ago

#22983 was marked as a duplicate.

#5 @markoheijnen
5 years ago

  • Keywords needs-patch added

Testing it and yes the multi_resize() call in wp_save_image() is wrong. It will then only put the changed images as meta sizes.

5 years ago

using array_merge() inside wp_save_image()

#6 @markoheijnen
5 years ago

  • Keywords has-patch added; needs-patch removed

#7 @markoheijnen
5 years ago

  • Priority changed from normal to high
  • Severity changed from major to critical

#8 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#9 @markoheijnen
5 years ago

For fixing broken metadata I think can use wp_restore_image() on all images that are missing thumbnail, medium or large on upgrade. Will going to play with it this or next week.

#10 @markjaquith
5 years ago

Let's get the fix in now, and then when we have code to go back and fix the issue on upgrade, put that in too.

#11 @markoheijnen
5 years ago

Was playing to get a script done for hotfixing this issue. The result I have as a gist: https://gist.github.com/4349227

#12 @nacin
5 years ago

In 23246:

Don't stomp existing sizes inside wp_save_image().

props markoheijnen.
see #22985.
for trunk.

#13 @nacin
5 years ago

Went through a bunch of random ideas when talking to westi, regarding an upgrade routine. One is that _edit_lock gets set when entering post.php. We can look for all attachments with an _edit_lock greater than December 11, then individually run over those to fix their metadata. It's not foolproof, but it'll help a decent amount.

_wp_attached_file also gets updated with a suffix on edit, -e followed by 13 digits. The first 10 digits are time() and the last three are rand(100, 999). So we can use the time instead (which would also last through an export/import).

Next, we need to figure out how to fix these. The gist is nice to prevent saves as they occur. But what about once the sizes are missing? What is the best way to re-calculate and recover this data?

#14 @markoheijnen
5 years ago

If I'm correct all the data still exists in get_post_meta( $post_id, '_wp_attachment_backup_sizes', true );
So then it looks like we still can do some kind of merge with the attachment meta data.

#15 @markoheijnen
5 years ago

Anyone working on fixing the lost data? I was checking if we could used backup sizes but that's a no.

So it seems we need to check if an attachment has a '_edit_lock' and then loop check if the meta data sizes doesn't have an image from get_intermediate_image_sizes(). And this should be done as late as possible since themes do need to add their sizes.

#16 @bananastalktome
5 years ago

A little new to phpunit tests with WordPress, but I attempted at a test to cover this case. Thoughts?

#17 @nacin
5 years ago

In 23342:

Don't stomp existing sizes inside wp_save_image().

Merges [23246] to the 3.5 branch.

props markoheijnen.
see #22985.

#18 follow-up: @markoheijnen
5 years ago

Is this fixed and if not what is there to be fixed to close this ticket.

#19 in reply to: ↑ 18 @SergeyBiryukov
5 years ago

Replying to markoheijnen:

Is this fixed and if not what is there to be fixed to close this ticket.

media_tests.patch probably needs to be reviewed and committed.

#20 @bananastalktome
5 years ago

  • Cc bananastalktome@… added

#21 @nacin
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 1305/tests:

First pass at tests for ajax media edit. props bananastalktome. fixes #22985.

Note: See TracTickets for help on using tickets.