WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 2 weeks ago

#17262 new enhancement

wp_get_attachment_thumb_file should check new 'thumbnail' image size

Reported by: lonnylot Owned by:
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.0
Component: Media Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description (last modified by DrewAPicture)

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.

Attachments (6)

post.diff (302 bytes) - added by lonnylot 5 years ago.
17262.diff (597 bytes) - added by lonnylot 5 years ago.
wp-includes/post.php diff
17262.2.diff (1.0 KB) - added by wonderboymusic 3 years ago.
post.2.diff (726 bytes) - added by JoshuaAbenazer 3 years ago.
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
17262.3.diff (2.1 KB) - added by romulodl 3 weeks ago.
17262.4.diff (2.3 KB) - added by swissspidy 2 weeks ago.

Download all attachments as: .zip

Change History (29)

@lonnylot
5 years ago

#1 @scribu
5 years ago

  • Keywords has-patch removed

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

Last edited 5 years ago by scribu (previous) (diff)

@lonnylot
5 years ago

wp-includes/post.php diff

#2 @lonnylot
5 years ago

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

#3 @nacin
5 years ago

  • Version changed from 3.2 to 3.0

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

#4 @lonnylot
5 years ago

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

#5 @azaozz
5 years ago

@nacin is right, deprecate +1.

#6 @scribu
5 years ago

  • Keywords needs-patch 3.3-early added
  • Milestone changed from Awaiting Review to Future Release

#7 @tw2113
3 years ago

  • Cc michael.d.beckwith@… added

#8 @wonderboymusic
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.6

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.

#9 follow-up: @nacin
3 years ago

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

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#10 @SergeyBiryukov
3 years ago

#23556 was marked as a duplicate.

@JoshuaAbenazer
3 years ago

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

#11 @SergeyBiryukov
3 years ago

#10492 was marked as a duplicate.

#12 @DrewAPicture
3 years ago

  • Description modified (diff)

#13 @nacin
3 years ago

  • Keywords 3.7-early added; 3.3-early removed
  • Milestone changed from 3.6 to Future Release
  • Summary changed from wp_get_attachment_thumb_file is always false to wp_get_attachment_thumb_file should check new 'thumbnail' image size
  • Type changed from defect (bug) to enhancement

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

#14 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 3.7

#15 @wonderboymusic
3 years ago

these are all marked 3.7-early

#16 @nacin
3 years ago

  • Keywords needs-patch needs-unit-tests added; 3.7-early has-patch removed
  • Milestone changed from 3.7 to Future Release

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

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


2 years ago

#18 in reply to: ↑ description @jevuska
14 months ago

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

#19 @swissspidy
4 months ago

  • Keywords good-first-bug added

@romulodl
3 weeks ago

#20 @romulodl
3 weeks ago

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

#21 @romulodl
3 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@swissspidy
2 weeks ago

#22 @swissspidy
2 weeks ago

  • Milestone changed from Future Release to 4.6

The latest patch looks good.

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

#23 in reply to: ↑ 9 @swissspidy
2 weeks ago

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

Note: See TracTickets for help on using tickets.