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.