WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 4 months ago

#40370 reopened enhancement

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

Reported by: piejesus Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9
Component: Media Keywords: needs-unit-tests good-first-bug has-patch 2nd-opinion
Focuses: Cc:
PR Number:

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

40370.diff (4.7 KB) - added by themattroyal 13 months ago.
40370.patch (1.9 KB) - added by nickylimjj 10 months ago.
Unit test with FIXME

Download all attachments as: .zip

Change History (19)

#1 follow-up: @sebastian.pisula
3 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
3 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
3 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
3 years ago

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

#5 in reply to: ↑ description @enochfung
2 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
14 months 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 14 months ago by peterwilsoncc (previous) (diff)

#7 @joemcgill
14 months 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
13 months ago

#8 @themattroyal
13 months 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
10 months 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
10 months 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
10 months ago

Unit test with FIXME

#11 @nickylimjj
10 months 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
7 months ago

Is there any updates about this issue?

#13 @subhankara
6 months 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
6 months 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 months 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.


4 months ago

#17 @azaozz
4 months 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 4 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.