Make WordPress Core

Opened 7 years ago

Last modified 5 weeks ago

#40370 reopened enhancement

add_image_sizes does not create the "crop position" versions of the image

Reported by: piejesus's profile piejesus Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 2.9
Component: Media Keywords: good-first-bug has-patch 2nd-opinion has-unit-tests needs-testing
Focuses: Cc:

Description (last modified by joemcgill)

I wanted to introduce 3 different version of post thumbnails - each with different cropping position (top, center, left) , so i added them like this:

add_image_size( 'newscentered', 400, 400, array( 'center', 'center') );
add_image_size( 'newstop', 400, 400, array( 'center', 'top' ) );
add_image_size( 'newsbottom', 400, 400, array( 'center', 'bottom' ) );

Now, whenever i use the the_post_thumbnail() with the name of my custom image size i always get the same image, cropped to the default WordPress crop position of ('center', 'center') .

Why is that happening? I did 'refresh' the thumbnails and tried also uploading fresh image files and still i can't get 3 differently cropped versions of the image.

I noticed that when i set the cropping defaults using the following function and set the cropping there, then it works:

set_post_thumbnail_size( 400, 400, array('center', 'bottom'));

... but it affects the cropping of all of my thumbnails, so i can only get one "crop position" for all my images.

Guys, is this some kind of bug or do i configure something in a wrong way?

I'm using the newest official WordPress version

Attachments (5)

40370.diff (4.7 KB) - added by themattroyal 5 years ago.
40370.patch (1.9 KB) - added by nickylimjj 5 years ago.
Unit test with FIXME
Fix incompatible declarations PHP Warnings - patch.diff (1.9 KB) - added by davidbawiec 3 years ago.
Fix PHP Warnings relating to incompatible declarations with WP_Image_Editor::update_crop_hash($hash = NULL).
40370-refresh-at-6-4.patch (7.2 KB) - added by misfist 6 months ago.
Upload patch previously submitted https://github.com/WordPress/wordpress-develop/pull/5086
40370-refresh-6-4-descriptions.patch (7.8 KB) - added by misfist 6 months ago.
Successfully Patched against 6.4 - https://github.com/misfist/wordpress-develop/pull/3

Download all attachments as: .zip

Change History (39)

#1 follow-up: @sebastian.pisula
7 years ago

  • Keywords dev-feedback added

I think that this is bug in this because each file will be name for example "filename-400x400.jpg" but should be "filename-400x400-center-center.jpg", "filename-400x400-center-top.jpg", "filename-400x400-center-bottom.jpg"

Last registered size will be overwrite file

#2 in reply to: ↑ 1 @piejesus
7 years ago

Ay, yes, this is exactly what's happening, just checked it by changing the sizes by 1 pixel. Thanks you!

Replying to sebastian.pisula:

I think that this is bug in this because each file will be name for example "filename-400x400.jpg" but should be "filename-400x400-center-center.jpg", "filename-400x400-center-top.jpg", "filename-400x400-center-bottom.jpg"

Last registered size will be overwrite file

#3 @swissspidy
7 years ago

  • Focuses administration removed
  • Type changed from defect (bug) to enhancement
  • Version changed from 4.7.3 to 2.9

Introduced in [12342] and extended in #27472.

As @sebastian.pisula mentioned, WordPress stores the files with the dimensions only, nothing in addition to that. I wouldn't consider this a bug though, but expected behaviour.

#4 @alexvorn2
7 years ago

I can advice a plugin, maybe it will help - https://wordpress.org/plugins/resize-post-thumbnails/

#5 in reply to: ↑ description @anonymized_5746546
6 years ago

Replying to piejesus:

I wanted to introduce 3 different version of post thumbnails - each with different cropping position (top, center, left) , so i added them like this:

add_image_size( 'newscentered', 400, 400, array( 'center', 'center') );
add_image_size( 'newstop', 400, 400, array( 'center', 'top' ) );
add_image_size( 'newsbottom', 400, 400, array( 'center', 'bottom' ) );

Now, whenever i use the the_post_thumbnail() with the name of my custom image size i always get the same image, cropped to the default Wordpress crop position of ('center', 'center') .

Why is that happening? I did 'refresh' the thumbnails and tried also uploading fresh image files and still i can't get 3 differently cropped versions of the image.

