Make WordPress Core

Opened 3 years ago

Last modified 8 days ago

#17626 assigned defect (bug)

image_get_intermediate_size() may return wrong thumbnail size

Reported by: chipbennett Owned by: markoheijnen
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:


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

media.php.diff (1.2 KB) - added by chipbennett 3 years ago.
slight modification to image_get_intermediate_size()
media.php.sortimagedata.diff (1.6 KB) - added by chipbennett 3 years ago.
Replaced usort() with uasort(), per @scribu's recommendation
17626.patch (1.9 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (26)

chipbennett3 years ago

slight modification to image_get_intermediate_size()

comment:1 scribu3 years ago

  • Keywords needs-testing added

I've ran into this issue myself.

comment:2 chipbennett3 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.)

comment:3 scribu3 years ago

You can use uasort().

comment:4 chipbennett3 years ago

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

chipbennett3 years ago

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

comment:5 follow-up: scribu3 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.

SergeyBiryukov3 years ago

comment:6 in reply to: ↑ 5 SergeyBiryukov3 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.

comment:7 scribu3 years ago

Related: #18532

comment:9 chipbennett2 years ago

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

comment:10 follow-up: scribu2 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)?

comment:11 in reply to: ↑ 10 chipbennett2 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?

comment:12 scribu2 years ago

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

comment:13 SergeyBiryukov2 years ago

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

comment:14 nacin2 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.

comment:15 nacin2 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.

comment:17 carstenbach13 months ago

  • Cc carstenbach added

comment:18 roytanck11 months 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).

comment:19 lkraav8 months ago

  • Cc leho@… added

comment:20 markoheijnen6 months 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.

comment:21 markoheijnen5 months ago

  • Milestone changed from 3.8 to Future Release

comment:22 lkraav6 weeks 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

comment:23 fliespl8 days 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.

Note: See TracTickets for help on using tickets.