Make WordPress Core

Opened 14 years ago

Closed 9 years ago

Last modified 8 years ago

#17626 closed defect (bug) (fixed)

image_get_intermediate_size() may return wrong thumbnail size

Reported by: chipbennett's profile chipbennett Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: Cc:

Description

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

Attachments (10)

media.php.diff (1.2 KB) - added by chipbennett 14 years ago.
slight modification to image_get_intermediate_size()
media.php.sortimagedata.diff (1.6 KB) - added by chipbennett 14 years ago.
Replaced usort() with uasort(), per @scribu's recommendation
17626.patch (1.9 KB) - added by SergeyBiryukov 13 years ago.
17626.diff (3.5 KB) - added by ericlewis 11 years ago.
17626k.diff (3.6 KB) - added by kitchin 11 years ago.
Two-line mod to 17626.diff
inter.diff (484 bytes) - added by kitchin 11 years ago.
inter-diff < 17626.diff > 17626k.diff
17626.2.diff (8.9 KB) - added by joemcgill 10 years ago.
Refresh of 17626k.diff with unit tests
17626.3.diff (11.0 KB) - added by joemcgill 10 years ago.
added tests for arrays with zero heights or widths
17626.4.diff (10.5 KB) - added by joemcgill 10 years ago.
Refresh so patch will apply after 31530
17626.5.diff (2.2 KB) - added by joemcgill 9 years ago.
Fixes tests for PHP 5.2

Download all attachments as: .zip

Change History (53)

@chipbennett
14 years ago

slight modification to image_get_intermediate_size()

#1 @scribu
14 years ago

  • Keywords needs-testing added

I've ran into this issue myself.

#2 @chipbennett
14 years ago

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

#3 @scribu
14 years ago

You can use uasort().

#4 @chipbennett
14 years ago

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

@chipbennett
14 years ago

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

#5 follow-up: @scribu
14 years ago

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.

#6 in reply to: ↑ 5 @SergeyBiryukov
13 years ago

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.

#7 @scribu
13 years ago

Related: #18532

#9 @chipbennett
13 years ago

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

#10 follow-up: @scribu
13 years ago

  • Milestone changed from Awaiting Review to 3.4

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

#11 in reply to: ↑ 10 @chipbennett
13 years ago

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?

#12 @scribu
13 years ago

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

#13 @SergeyBiryukov
13 years ago

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

#14 @nacin
13 years ago

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

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.

#15 @nacin
13 years ago

  • Milestone changed from 3.4 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

[20696] was meant for #20566.

Re-opening, and also punting.

#17 @carstenbach
12 years ago

  • Cc carstenbach added

#18 @roytanck
11 years ago

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

#19 @lkraav
11 years ago

  • Cc leho@… added

#20 @markoheijnen
11 years ago

  • Milestone changed from Future Release to 3.8
  • Owner changed from nacin to markoheijnen
  • Status changed from reopened to assigned

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

#21 @markoheijnen
11 years ago

  • Milestone changed from 3.8 to Future Release

#22 @lkraav
11 years ago

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

#23 @fliespl
11 years ago

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
11 years ago

#24 follow-up: @ericlewis
11 years ago

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

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.

#25 in reply to: ↑ 24 @roytanck
11 years ago

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.

#26 @kitchin
11 years ago

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
11 years ago

Two-line mod to 17626.diff

@kitchin
11 years ago

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

#27 @ericlewis
11 years ago

attachment:17626k.diff has some good cleanup.

Still need unit tests here.

#28 @Krstarica
10 years ago

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
10 years ago

Refresh of 17626k.diff with unit tests

#29 @joemcgill
10 years ago

  • Keywords needs-unit-tests removed

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.

Last edited 10 years ago by joemcgill (previous) (diff)

#30 follow-up: @Krstarica
10 years ago

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

#31 in reply to: ↑ 30 @joemcgill
10 years ago

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.

#32 follow-up: @Krstarica
10 years ago

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
10 years ago

added tests for arrays with zero heights or widths

#33 in reply to: ↑ 32 @joemcgill
10 years ago

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.

Last edited 10 years ago by joemcgill (previous) (diff)

#34 @Krstarica
10 years ago

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

#35 @joemcgill
10 years ago

  • Keywords needs-refresh added

@joemcgill
10 years ago

Refresh so patch will apply after 31530

#36 @joemcgill
10 years ago

  • Keywords needs-refresh removed

#37 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4
  • Owner changed from markoheijnen to wonderboymusic

#38 @wonderboymusic
9 years ago

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

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.

#39 @netweb
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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
9 years ago

Fixes tests for PHP 5.2

#40 @wonderboymusic
9 years ago

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

In 33846:

After [33807], fix unit tests for PHP 5.2

Fixes #17626.

#41 @netweb
9 years ago

Thanks Joe and Scott for the PHP 5.2 fix :)

#42 @boonebgorges
9 years ago

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.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.