Make WordPress Core

Opened 3 years ago

Closed 7 months ago

Last modified 7 months ago

#51780 closed enhancement (fixed)

Add ability to easily get path to sized images (such as parameter to get_attached_file() or wp_get_attachment_thumb_file())

Reported by: paulschreiber's profile paulschreiber Owned by: joedolson's profile joedolson
Milestone: 6.4 Priority: normal
Severity: normal Version: 5.5.3
Component: Media Keywords: 2nd-opinion
Focuses: Cc:

Description

Currently, you can call get_attached_file() and get the path to the full-sized version of the image.

However, if you want the path to a smaller variant of the image, you end up with something like this:

<?php
$imagedata = wp_get_attachment_metadata( $image_id );
$file_path = get_attached_file( $image_id );
$thumbnail_file = str_replace( wp_basename( $file_path ), $imagedata['sizes'][ $size ]['file'], $file_path );

Or like this:

<?php
$imagedata = image_get_intermediate_size( $image_id, $size );
$upload_directory = wp_get_upload_dir();
$thumbnail_file = $upload_directory['basedir'] . '/' . $imagedata['path'];

For brevity, these examples omit the null dereference checks, which triple the size of the code needed.

This is similar to the behaviour of wp_get_attachment_thumb_file(), but that function hard-codes thumb as the only size.

It would be very helpful if this existed:

<?php
get_attached_file( int $attachment_id, bool $unfiltered = false, string $size = false )

Or this existed:

<?php
wp_get_attachment_thumb_file( int $post_id, string $size = false )

See related tickets #33959 (from 2015) and #17262 (from 2011).

I'm happy to make a patch for either/both of these approaches, but don't know which one you prefer, and don't want this to languish for 11 years.

Attachments (2)

51780.1.patch (3.2 KB) - added by joedolson 14 months ago.
Fixes $size param docs default value.
51780.filter.patch (1.8 KB) - added by joedolson 14 months ago.
Passes $size to filter

Download all attachments as: .zip

Change History (39)

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


17 months ago

#2 @antpb
17 months ago

  • Milestone changed from Awaiting Review to 6.2

Moving this to 6.2 so we can investigate a new function for paths that meets the intent of the suggestions of this ticket.

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


14 months ago

#4 @audrasjb
14 months ago

  • Keywords needs-patch added

FWIW, and maybe I’m wrong, but the first proposal looks better to me as it is less opinionated (the second proposal contains thumb which can refer to the thumbnail size).

This ticket was mentioned in PR #3939 on WordPress/wordpress-develop by Mahjouba91.


14 months ago
#5

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

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

Currently, you can call get_attached_file() and get the path to the full-sized version of the image.
However, if you can't directly get the path to a smaller variant of the image, making further code implementation prone to error and not efficient.

#6 @Mista-Flo
14 months ago

Hey folks,

I came up with a patch for this ticket, I think it definitely makes sense to extends get_attached_file function with a size parameter, furthermore it doesn't affect any current usage of the function as the parameter will remain an empty string by default.

I also added a quick unit test to check the function works as expected, not sure if it needs further unit testing?

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


14 months ago

#8 @antpb
14 months ago

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

#9 @joedolson
14 months ago

  • Status changed from assigned to accepted

@joedolson
14 months ago

Fixes $size param docs default value.

#10 @joedolson
14 months ago

  • Keywords commit added

PR looks good; updated the patch to correct the default value on the $size param in the documentation block.

This ticket was mentioned in PR #3972 on WordPress/wordpress-develop by @joedolson.


14 months ago
#11

Adds argument to get paths for intermediate sizes.

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

#12 @joedolson
14 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55199:

Media: Add argument to get_attached_file() for subsizes.

Add a $size argument to get_attached_file() to simplify getting the path to an intermediate image size.

Props paulschreiber, audrasjb, Mista-Flo.
Fixes #51780.

@joedolson commented on PR #3939:


14 months ago
#13

Fixed in r55199

@joedolson commented on PR #3972:


14 months ago
#14

Fixed in r55199

#15 @SergeyBiryukov
14 months ago

In 55202:

Docs: Improve the DocBlock for get_attached_file().

Includes:

  • Using 3-digit format for the @since tag.
  • Minor wording updates and formatting corrections.

Follow-up to [3203], [4612], [6379], [8203], [24983], [29090], [55199].

See #51780, #56792.

#16 follow-up: @SergeyBiryukov
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the commit!

