WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 3 months ago

#17262 reviewing enhancement

wp_get_attachment_thumb_file should check new 'thumbnail' image size

Reported by: lonnylot Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Media Keywords: good-first-bug needs-patch
Focuses: Cc:
PR Number:

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

post.diff (302 bytes) - added by lonnylot 9 years ago.
17262.diff (597 bytes) - added by lonnylot 9 years ago.
wp-includes/post.php diff
17262.2.diff (1.0 KB) - added by wonderboymusic 7 years ago.
post.2.diff (726 bytes) - added by JoshuaAbenazer 7 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 4 years ago.
17262.4.diff (2.3 KB) - added by swissspidy 4 years ago.
17262.5.diff (1.4 KB) - added by mfgmicha 20 months ago.
My first patch for WordPress :-) Have not been tested yet. Quick review done by @SergeyBiryukov
17262.6.diff (134 bytes) - added by mashukushibiki 19 months ago.
17262.7.diff (1.6 KB) - added by mashukushibiki 19 months ago.
I modified my previous attachment. I also have not tested this code yet.
17262.7.2.diff (3.5 KB) - added by killua99 9 months ago.
Patch updated a bit change, and with Unit Test
17262.8.diff (3.5 KB) - added by killua99 9 months ago.
Wrong PHP DocBlock
17263.interdiff-8-9.txt (996 bytes) - added by killua99 9 months ago.
Interdiff betweek path 8 and 9 (helps to keep tracks of the progress and evolution of the patch)
17262.9.diff (22.2 KB) - added by killua99 9 months ago.
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.
17262.9.2.diff (4.0 KB) - added by killua99 9 months ago.
Sorry, I didn't update the branch against trunk / master. Now is clean and updated to the latest trunk state.
17262.inter-diff-9-10.diff (4.0 KB) - added by killua99 9 months ago.
Inter diff to track progress of patch
17262.10.diff (6.3 KB) - added by killua99 9 months ago.
Adding filter to the new function and moving deprecated function to the right file.

Download all attachments as: .zip

Change History (67)

@lonnylot
9 years ago

#1 @scribu
9 years ago

  • Keywords has-patch removed

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

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

@lonnylot
9 years ago

wp-includes/post.php diff

#2 @lonnylot
9 years ago

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

#3 @nacin
8 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
8 years ago

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

#5 @azaozz
8 years ago

@nacin is right, deprecate +1.

#6 @scribu
8 years ago

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

#7 @tw2113
7 years ago

  • Cc michael.d.beckwith@… added

#8 @wonderboymusic
7 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
7 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 7 years ago by SergeyBiryukov (previous) (diff)

#10 @SergeyBiryukov
7 years ago

#23556 was marked as a duplicate.

@JoshuaAbenazer
7 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
7 years ago

#10492 was marked as a duplicate.

#12 @DrewAPicture
7 years ago

  • Description modified (diff)

#13 @nacin
6 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
6 years ago

  • Milestone changed from Future Release to 3.7

#15 @wonderboymusic
6 years ago

these are all marked 3.7-early

#16 @nacin
6 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.


6 years ago

#18 in reply to: ↑ description @jevuska
5 years 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 years ago

  • Keywords good-first-bug added

@romulodl
4 years ago

#20 @romulodl
4 years 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
4 years ago

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

@swissspidy
4 years ago

#22 @swissspidy
4 years 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
4 years 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() .

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


4 years ago

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


4 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


4 years ago

#27 @chriscct7
4 years ago

  • Owner set to joemcgill
  • Status changed from new to assigned

#28 @joemcgill
4 years ago

  • Keywords close added

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.

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


4 years ago

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


4 years ago

#31 @swissspidy
3 years ago

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

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
20 months ago

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

#32 @SergeyBiryukov
20 months ago

  • Owner changed from joemcgill to SergeyBiryukov
  • Status changed from assigned to reviewing

#33 @SergeyBiryukov
20 months ago

  • Milestone changed from Future Release to 5.0

#34 @mfgmicha
20 months ago

  • Keywords has-patch added; needs-patch removed

#35 @mashukushibiki
19 months ago

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

Last edited 19 months ago by mashukushibiki (previous) (diff)

@mashukushibiki
19 months ago

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

#36 @mirekgab
17 months ago

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 ?

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


16 months ago

#38 @joemcgill
16 months ago

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.

#39 follow-up: @lonnylot
16 months ago

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?

#40 in reply to: ↑ 39 @peterwilsoncc
14 months ago

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.

#41 @peterwilsoncc
14 months ago

  • Milestone changed from 5.0 to 5.1

#42 @pento
11 months ago

  • Milestone changed from 5.1 to 5.2

@killua99
9 months ago

Patch updated a bit change, and with Unit Test

@killua99
9 months ago

Wrong PHP DocBlock

@killua99
9 months ago

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

@killua99
9 months ago

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

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

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


9 months ago

@killua99
9 months ago

Inter diff to track progress of patch

@killua99
9 months ago

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

This ticket was mentioned in Slack in #core by killua99. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


9 months ago

#46 @SergeyBiryukov
9 months ago

  • Milestone changed from 5.2 to 5.3

Missed the 5.2 Beta 1 deadline, moving to 5.3.

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 months ago

#50 @mikeschroder
3 months ago

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

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.

#51 @mikeschroder
3 months ago

  • Milestone changed from 5.3 to Future Release

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!

Note: See TracTickets for help on using tickets.