image_get_intermediate_size() may return wrong thumbnail size

[chipbennett]

See ​this WPSE question for a more detailed explanation of the problem.

Essentially, if get_the_post_thumbnail() is passed an array for the $size argument, and if two images have one dimension exactly the same, the image with the smaller opposing dimension will be returned, even if the dimensions for the other image are declared explicitly.

The issue appears to be due to the way that image_get_intermediate_size() determines if an image exists that is cropped to dimensions similar to the specified $size array. As soon as it finds one, it uses it.

I've attached a patch that first attempts to find an image cropped exactly on both dimensions, before looking for images cropped exactly only on one dimension. There will still be edge cases where the wrong image might be returned, but I'm not sure of the most efficient way to handle such cases.

(Note: props to Rarst for finding the underlying issue.)

[chipbennett]

slight modification to image_get_intermediate_size()

[scribu]

I've ran into this issue myself.

[chipbennett]

I'm trying to work out the best way to cover the remaining edge case. For example, two image sizes have the same width, but different heights. In this case, the image size with the smaller height will return, even if the image size with the larger height has a height that is smaller than the height specified in the $size array.

My thinking is that, if $imagedata['sizes'] can be sorted from high to low on the variable dimension, before looping through the sizes, then the largest, rather than smallest, image would be returned.

Would a function like this work, prior to looping through the images?

// sort $imagemeta['sizes'] descending by width, height
function compare_sizes( $a, $b ) {
	$sorted = strnatcmp( $a['width'], $b['width'] );
	if( ! $sorted ) return strnatcmp( $a['height'], $b['height'] );
	array_reverse( $sorted );
	return $sorted;
}
usort( $imagedata['sizes'], 'compare_sizes' );

