wp_get_attachment_thumb_file should check new 'thumbnail' image size

[lonnylot]

The issue is that on line 3863 we always search for $imagedata['thumb'] and it never exists. Instead we have $imagedata['thumbnail']. This always exists.

[scribu]

Please submit patches as svn diffs. Tutorials can be found on the ​trac homepage.

[lonnylot]

wp-includes/post.php diff

[lonnylot]

Sorry for the delay! Added the correct diff off of trunk rev. 18191

[nacin]

So does wp_get_attachment_thumb_file() ever work? That function should probably be deprecated in favor of the various other functions now.

[lonnylot]

As far as I can tell not unless you add a custom image type called 'thumb'

[azaozz]

@nacin is right, deprecate +1.

[wonderboymusic]

17262.2.diff​ updates wp_get_attachment_thumb_file() so it works in trunk. The previous patch did not have current code in mind. It's not immediately obvious to me what wp_get_attachment_thumb_file() and wp_get_attachment_thumb_url() would be deprecated in favor of.

[nacin]

'thumb' is the old-style thumbnail, prior to proper intermediate sizes ('thumbnail' / 'medium' / 'large').

wp_get_attachment_thumb_url() does return the newer 'thumbnail' if it exists, and falls back to wp_get_attachment_thumb_file() if it does not.

You can see a bit of code history in image_downsize(): "fall back to the old thumbnail" in a code comment.

Not sure what _file() should return in a deprecated situation. _url() is basically the old-school form of wp_get_attachment_image_src( $size = 'thumbnail' ), which returns array( $src, $width, $height ). Which is kind of lame.

So, perhaps they can both be updated to work on 'thumbnail', and then left, as wp_get_attachment_thumb_url() is definitely easier, nicer, and more obvious to use than wp_get_attachment_image_src()['url'], and according to MarkJaquith, is widely used by plugins (probably for that reason).

[SergeyBiryukov]

#23556 was marked as a duplicate.

[JoshuaAbenazer]

This one first checks for the "fall back to the old thumbnail" and if that returns false then goes ahead and checks according to the revised array

[SergeyBiryukov]

#10492 was marked as a duplicate.

[nacin]

Let's clean this up and get it going for 3.7.

[wonderboymusic]

these are all marked 3.7-early

[nacin]

post.2.diff​ is still a bit messy, and could use unit tests.

[jevuska]

Replying to lonnylot:

The issue is that on line 3863 we always search for $imagedata['thumb'] and it never exists. Instead we have $imagedata['thumbnail']. This always exists.

Nothing change in 4.2 beta. Just found this solving at ​https://wordpress.org/support/topic/fix-for-bug-in-wp_get_attachment_thumb_file-1

[romulodl]

Just added a fix for this ticket and added a test for it as well.

Please let me know if it is all right.

Thanks,
Romulo De Lazzari

[swissspidy]

The latest patch looks good.

17262.4.diff​ just adds some needed spacing and uses a third-person singular verb in the docbocks.

[swissspidy]

Replying to nacin:

'thumb' is the old-style thumbnail, prior to proper intermediate sizes ('thumbnail' / 'medium' / 'large').

wp_get_attachment_thumb_url() does return the newer 'thumbnail' if it exists, and falls back to wp_get_attachment_thumb_file() if it does not.

You can see a bit of code history in image_downsize(): "fall back to the old thumbnail" in a code comment.

Not sure what _file() should return in a deprecated situation. _url() is basically the old-school form of wp_get_attachment_image_src( $size = 'thumbnail' ), which returns array( $src, $width, $height ). Which is kind of lame.

So, perhaps they can both be updated to work on 'thumbnail', and then left, as wp_get_attachment_thumb_url() is definitely easier, nicer, and more obvious to use than wp_get_attachment_image_src()['url'], and according to MarkJaquith, is widely used by plugins (probably for that reason).

wp_get_attachment_image_url() is a simple wrapper around wp_get_attachment_image_src()['url'] and available since 4.4.

