#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 | Owned by: | 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)
Change History (39)
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
21 months ago
#4
@
21 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.
21 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
@
21 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.
21 months ago
#10
@
20 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.
20 months ago
#11
Adds argument to get paths for intermediate sizes.
Trac ticket: https://core.trac.wordpress.org/ticket/51780
@joedolson commented on PR #3939:
20 months ago
#13
Fixed in r55199
@joedolson commented on PR #3972:
20 months ago
#14
Fixed in r55199
#16
follow-up:
↓ 17
@
20 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
@
20 months ago
Replying to SergeyBiryukov:
Thanks for the commit!
Should the
$size
parameter be passed to theget_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.
This ticket was mentioned in PR #3982 on WordPress/wordpress-develop by @joedolson.
20 months ago
#19
Pass $size to filter.
Trac ticket: https://core.trac.wordpress.org/ticket/51780
@joedolson commented on PR #3982:
20 months ago
#21
Fixed in r55217
#22
@
20 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()`.
#23
follow-up:
↓ 24
@
20 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
@
20 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?
#25
follow-up:
↓ 26
@
20 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
@
20 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.
#27
follow-up:
↓ 31
@
20 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
@
20 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
@
20 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
@
20 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
@
20 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.
#33
@
20 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
@
16 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.
14 months ago
#36
@
14 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
@
14 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).
Moving this to 6.2 so we can investigate a new function for paths that meets the intent of the suggestions of this ticket.