(I'm a bit out of my depth on this one, with sorting associative arrays. If there's a better way to do this, I'm all ears.)

[scribu]

You can use ​uasort().

[chipbennett]

@scribu should I update the patch to use uasort() instead of usort()?

[chipbennett]

Replaced usort() with uasort(), per @scribu's recommendation

[scribu]

Note that, even if you define the compare_sizes() function inside the if, it's still created globally.

Better to define it outside of image_get_intermediate_size(), as a private helper function - that is to say with a leading underscore: _compare_sizes(), or something even more obscure, to prevent collisions.

[SergeyBiryukov]

Replying to scribu:

Better to define it outside of image_get_intermediate_size(), as a private helper function - that is to say with a leading underscore: _compare_sizes(), or something even more obscure, to prevent collisions.

Done in 17626.patch​.

[scribu]

Related: #18532

[SergeyBiryukov]

Related: #18799

[chipbennett]

Hey scribu, any chance we can move on this one?

[scribu]

One question: in _compare_imagedata_sizes(), why is strnatcmp() used to compare the dimensions (which are just numbers)?

[chipbennett]

Replying to scribu:

One question: in _compare_imagedata_sizes(), why is strnatcmp() used to compare the dimensions (which are just numbers)?

Not sure? Probably because it worked, and I wasn't aware of a better method? :) I can change it though. Suggestions?

[scribu]

Just use plain comparison operators: <, >, =, as in the uasort example: ​http://php.net/uasort#example-4584

[SergeyBiryukov]

Noticed an inconsistency in the comments: $imagemeta['sizes'] should be $imagedata['sizes'].

[nacin]

In [20696]:

Have the $fields argument for XML-RPC's wp.getPostType, getPostTypes, wp.getTaxonomy and wp.getTaxonomies take the names of the post type or taxonomy object keys that will be returned.

• capabilities becomes cap
• object_types becomes object_type

fixes #17626.

[nacin]

[20696] was meant for #20566.

Re-opening, and also punting.

[SergeyBiryukov]

Related: #21004

[roytanck]

I'm running into this bug, and it's driving me crazy.

• Client uploads a 208*60px image for use in the Image Widget plugin
• Thumbnail size is set to 120*120px
• WP creates a 120*60px version of the image
• image_get_intermediate_size returns the 120*60px one, even though $size is array(208,60)
• Wrong image show in widget

I'm forced to use WP 3.4.2 (which means the plugin uses legacy media lib code) for now, but can reproduce the problem with trunk by calling wp_get_attachment_image directly.

@nacin Any suggestions? I think image_get_intermediate_size should either work with cropped images, or filter them out completely. In the first case, "area" is no longer a valid metric (a 400*100px and a 100*400px crop of the same 400*400px image have the same area).

[markoheijnen]

Moving to 3.8. I will look at it next week and try to write some unit tests for it.

[lkraav]

I guess the schedule didn't work out @markohejinen? I'm wondering if this is the bug I'm running up against on 3.8.1.

• crop thumbnails
• change thumbnail size in Media dialog, let's say 220x220 -> 480x480
• regenerate thumbnails
• go to image Editor + all front end queries still get old size (smaller) thumbnail

[fliespl]

This bug occurs when you switch from template A which is setting set_post_thumbnail_size to size X and to template B which is using other value for set_post_thumbnail_size(); (post-thumbnail)

image_get_intermediate_size then returns wrong value because data in wp_get_attachment_metadata is not being refreshed on template change - will return size X.

[ericlewis]

I've simplified the logic in attachment:17626.diff​.

In the first loop, if there's an exact match, short circuit. Otherwise, store any image sizes that are larger than the one requested in a hash. Loop back through this hash (sorted by area size), and return the first one.

[roytanck]

Replying to ericlewis:

I've simplified the logic in attachment:17626.diff​.

In the first loop, if there's an exact match, short circuit. Otherwise, store any image sizes that are larger than the one requested in a hash. Loop back through this hash (sorted by area size), and return the first one.

Looks promising. But I think $candidate_sizes[$data['width'] * $data['height']] = $_size; (line 589) could potentially cause issues with different crops where width*height is the same (i.e. 400px*100px and 100px*400px images).

That said, simply short circuiting on the exact requested dimensions would likely fix all the issues I've been having with this.

[kitchin]

The ! $size check is redundant (except for array(), a pathological case which would cause other errors). It's confusing and probably an artifact of past logic. Also would be good to initialize $candidate_sizes.

[kitchin]

Two-line mod to 17626.diff

[kitchin]

inter-diff < 17626.diff > 17626k.diff

[ericlewis]

attachment:17626k.diff​ has some good cleanup.

Still need unit tests here.

[Krstarica]

There is a special case when one of the image dimensions is zero.

So:

if ( $data['width'] >= $size[0] || $data['height'] >= $size[1] ) {

should be replaced with:

if ( ($data['width'] >= $size[0] && $size[0] != 0) || ($data['height'] >= $size[1] && $size[1] != 0 ) ) {

[joemcgill]

Refresh of 17626k.diff with unit tests

[joemcgill]

I recently ran across this bug on a project and noticed that there was a viable patch (though stale) that needed unit tests. In 17626.2.diff​ I've refreshed 17626k.diff​ and added unit tests so we might be able to close this one.

[Krstarica]

Joe, can you add special case as mentioned in #comment:28

[joemcgill]

Replying to Krstarica:

Joe, can you add special case as mentioned in #comment:28

Hi Krstarica,

Could you elaborate on what you expect the result to be when an array is passed containing a zero for either width or height? And would this be a change in behavior in the function in its current, pre patched, form?

Thanks.

[Krstarica]

Hi Joe,

When one of the image dimensions is 0, WP resizes image proportionally using non-zero dimension. This is the only way to generate correct thumbnail without using crop.

E.g.

• if we specify array(300,0) as a new size, the image with dimensions 600x400 should be scaled to 300x200.

• if we specify array(0,100), the image with dimensions 450x300 should be scaled to 150x100.

Sometimes this was working properly and sometimes it was picking the incorrect dimension. Lately I've found some images not having the specified dimension even with my 'patch', e.g. instead of array(300,0) it picks array(640,0).

Warm regards,
Ivan

[joemcgill]

added tests for arrays with zero heights or widths

[joemcgill]

Replying to Krstarica:

Thanks for the clarification.

In 17262.3.diff I've added new unit tests for the cases you bring up and they are currently passing with the changes to the function as is.

To explain how this is working: when you add a custom image size with a zero value like add_image_size('my_size, 300, 0), an intermediate size will be added to the attachment metadata for uploaded images with the height and width attributes that have been calculated after it was resized. For example, an image that is 600x400 will have an intermediate size with an array that looks like this:

["my_size"]
    array(4) {
      ["file"]=>
      string(19) "filename.jpg"
      ["width"]=>
      int(300)
      ["height"]=>
      int(200)
      ["mime-type"]=>
      string(10) "image/jpeg"
    }

So, calling image_get_intermediate_size() with a zero value will never find an exact match, but will instead need to find the closest crop, which it now seems to be doing correctly. If possible, I would suggest actually passing the name of the custom crop, like image_get_intermediate_size('my_size').

Let me know if this answers your question.

[Krstarica]

Many thanks Joe, your patch seems to be working fine.

[joemcgill]

Refresh so patch will apply after 31530

[wonderboymusic]

In 33807:

Improve the reliability of the crop returned by image_get_intermediate_size().

Add a bunch of unit tests to tests/image/intermediate_size.php.

Props joemcgill, ericlewis, kitchin, SergeyBiryukov, chipbennett.
Fixes #17626.

[netweb]

r33807 broke our PHP 5.2 tests ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/77878771

Fatal error: Call to undefined method Tests_Image_Intermediate_Size::assertNotFalse() in /home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/image/intermediate_size.php on line 101

A case of the PHPUnit 3.6.x branch used with PHP 5.2 not supporting assertNotFalse

[joemcgill]

Fixes tests for PHP 5.2

[wonderboymusic]

In 33846:

After [33807], fix unit tests for PHP 5.2

Fixes #17626.

[netweb]

Thanks Joe and Scott for the PHP 5.2 fix :)

[boonebgorges]

In 34775:

More explicit tests for image_get_intermediate_size().

After [33807], Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width()
was failing intermittently. This failure was due to the use of a random number
for the image height. When the height was sufficiently large - $height >= 202 -
to change the aspect ratio of the cropped image (based on WP's threshold of a
1px difference), the test passed. And when the height was exactly 200, the
medium image size was hit exactly. The failure occurred only with a height of
201, which is close enough to 200 so that WP determined that the aspect ratio
of the potential crop was close enough to match.

The current changeset splits the test into two, in order to demonstrate the
failure.

See #17626. Fixes #34087.