WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 5 months ago

#20657 closed defect (bug) (fixed)

Custom Header Image Cropping deletes header images

Reported by: westi Owned by: ryan
Milestone: 3.4 Priority: high
Severity: blocker Version: 3.4
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

Step to reproduce the issue.

  1. Go to Appearance → Header when using Twenty Eleven
  2. Upload an image where one of the dimensions matches the suggested dimensions ( Mine was 1000 width x 649 height)
  3. Enlarge the cropping marker to cover the whole image (Because I don't want to crop it)
  4. Hit "Crop and Publish" button
  5. The header image appears in the Custom Header admin page as expected.
  6. The header image appears on the front-end also as expected BUT…. wait for a few minutes.
  7. Open an incognito window to check the front-end.
  8. Header image is missing :( It's missing in the media library also :(

Looking at the file system I see the file uploaded fine - header.jpg and then it vanishes and I only have header-462x300.jpg and friends - no cropped-header.jpg and no original.

The cause for this is the code in Custom_Image_Header::step_3() it doesn't check the return value from wp_crop_image() and just assumes that the image will be cropped.

Later in the function we delete the original files and so we have no longer got the file that the "cropped" file attachment would have pointed at.

Relates to the changes for #19840

Attachments (12)

20657.patch (724 bytes) - added by SergeyBiryukov 2 years ago.
20657.2.patch (1.1 KB) - added by SergeyBiryukov 2 years ago.
20657.3.patch (2.9 KB) - added by SergeyBiryukov 2 years ago.
20657.4.patch (2.6 KB) - added by SergeyBiryukov 2 years ago.
20657.5.diff (1.7 KB) - added by greuben 2 years ago.
20657.6.patch (2.0 KB) - added by SergeyBiryukov 2 years ago.
20657.7.patch (4.4 KB) - added by SergeyBiryukov 2 years ago.
20657.8.patch (3.1 KB) - added by SergeyBiryukov 2 years ago.
20657.9.patch (6.1 KB) - added by SergeyBiryukov 2 years ago.
20657.10.patch (6.2 KB) - added by SergeyBiryukov 2 years ago.
20657.11.patch (6.9 KB) - added by SergeyBiryukov 2 years ago.
20657.minor-typo-fix.patch (860 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (41)

comment:1 iamtakashi2 years ago

  • Cc takashi@… added

SergeyBiryukov2 years ago

comment:2 SergeyBiryukov2 years ago

  • Component changed from General to Appearance
  • Keywords has-patch added; needs-patch removed

20657.patch removes the part that was an additional suggestion in #20555.

20657.2.patch makes sure that the file was actually cropped before deleting the original.

SergeyBiryukov2 years ago

SergeyBiryukov2 years ago

comment:3 SergeyBiryukov2 years ago

20657.3.patch also handles the case when wp_crop_image() returns false.

comment:4 dllh2 years ago

  • Cc daryl@… added

comment:5 ryan2 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [20769]:

Check wp_crop_image() for a false return value. Don't delete original image if crop not sucessful. Don't delete the original image when wp_crop_image() returns it untouched. Prevents deletion of header image when no cropping is done to the originally uploaded image. Props SergeyBiryukov, westi. fixes #20657

comment:6 SergeyBiryukov2 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  1. Add an image to Media Library which doesn't require cropping (1000x288 for Twenty Eleven).
  2. Go to Appearance → Header, click "Choose from image library".
  3. Select the image from point 1, click "Set as header", "Crop and Publish".
  4. Delete the new "Header Image" attachment from Media Library.
  5. The original attachment is now orphaned (the files are deleted in point 4).

20657.patch (always run through cropping) seems to be the safest way for now.

Or, wp_crop_image() should always copy the file under a different name, even with no changes.

SergeyBiryukov2 years ago

comment:7 SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

Or, wp_crop_image() should always copy the file under a different name, even with no changes.

20657.4.patch implements the idea.

comment:8 follow-up: ryan2 years ago

Hmmm, leaning toward 20657.patch for 3.4.

comment:9 in reply to: ↑ 8 ryan2 years ago

Replying to ryan:

Hmmm, leaning toward 20657.patch for 3.4.

Or, maybe not. 3.3 did let you skip cropping in more situations. And not touching an image when not necessary is best.

comment:10 follow-up: ryan2 years ago

After talking with nacin, .4.patch sounds like the direction to go.

A "Skip Crop, Use Image as Is" type button on the crop page was also discussed. If the image is within the flex limits, present another button that allows skipping the crop so you don't have to pointlessly fiddle with size selection. Seems like it would be a small patch and doable for 3.4, if we like it.

Last edited 2 years ago by ryan (previous) (diff)

greuben2 years ago

comment:11 follow-up: greuben2 years ago

20657.5.diff or we can create a new attachment only when the image is cropped.

comment:12 in reply to: ↑ 10 azaozz2 years ago

Replying to ryan:

A "Skip Crop, Use Image as Is" type button on the crop page was also discussed. If the image is within the flex limits, present another button that allows skipping the crop so you don't have to pointlessly fiddle with size selection. Seems like it would be a small patch and doable for 3.4, if we like it.

+1. Seems like the most logical thing to do.

SergeyBiryukov2 years ago

comment:13 in reply to: ↑ 11 SergeyBiryukov2 years ago

Replying to greuben:

or we can create a new attachment only when the image is cropped.

That is much simpler. Perhaps 20657.4.patch is not necessary then.

20657.6.patch is an attempt at the "Skip Crop, Use Image as Is" button. Includes 20657.5.diff.

comment:14 follow-up: ryan2 years ago

If the existing attachment is used when the image is not cropped, then _wp_attachment_is_custom_header could get stomped if the attachement is used by more than one theme. Wondering if always creating a new attachment would be best policy here.

SergeyBiryukov2 years ago

comment:15 in reply to: ↑ 14 SergeyBiryukov2 years ago

Replying to ryan:

Wondering if always creating a new attachment would be best policy here.

Done in 20657.7.patch.

SergeyBiryukov2 years ago

comment:16 SergeyBiryukov2 years ago

Seems like 20657.patch is still the safest way after all, see #20667.

Included in 20657.8.patch. The only remaining concern is that the original attachment is not copied if "Skip Crop, Use Image as Is" was chosen, so comment:14 still applies.

SergeyBiryukov2 years ago

comment:17 SergeyBiryukov2 years ago

The only remaining concern is that the original attachment is not copied if "Skip Crop, Use Image as Is" was chosen, so comment:14 still applies.

Solved in 20657.9.patch.

comment:18 follow-up: ryan2 years ago

Can $dst_file be constructed from $src_file when src_file is a url? Seems we'd need get_attached_file() to create the dst_file path and name and use _load_image_to_edit_path() as the source when doing the copy().

SergeyBiryukov2 years ago

comment:19 in reply to: ↑ 18 SergeyBiryukov2 years ago

Replying to ryan:

Seems we'd need get_attached_file() to create the dst_file path and name and use _load_image_to_edit_path() as the source when doing the copy().

Right, done in 20657.10.patch. Perhaps we should also silence a possible warning from copy() in case a file is not accessible for some reason (included in the patch). The error message will be displayed in step_3().

comment:20 follow-up: ryan2 years ago

.10.patch is working well so far.

When uploading an image and then using that image as is with no cropping, is _copy_image_file() necessary? Seems it isn't. Always copying does simplify the logic quite a bit, however.

comment:21 in reply to: ↑ 20 ; follow-up: SergeyBiryukov2 years ago

Replying to ryan:

When uploading an image and then using that image as is with no cropping, is _copy_image_file() necessary?

I guess so. Otherwise _wp_attachment_is_custom_header could be overwritten by another theme, as you mentioned in comment:14.

comment:22 in reply to: ↑ 21 ; follow-up: ryan2 years ago

Replying to SergeyBiryukov:

Replying to ryan:

When uploading an image and then using that image as is with no cropping, is _copy_image_file() necessary?

I guess so. Otherwise _wp_attachment_is_custom_header could be overwritten by another theme, as you mentioned in comment:14.

Seems like that path (upload new image from custom header page and skip cropping) creates an attachment with the uploaded image, copies the image in that attachment, updates the attachment with the copied image, and then deletes the original image. The original image and the copy are identical all the way through.

Probably not worth worrying with, but I want to make sure I'm understanding all of the different paths through this code.

comment:23 in reply to: ↑ 22 SergeyBiryukov2 years ago

Replying to ryan:

The original image and the copy are identical all the way through.

Ah, I see. I misread the comment. Will reconsider.

SergeyBiryukov2 years ago

comment:24 SergeyBiryukov2 years ago

20657.11.patch only copies the file if an existing image was selected.

comment:25 ryan2 years ago

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

In [20806]:

Create a new attachment and make a copy of the image when selecting an image from the image library. This prevents orphaning the header if the original attachment is deleted. This also prevents stomping of meta.

Add a button to skip cropping.

Props SergeyBiryukov
Fixes #20657 #20667

comment:26 SergeyBiryukov2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

20657.minor-typo-fix.patch fixes a PHPDocs typo and removes debug line.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

comment:27 ryan2 years ago

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

In [20808]:

Remove debug cruft. Fix typo. Props SergeyBiryukov. fixes #20657

comment:28 ryan2 years ago

In [21039]:

Make sure the path exists before copying. see #20657

comment:29 ircbot5 months ago

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.

Note: See TracTickets for help on using tickets.