Custom Header Image Cropping deletes header images

[westi]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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

[ryan]

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

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

20657.4.patch​ implements the idea.

[ryan]

Hmmm, leaning toward 20657.patch for 3.4.

[ryan]

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.

[ryan]

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.

[greuben]

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

[azaozz]

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.

[SergeyBiryukov]

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​.

[ryan]

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.

[SergeyBiryukov]

Replying to ryan:

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

Done in 20657.7.patch​.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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​.

[ryan]

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().

[SergeyBiryukov]

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().

[ryan]

.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.

[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.

[ryan]

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.

[SergeyBiryukov]

Replying to ryan:

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

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

[SergeyBiryukov]

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

[ryan]

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

[SergeyBiryukov]

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

[ryan]

In [20808]:

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

[ryan]

In [21039]:

Make sure the path exists before copying. see #20657