Should the $size parameter be passed to the get_attached_file filter?

#17 in reply to: ↑ 16 @Mista-Flo
14 months ago

Replying to SergeyBiryukov:

Thanks for the commit!

Should the $size parameter be passed to the get_attached_file filter?

Indeed I missed that, I think it will be a nice addition if plugins need to do some actions on that filter.

#18 @joedolson
14 months ago

I agree; that makes a lot of sense. I think it would be confusing if it didn't.

@joedolson
14 months ago

Passes $size to filter

This ticket was mentioned in PR #3982 on WordPress/wordpress-develop by @joedolson.


14 months ago
#19

Pass $size to filter.

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

#20 @joedolson
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55217:

Media: Pass $size argument to get_attached_file filter.

Pass the new $size argument on get_attached_file() to get_attached_file filter. Follow up to [55199].

Props SergeyBiryukov, joedolson.
Fixes #51780.

@joedolson commented on PR #3982:


14 months ago
#21

Fixed in r55217

#22 @azaozz
13 months ago

  • Keywords 2nd-opinion added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Not sure this is a good addition to get_attached_file(). This function should get the attached file, i.e. a PDF, a video, a document, etc. This function is not intended only for images, what is the point to extend it only for image sub-sizes? That seems confusing and contrary to the initial design decision :(

Also, looking at the description, there is no reason why this is needed. If there is really a good reason for such function, it should probably be a new function.

However I don't see a reason why plugin authors cannot use image_get_intermediate_size() for this. That is probably currently being used. Getting the path to an image sub-size seems like something that's rarely needed anyway.

Another serious objection here is that this patch slows down get_attached_file() as it always does another DB request looking for image meta, no matter what file is attached.

In my opinion this patch should be reverted and moved to 6.3 or future release allowing for a better review whether this is really needed, and for better patch (new function) that doesn't reduce performance of get_attached_file()`.

Last edited 13 months ago by azaozz (previous) (diff)

#23 follow-up: @joedolson
13 months ago

I'm not seeing how this impacts the performance of get_attached_file() by default; it only performs the additional request if the $size param isn't empty. Can you clarify that?

#24 in reply to: ↑ 23 @azaozz
13 months ago

Replying to joedolson:

it only performs the additional request if the $size param isn't empty

Hmm, true, but that's the expected use?

In any case, why a function that works for all attachments should be extended in a way that doesn't work for all?

Last edited 13 months ago by azaozz (previous) (diff)

#25 follow-up: @joedolson
13 months ago

The logic, to me, is that images are the only file type that has managed variants, and it makes sense to use the same function to fetch variants of a file as you would use to fetch the attachment. The function is to get a file, not to get an attachment, so it makes sense that you could use it to fetch any file managed in the media library.

I could imagine it being extended to fetch alternate versions of audio/video files, too (e.g., different formats or different resolutions), though that isn't currently something managed by the library.

#26 in reply to: ↑ 25 @azaozz
13 months ago

  • Keywords changes-requested added

Replying to joedolson:

it makes sense to use the same function to fetch variants of a file

The functionality of get_attached_file() is to "get the attached file", i.e. to get the path to the file that was uploaded by the user and is now "attached" to an attachment post.

The function is to get a file, not to get an attachment

Exactly, it is to get the path to the file attached to a particular attachment post.

fetch any file managed in the media library

I think I understand where you're coming from but don't think it is a good idea to extend a function that has to perform one simple task for all uploaded files, and make it work differently for different file types. In addition I don't think this is a good functionality to have in core anyway. It is not used anywhere in core, and there hasn't been a need for it for so many years.

On top of that the ticket description mentions that an existing function can be used for this, but doesn't mention why the proposed functionality would be useful. My guess is it would be a very minor convenience for maybe a few plugins that may meed the path to an image sub-size. Not sure this is a good reason to add this to core.

Last edited 13 months ago by azaozz (previous) (diff)

#27 follow-up: @paulschreiber
13 months ago

On top of that the ticket description mentions that an existing function can be used for this, but doesn't mention why the proposed functionality would be useful. My guess is it would be a very minor convenience for maybe a few plugins that may meed the path to an image sub-size. Not sure this is a good reason to add this to core.

I gave context in the original ticket. Right now, everyone has to write 3 lines of code + null checks (so ≥12 lines). This greatly increases the possibility of error. Having core do this for you is very helpful, and core is more likely to get it right than random theme/plugin authors.

You can see the theme where this was used:
https://github.com/CTCL/election-websites/blob/e5fc55dd651fb73d13114262b2dd60e8a095a91c/includes/class-helpers.php#L321-L356

Much of this was required because WPCOM behaves differently than everywhere else, which I detail in the comments in that source code.

#28 @flixos90
13 months ago

Apologies for chiming in late here, just came across this ticket. I agree with @azaozz that the idea of adding a $size parameter to this function conflicts with its purpose. This is a general attachment function, not a function for attachment images (or image attachments, whichever you wanna call it).

We have numerous functions specific to image attachments in core already, all of which (rightfully!) allow passing the $size parameter. But this here is not one of them. From the ticket description, I would think the wp_get_attachment_image_url() function is what should be used for this? But even if I'm missing something and maybe a function for something is indeed missing in core, we should rather add or modify an image attachment specific function, not this general one.

I think this changeset should be reverted. We can separately continue discussing for 6.3 whether the use-case here indeed requires a change, and which image function it would best fit into.

#29 @joedolson
13 months ago

wp_get_attachment_image_url() wouldn't do it, since it fetches URLs, rather than paths.

However, no problem with reverting this. I won't have time for it right away, so if anybody gets to it sooner, go right ahead.

#30 @flixos90
13 months ago

@joedolson Ah I see, this is about paths, that's what I had missed. :)

In that case I would rather suggest we introduce a new function for that purpose, e.g. wp_get_attachment_image_path( $attachment_id, $size ). That may be a useful enhancement for 6.3, and that way we build the image specific functionality into a new image specific function.

#31 in reply to: ↑ 27 @azaozz
13 months ago

Replying to paulschreiber:

Much of this was required because WPCOM behaves differently than everywhere else

Yes, the media on WP.com is handled in a quite different way. For example, afaik, there is no sizes array in the image meta, and image sub-sizes are created "on the fly" when needed and then cached for some time. If a convenience function for WP.com is needed, perhaps a better option would be to request it there.

Looking at the example use, it is for outputting a thumbnail image as data URL. Not sure if this is a common case to the extend that it would need a separate "convenience function", i.e. how many plugins would need that. But lets try to figure this out in 6.3.

#32 @azaozz
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55437:

Media: Revert the addition of a $size parameter to get_attached_file().

Reverts [55199], [55202], and [55217] but keeps the updated docs.

Props: flixos90, joedolson, azaozz.
Fixes: #51780.

#33 @flixos90
13 months ago

  • Keywords has-patch has-unit-tests changes-requested removed
  • Milestone changed from 6.2 to 6.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

After the revert, reopening this for continued discussion on whether a new image function should be introduced or an existing image function modified to support getting the full file path to a sub-sized image file.

#34 @oglekler
9 months ago

  • Milestone changed from 6.3 to 6.4

This is an enhancement, and we are in 12 days until Beta 1 after which we will not add new enhancement to the release, so, because there is no patch, I am moving this ticket to 6.4.

As far as I understand, the question now is if we need a new wp_get_attachment_image_path( $attachment_id, $size ) function or not, please continue discussion and look for use cases. From my point of view it is better to check that the file exists before using it for the frontend otherwise you can end with broken images, and it isn't looks nice. For example, new image size was added and used at some place but old images didn't regenerate. So, checking if image exists, we can use different size and avoid broken images to appear.

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


7 months ago

#36 @oglekler
7 months ago

This ticket was discussed during a bug scrub.

This ticket needs an analysis when image_get_intermediate_size() cannot fit the demand and a new function is needed.

Because this is an enhancement, please keep in mind that there is limited time to make it into 6.4 if some patch is needed.

Add props too: @hellofromTonya @vipuljnext

#37 @azaozz
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

As mentioned earlier I think the whole premise for this ticket is somewhat incorrect. The first line in the description says that:

Currently, you can call get_attached_file() and get the path to the full-sized version of the image.

Technically that is not exactly true. You get the the path to the attached file which may be an image.

I'm also not convinced that a separate helper function is needed to get paths to sub-size images. Currently this is easily done without a helper by using image_get_intermediate_size(). Even the name of that function suggests its usage.

After the revert, reopening this for continued discussion..

There hasn't been new info or opinions for six months. Re-closing this as fixed. Please feel free to reopen with more details why such helper function might be useful (although it seems a new enhancement ticket may be a better choice).

Last edited 7 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.