I noticed that when i set the cropping defaults using the following function and set the cropping there, then it works:

set_post_thumbnail_size( 400, 400, array('center', 'bottom'));

... but it affects the cropping of all of my thumbnails, so i can only get one "crop position" for all my images.

Guys, is this some kind of bug or do i configure something in a wrong way?

I'm using the newest official Wordpress version

Hello, I'm using version 4.9 now (2017) and the same thing is still happening. Was wondering if it had been "enhanced" in previous versions?

#6 @peterwilsoncc
5 years ago

  • Description modified (diff)

I've hit this recently too, the problem occurs during the upload process.

Using the crop names in the original report, the three images result in the same file name. During resize newscentered is saved, it's then replaced by the newstop crop which is in turn replaced by the newsbottom crop.

While site owners don't get the crops they expect, a side-effect is the srcset for an image may contain different crop orientations.

I suggest files be saved with an eight character hash of the crop information returned by image_resize_dimensions() to ensure neither of these problems occur. The file name could be uploaded-name-c{$hash}.ext.

image_resize_dimensions()'s return array is:

<?php

$dims = [
  0 => 0 
  1 => 0 
  2 => // Crop start X axis
  3 => // Crop start Y axis
  4 => // New width
  5 => // New height
  6 => // Crop width on source image
  7 => // Crop height on source image
];

The crop hash could be generated from $source_w,$source_h,$dims[2],$dims[3],$dims[6],$dims[7]. This will ensure images from the same source with the same aspect ratio and crop will end up with the same hash.

If the final two values are the same as the source image then no hash is required.

When generating the srcset in wp_calculate_image_srcset() the crop hash can be checked in the same way as that the edit suffix is checked.

In terms of backward compatibility I don't think there are implications as previously uploaded images will not have a crop hash.

cc @joemcgill as we've discussed this previously.

Last edited 5 years ago by peterwilsoncc (previous) (diff)

#7 @joemcgill
5 years ago

  • Description modified (diff)
  • Keywords needs-patch needs-unit-tests good-first-bug added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

This is indeed an oversight in the way file naming occurs. I'd be +1 for a patch that adds a hash to the end of the filename whenever a crop position is included in the image size. So something like: filename-{width}x{height}-{hash}.ext.

@themattroyal
5 years ago

#8 @themattroyal
5 years ago

  • Keywords has-patch added; needs-patch removed

@joemcgill @peterwilsoncc - I have implemented this based on my understanding of the requirements set out in your comments.

https://core.trac.wordpress.org/attachment/ticket/40370/40370.diff

#9 follow-up: @nickylimjj
5 years ago

After applying the patch from @themattroyal, I do not see any difference in the files saved in build/wp-content/uploads/... and only one file is saved as filename-400x400.png

Not sure how to proceed with unit test for this.

Also @junkai is looking into this

#10 in reply to: ↑ 9 @nickylimjj
5 years ago

Replying to nickylimjj:

After applying the patch from @themattroyal, I do not see any difference in the files saved in build/wp-content/uploads/... and only one file is saved as filename-400x400.png

Not sure how to proceed with unit test for this.

Also @junkai is looking into this

Solved in replicating. I forgot to build it with grunt after applying the patch. Working on the unit test now.

@nickylimjj
5 years ago

Unit test with FIXME

#11 @nickylimjj
5 years ago

Any updates? To more experience contributors, if you can offer any tips to my latest patch above, do let me know and I will update it. Thanks.

#12 @indeveler
5 years ago

Is there any updates about this issue?

#13 @subhankara
5 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Hi,

I think you need to regenerate all post thumbnails.

add_image_size( string $name, int $width, int $height, bool|array $crop = false );

Before uploading all images are not regenerate automatically. You need to regenerate manually or add plugin https://wordpress.org/plugins/regenerate-thumbnails/

But after adding this code all images are regenerate automatically you do not need to regenerate again.

And you can use this function to show the Image url :

wp_get_attachment_image_src(get_post_thumbnail_id( POST_ID ) ,'CUSTOM_SIZE');

Thanks,
Subhankar

#14 @themattroyal
5 years ago

  • Keywords 2nd-opinion added
  • Resolution invalid deleted
  • Status changed from closed to reopened