I'd be in favour of deprecating wp_get_attachment_thumb_url() in favour of that function. It's currently nowhere used in core except for get_attachment_icon_src(), which is deprecated.

After that, we could also think about deprecating wp_get_attachment_thumb_file(), but I guess we can leave it for now as it's currently used in image_downsize() .

[joemcgill]

After spending some more time thinking about this, I don't believe we should make the change suggested in this ticket at this point.

My reasoning is that each time wp_get_attachment_thumb_file() is used in core, it's sole purpose is to fallback to the old-style thumb size whenever thumbnail is requested but not found (see image_downsize() and wp_get_attachment_thumb_url()).

I do like the idea of deprecating wp_get_attachment_thumb_url() as @swissspidy suggests above. One option for deprecating wp_get_attachment_thumb_file() would be to add a new function like wp_get_attachment_image_file(), which would return the server file path of an image when passed an attachment's $post_id and $size. However, I can't think of a place where we would make use of such a function in Core other than where wp_get_attachment_thumb_file() is working just fine.

[swissspidy]

The decision in the last bug scrub in #core-images was to deprecate wp_get_attachment_thumb_file() and create a replacement function that handles both new and old data. wp_get_attachment_image_file() sounds good.

wp_get_attachment_thumb_url() can be deprecated in favour of wp_get_attachment_image_url(), which already handles the old-style thumbnail thanks to image_downsize().

I would really appreciate unit tests and some improved docs, but I don't see that happening for 4.6.

[mfgmicha]

My first patch for WordPress :-) Have not been tested yet. Quick review done by @SergeyBiryukov

[mashukushibiki]

Hi, @mfgmicha

$ is missing on line 962 at Attachment 17262.5.diff.

- $file = wp_get_attachment_metadata( attachment_id );

+ $file = wp_get_attachment_metadata( $attachment_id );

[mashukushibiki]

I modified my previous attachment. I also have not tested this code yet.

[mirekgab]

Hi

I am trying to write a unit test for a function

function wp_get_attachment_image_file( $attachment_id, $size = 'thumbnail' )

and I need a hint about the parameter $attachment_id. Should I use the variable self::$large_id available in class Tests_Media (file tests/phpunit/tests/media.php) or create an object like this:

$post_id       = self::factory()->post->create();
$attachment_id = self::factory()->attachment->create_object(
	$this->img_name, $post_id, array(
		'post_mime_type' => 'image/jpeg',
		'post_type'      => 'attachment',
	)
);

and use created $attachment_id, or maybe use something else ?

[joemcgill]

17262.5.diff​ looks the closest at present.

A few thoughts:

• The default size should probably be 'full', not 'thumbnail'
• We still need to deprecate and replace wp_get_attachment_thumb_file()
• Still needs tests.

I don't see any of this being a priority for 5.0, but am happy to ship it whenever the functionality is ready.

[lonnylot]

Hi all, I appreciate that this is still being looked at so many years after I created the ticket, but I've moved on. Is there any way to remove myself from the notifications on this ticket?

[peterwilsoncc]

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

Replying to lonnylot:

Is there any way to remove myself from the notifications on this ticket?

You can do so in the notifications box on the ticket.

[killua99]

Patch updated a bit change, and with Unit Test

[killua99]

Wrong PHP DocBlock

[killua99]

Interdiff betweek path 8 and 9 (helps to keep tracks of the progress and evolution of the patch)

[killua99]

Proposal 9, that include performance into looking for the upload path folder, skipping creation. Adding deprecation message to wp_get_attachment_thumb_file since this ticket mention about deprecate it.

[killua99]

Sorry, I didn't update the branch against trunk / master. Now is clean and updated to the latest trunk state.

[killua99]

Inter diff to track progress of patch

[killua99]

Adding filter to the new function and moving deprecated function to the right file.

[SergeyBiryukov]

Missed the 5.2 Beta 1 deadline, moving to 5.3.