@joemcgill @peterwilsoncc - can you guys please confirm that this should be closed and flagged as invalid? I don't believe it should be based on the outlined requirements.

This is my first contribution to core and have worked really long and hard to get to a point I can contribute back and sadly its been a very disheartening process so far :( I would really appreciate some feedback and help to make it happen. Please could you guys give me a helping hand :)

#15 @peterwilsoncc
5 years ago

@themattroyal Yes, you are correct; this is a valid bug.

@subhankara add_image_size() allows developers to define the crop location on hard cropped images. Multiple images of the same size but with different crop locations (eg 300x300 center cropped, and 300x300 top cropped) produce the same file name and one crop deletes the other.

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


5 years ago

#17 @azaozz
5 years ago

There was a discussion in the #core-media channel on Slack about image sub-size names and file names.

There are several related tickets and couple of ideas about how to fix them. Generally the solution seems to be to add a short hash to image sub-size file names (like in 40370.diff) but in order to fix the file name clashing with original files named like my-image-file-name-800x600.jpg, it would need to add that hash in all cases, not just for cropped images. Then the hash can be generated from the sub-size settings array. For example:

$size_settings = array (
    'width'  => 1024,
    'height' => 1024,
    'crop'   => false,
),

would result in:

$str = '';

// `crop` may be an array.
array_walk_recursive( $size_settings, function( $value ) {
     $str .= $value;
} );

$hash = substr( md5( $str ), 0, 8 );

Another option would be to add the "size name" to the image file name. That should work properly as the size name is unique, but may make the file name too long in some cases (may need to shorten it). Example:

[my-image-name]-medium-400x300.jpg
[my-image-name]-large-1024x768.jpg
[my-image-name]-newscentered-400x400.jpg
[my-image-name]-newstop-400x400.jpg
...

Yet another idea is to append -1, -2, etc. to the file name (before the -800x600 part) when testing for unique file name. That will be a bit more complex in case of sizes with the same dimensions and different crop values. It will have to keep previously tested and approved file names in cache and test/increment against them too.

Related: #42437.

Last edited 5 years ago by azaozz (previous) (diff)

#18 @davidbawiec
3 years ago

Hi all,

Happy new year! Checking in on this. This is still (sadly) a valid bug in WordPress 5.6. In fact, this thread on StackExchange from 2015 suggests it's been a bug for 6 years now, so it would be great to get this one squashed and out the door. :)

As previously suggested, adding either a hash or the size name to the filename would definitely solve this issue.

As a temporary fix, a potential is also to add a filter to the get_suffix() method in wp-includes/class-wp-image-editor.php:

ORIGINAL CODE:

public function get_suffix() {
		if ( ! $this->get_size() ) {
			return false;
		}

		return "{$this->size['width']}x{$this->size['height']}";
	}

MODIFIED CODE:

public function get_suffix() {
		if ( ! $this->get_size() ) {
			return false;
		}


		$format = "{$this->size['width']}x{$this->size['height']}";
		return apply_filters('get_image_filename_suffix', $format, $this->size, $size_name );
	}

The filter could allow devs to override the naming format for image thumbnails. But that would also require passing the requested size name to the filter. So having the naming convention altered in WP Core to fix this altogether would be fantastic and a preferred solution to this "filter" patch.

Either way would love to hear about any progress that has been made on this bug. @azaozz it seems like you were last involved in some conversations on this?

Cheers!
David

Last edited 3 years ago by davidbawiec (previous) (diff)

@davidbawiec
3 years ago

Fix PHP Warnings relating to incompatible declarations with WP_Image_Editor::update_crop_hash($hash = NULL).

#19 @davidbawiec
3 years ago

@themattroyal your modification of the code works like a charm for the most part. Thanks!

In PHP 7.1.33 it throws the following two Warnings:

Declaration of WP_Image_Editor_GD::update_crop_hash($crop, $dst_w, $dst_h) should be compatible with WP_Image_Editor::update_crop_hash($hash = NULL)
Declaration of WP_Image_Editor_Imagick::update_crop_hash($crop, $dst_w, $dst_h) should be compatible with WP_Image_Editor::update_crop_hash($hash = NULL)

To fix it, I suggest renaming the instances of the code update_crop_hash() in the child classes to create_crop_hash().

I've created a patch that does just that and that seems to fix the PHP Warnings.
Fix incompatible declarations PHP Warnings - patch.diff

Would love to hear your thoughts.
Thanks!

#20 @themattroyal
3 years ago

@davidbawiec looks good to me :)

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


2 years ago

This ticket was mentioned in PR #5081 on WordPress/wordpress-develop by @misfist.


7 months ago
#22

Patches failed on 6.4.
Updated patch for WP 6.4.

Trac ticket: `add_image_sizes` does not create the "crop position" versions of the image

#23 @antpb
7 months ago

  • Milestone changed from Future Release to 6.4

This ticket was mentioned in PR #5086 on WordPress/wordpress-develop by @misfist.


7 months ago
#24

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Applied 3 patches
  • Inserted patch updates that failed
  • Corrected code formatted for newly added code

Additional

Unit testing is required.

In manual tests, I found that an image uploaded when there are 3 image sizes with the same dimensions, but different crop settings, separate images were created for each of the images sizes.

Give the usecase:

add_image_size( 'newscentered', 400, 400, array( 'center', 'center') );
add_image_size( 'newstop', 400, 400, array( 'center', 'top' ) );
add_image_size( 'newsbottom', 400, 400, array( 'center', 'bottom' ) );

Uploaded the image carmella-red-dress.png

Produced the following files:
carmella-red-dress.png (original file)
carmella-red-dress-400x400-53fd9f7a.png
carmella-red-dress-400x400-420047e7.png
carmella-red-dress-400x400-34920233.png

Trac ticket: add_image_sizes does not create the "crop position" versions of the image

#25 @misfist
7 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Patches were tested for merging into v6.4 and failed code was incorporated. Minor WP coding standards formatting changes were made.

Details can be found on the GitHub repo: https://github.com/WordPress/wordpress-develop/pull/5086

Should be ready for unit testing.

Last edited 7 months ago by misfist (previous) (diff)

@TimothyBlynJacobs commented on PR #5086:


7 months ago
#26

Thanks for the PR @misfist! It looks like there are some linting issues we need to resolve.

Could you also add a description for the function parameters we are introducing?

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


7 months ago

#28 follow-up: @oglekler
6 months ago

@misfist possibly you missed previous comment. There are little WPCS issues that need to be addressed. Please, address previous comment. We are getting close to Beta 1 and the patch still needs to be tested.

#29 in reply to: ↑ 28 @misfist
6 months ago

Replying to oglekler:

@misfist possibly you missed previous comment. There are little WPCS issues that need to be addressed. Please, address previous comment. We are getting close to Beta 1 and the patch still needs to be tested.

My apologies! I will address this ASAP.

All the best,
~/Pea

This ticket was mentioned in PR #5269 on WordPress/wordpress-develop by @misfist.


6 months ago
#30

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Successfully ran "patch:https://github.com/misfist/wordpress-develop/pull/3" (patch) task
    • patching file src/wp-includes/class-wp-image-editor-gd.php
    • patching file src/wp-includes/class-wp-image-editor-imagick.php
    • patching file src/wp-includes/class-wp-image-editor.php
    • patching file tests/phpunit/tests/media.php
  • Added descriptions for newly introduced functions
    • WP_Image_Editor_GD::create_crop_hash
    • WP_Image_Editor_Imagick::create_crop_hash
    • WP_Image_Editor::get_crop_hash
    • WP_Image_Editor::set_crop_hash
  • Bumped WP version @since 5.1.0 >6.4.0
  • Linted and fixed code formatting

Trac ticket: https://core.trac.wordpress.org/ticket/40370

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


6 months ago

#32 @oglekler
6 months ago

  • Keywords needs-testing added
  • Milestone changed from 6.4 to 6.5

This ticket was discussed during bug scrub.

Sadly we don't have time for propper testing, and it needs to be tested thoroughly, so I am moving it into the next milestone.

Add props @hellofromtonya

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


7 weeks ago

#34 @swissspidy
5 weeks ago

  • Milestone changed from 6.5 to 6.6

Moving out of 6.5 due to lack of traction and beta 1 approaching.

Note: See TracTickets for help on using tickets.