[kirasong]

Just to make the status here clear, from @SergeyBiryukov in the last mention in Slack above:
"The patch deprecates wp_get_attachment_thumb_file() and introduces a new function, though core still uses the old function in a few places. Needs some more work to replace all the instances."

If this happens before the beta on Monday, it can still be committed.
Otherwise, it'll need to get moved from the 5.3 milestone.

[kirasong]

Because there's still work remaining here, and the beta is tomorrow, I'm moving this ticket out of the 5.3 milestone.

If there's a committable patch before the beta deadline, please feel free to move it back in!

[SergeyBiryukov]

#54383 was marked as a duplicate.

[SergeyBiryukov]

Related: #33959

[markhowellsmead]

The function wp_get_registered_image_subsizes indicates that the actual size key in the array is thumbnail. The default value of the size parameter in get_the_post_thumbnail is post-thumbnail. It might be worth considering reviewing this conflict.

The specific issue which has caused me to provide feedback to this ticket is render_block_core_post_featured_image in the new wp-includes/blocks/post-featured-image.php file. If the issue with get_the_post_thumbnail can't be resolved, it may be a simpler solution (for this case) to modify the line…

$featured_image = get_the_post_thumbnail( $post_ID );

…to…

$featured_image = get_the_post_thumbnail( $post_ID, 'thumbnail' );

[markhowellsmead]

I’ve submitted a pull request to Gutenberg which works around this issue, specifically in the post-featured-image block renderer. ​https://github.com/WordPress/gutenberg/pull/36393

[csesumonpro]

Patch created

[mukesh27]

Hi there!

@markhowellsmead The Gutenberg block added the image size attribute in the function get_the_post_thumbnail(). Check ​here.

@SergeyBiryukov Is this ticket is in your to-do list for the upcoming release? Is there any patch stand with the current trunk version?

Remove Performance focus.

[azaozz]

Looking at the history of this ticket (all 11 years of it!), thinking @nacin's comment from [2013 https://core.trac.wordpress.org/ticket/17262#comment:9] and @joemcgill's comment from [2016 https://core.trac.wordpress.org/ticket/17262#comment:28] still make sense. In short:

1. Deprecate wp_get_attachment_thumb_file().
2. Make wp_get_attachment_thumb_url() a direct alias of wp_get_attachment_image_url().
3. (TBD) Perhaps add some code to look for $image_meta['thumb'] when $image_meta['sizes'] is not set (only when not set, can be empty for other reasons).

The consideration for the TBD is whether it's worth it to still support this format of the image meta. As far as I know it has existed only for a short time about 15 years ago. Probably worth it but needs some "track archeology" to confirm :)

Not sure it is a good idea to add a new function to specifically get the path to an image's thumbnail. Don't see the need for it (see mentioned comments).

[markhowellsmead]

@mukesh27 Please refer to https://core.trac.wordpress.org/ticket/17262#comment:55 which explains how the conflict between "post-thumbnail" and "thumbnail" size keywords leads to the output of the wrong image, thereby causing performance issues on any site using get_the_post_thumbnail.

• Deprecate wp_get_attachment_thumb_file().
• Make wp_get_attachment_thumb_url() an alias of wp_get_attachment_image_url().
• Add back-compat to image_downside() to look for $image_meta['thumb'] when $image_meta['sizes']['thumbnail'] is missing.

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

[azaozz]

Fixed in 53685

Media:

• Deprecate wp_get_attachment_thumb_file().
• Make wp_get_attachment_thumb_url() an alias of wp_get_attachment_image_url(). This fixes it to return the proper thumbnail URL and fall back to returning the URL to $image_meta['thumb'] if only that exists.

Props: markhowellsmead, mukesh27, csesumonpro, SergeyBiryukov, mikeschroder, killua99, joemcgill, mashukushibiki, mfgmicha, swissspidy, romulodl, nacin, JoshuaAbenazer, wonderboymusic, lonnylot, azaozz.