Make WordPress Core

Opened 13 years ago

Last modified 6 years ago

#20205 new enhancement

Add the ability to filter wp_get_attachment_image()

Reported by: helgatheviking's profile helgatheviking Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.4
Component: Media Keywords: needs-patch
Focuses: Cc:

Description

i'm working on a plugin that lets users define any image to serve as the thumbnail for any PDF document. I'm storing the ID of the image as post_meta for the PDF attachment. Instead of showing the default icon with wp_get_attachment_image() I'm quite close to being able to do this by filtering image_downsize.

the trouble is that the image_downsize filter it run after

if ( !wp_attachment_is_image($id) )
		return false;

so the PDF image fails the test and I never have a chance to run my filter.

Attachments (9)

post.php.patch (614 bytes) - added by helgatheviking 13 years ago.
add argument to icon_dir filter
media.php (50.6 KB) - added by helgatheviking 13 years ago.
add argument to icon_dir filter
media.php.patch (642 bytes) - added by jfarthing84 13 years ago.
Add a filter to wp_get_attachment_image_src() within wp_get_attachment_image().
20205.patch (347 bytes) - added by SergeyBiryukov 12 years ago.
media.php.2.patch (1.6 KB) - added by doublesharp 12 years ago.
wp_get_attachment_image_src filter in wp_get_attachment_image_src() function
20205.2.patch (349 bytes) - added by jfarthing84 12 years ago.
Refreshing 20205.patch from trunk.
20205.3.patch (1.3 KB) - added by jfarthing84 12 years ago.
Filter wp_attachment_image_src instead.
20205.diff (2.6 KB) - added by ericlewis 11 years ago.
20205.1.diff (2.7 KB) - added by ericlewis 11 years ago.

Download all attachments as: .zip

Change History (47)

#1 @helgatheviking
13 years ago

sorry that should read PDF attachment and not PDF image

#2 @scribu
13 years ago

  • Component changed from General to Media

Moving the filter below the wp_attachment_is_image() check might have back-compat implications.

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

#3 @helgatheviking
13 years ago

ok, well then what about adding a filter to the wp_attachment_is_image check?

if ( !(apply_filters ( 'wp_attachment_is_image' , wp_attachment_is_image($id), $id, $size)))
	return false;

should provide back compatibility while still allowing it to be overridden.

#4 @jeremyfelt
13 years ago

Would the wp_mime_type_icon filter work? Seems that it could run in this scenario after image_downsize returns false.

#5 @helgatheviking
13 years ago

i looked at the wp_mime_type_icon filter and it almost works. you can send back a custom URL to the wp_mime_type_icon filter, but the URL gets truncated down to simply the filename by wp_basename($src) in the wp_get_attachment_image_src() function.

if ( $icon && $src = wp_mime_type_icon($attachment_id) ) {  
		$icon_dir = apply_filters( 'icon_dir', ABSPATH . WPINC . '/images/crystal' );  
		$src_file = $icon_dir . '/' . wp_basename($src); 
		@list($width, $height) = getimagesize($src_file);
	}

i've also tried filtering the icon directory through the icon_dir filter, but b/c there are no variables passed i cannot get the directory to be the exact directory the custom icon is in.

so i can filter the $src of my icon to be:

http://localhost/multi/wp-content/uploads/2011/11/capri-100x100.jpg

and i can filter the icon directory path to be the uploads directory

D:\Users\helga\xampp\htdocs\multi/wp-content/uploads

but b/c the

$src_file = $icon_dir . '/' . wp_basename($src);


the end result for $src_file is:

D:\Users\helga\xampp\htdocs\multi/wp-content/uploads/capri-100x100.jpg

which is missing the sub-directories. i thought about trying to send an empty path to the icon_dir filter and sending the full path to the wp_mime_type_filter, but you can't do that either b/c of that string '/' in the way.

http://localhost/multi/wp-content/uploads/2011/11/capri-100x100.jpg

#6 @helgatheviking
13 years ago

actually i think the function is choking (in terms of being able to do what i am trying to do)

on the next line in wp_get_attachment_src()

	if ( $icon && $src = wp_mime_type_icon($attachment_id) ) {
		$icon_dir = apply_filters( 'icon_dir', ABSPATH . WPINC . '/images/crystal' );
		$src_file = $icon_dir . '/' . wp_basename($src);
		@list($width, $height) = getimagesize($src_file);
	}
	if ( $src && $width && $height )
		return array( $src, $width, $height );
	return false;

b/c of what i mentioned above, the path info is wrong, which leads to not getting the $height and $width, which ends in the function returning false.

#7 @helgatheviking
13 years ago

what about passing the $attachment_id to the icon_dir filter as a second parameter? that shouldn't cause any back-compat issues.

$icon_dir = apply_filters( 'icon_dir', ABSPATH . WPINC . '/images/crystal', $attachment_id );

@helgatheviking
13 years ago

add argument to icon_dir filter

@helgatheviking
13 years ago

add argument to icon_dir filter

#8 @SergeyBiryukov
13 years ago

Related/duplicate: #8268

#9 @jfarthing84
13 years ago

  • Cc jeff@… added

+1 for this.

There are many different ways to tackle this issue. Whether it be through wp_get_attachment_image(), wp_get_attachment_image_src() or image_downsize(). They are all called successively.

I believe a simple new filter on wp_get_attachment_image_src() within wp_get_attachment_image() should work.

@jfarthing84
13 years ago

Add a filter to wp_get_attachment_image_src() within wp_get_attachment_image().

#10 @jfarthing84
13 years ago

  • Keywords has-patch dev-feedback added

#11 @jfarthing84
13 years ago

  • Summary changed from move image_downsize filter to beginning of image_downsize() function to Add the ability to filter wp_get_attachment_image()

#12 @helgatheviking
13 years ago

wow a duplicate from 4 years ago! i almost have to applaud that. i would have to dig out the code i was working on to see if this would work for what I was attempting, but it definitely looks like an elegant solution. 1 line of code and shouldn't have any backcompat issues. i hope it makes it into core.

#13 @helgatheviking
12 years ago

any progress on adding jfarthing84's patch?

#14 @SergeyBiryukov
12 years ago

Looking at media.php.patch, it seems weird that wp_get_attachment_image_src filter is not inside the wp_get_attachment_image_src() function.

20205.patch adds wp_get_attachment_image filter.

Given your use case, post.php.patch also makes sense to me.

#15 @helgatheviking
12 years ago

my post.php.patch is minimally intrusive. adding one more argument shouldn't break anything.

i'd agree that a wp_get_attachment_image_src filter should be inside the wp_get_attachment_image_src function, but that function is currently structured such that if you are getting the attachment_image for a non-image, you pretty much fail right away b/c if you don't get an image back from image_downsize(), you get kicked to looking for an icon. and there's no way to return an image from an attachment's postmeta, b/c image_downsize() immediately returns false if the attachment itself isn't an image.

i like your solution too though. it should be there, but for my use case, it is a little late and would be redundant. but a filter for wp_get_attachment_image_src definitely gives the most control. just a matter of where it should be. it is definitely trickier in the wp_get_attachment_image_src function b/c there are places where a value could return

Last edited 12 years ago by helgatheviking (previous) (diff)

@doublesharp
12 years ago

wp_get_attachment_image_src filter in wp_get_attachment_image_src() function

#16 @doublesharp
12 years ago

Added a new patch for media.php (media.php.2.patch) that places the wp_get_attachment_image_src filter in the wp_get_attachment_image_src() function. Multiple returns are removed in favor of a single array which is then filtered, and if the result is still empty() returns false.

I think this makes more sense as the wp_get_attachment_image_src() could be called directly, and I would expect the results to be filtered.

#17 @jfarthing84
12 years ago

Would love to see this implemented ASAP. I am now working on a new project that includes video uploads and would like to be able to display a video thumbnail using native functions. Obviously, this can't be done until we work this out. It is a rather unfortunate situation to have to remove the default icon row from the list table completely, and replace it with my own in order to display thumbnails.

#18 follow-up: @markoheijnen
12 years ago

If it was me I would close the ticket since that function isn't for that kind functionality. We are talking about attachments and by default they don't have any relationship with something else. I would change some of the logic a different way. Since 3.5 we have Imagick support and hopefully in 3.6 Gmagick support and with that you can create an image from a PDF.

So I guess 20205.patch makes the most sense since that would generate fallback when WordPress can't handle it.

#19 in reply to: ↑ 18 ; follow-up: @jfarthing84
12 years ago

Replying to markoheijnen:

If it was me I would close the ticket since that function isn't for that kind functionality. We are talking about attachments and by default they don't have any relationship with something else.

You don't think that serving up thumbnails of a video (attachment) should be possible with this function? I don't understand that logic.

#20 in reply to: ↑ 19 @markoheijnen
12 years ago

Replying to jfarthing84:

Replying to markoheijnen:

If it was me I would close the ticket since that function isn't for that kind functionality. We are talking about attachments and by default they don't have any relationship with something else.

You don't think that serving up thumbnails of a video (attachment) should be possible with this function? I don't understand that logic.

WordPress can't serve a frame from a video. You need software on a server for doing that. I'm not sure if Imagick/Gmagick can do that.

#21 @jfarthing84
12 years ago

WordPress can't serve a frame from a video. You need software on a server for doing that. I'm not sure if Imagick/Gmagick can do that.

That's not the issue at hand. I should be able to serve up whatever I want in the form of an image that represents an attachment. We're just talking about a simple filter here.

Last edited 12 years ago by jfarthing84 (previous) (diff)

#22 follow-up: @markoheijnen
12 years ago

So in my first comment I did mentioned that 20205.patch is good enough to solve this issue.

#23 in reply to: ↑ 22 @jfarthing84
12 years ago

Replying to markoheijnen:

So in my first comment I did mentioned that 20205.patch is good enough to solve this issue.

Agreed.

@jfarthing84
12 years ago

Refreshing 20205.patch from trunk.

#24 @scribu
12 years ago

In summary, the use case is being able to represent non-image attachments visually, via a plugin.

Can someone provide such a sample plugin, leveraging 20205.2.patch?

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

#25 @scribu
12 years ago

  • Milestone changed from Awaiting Review to 3.6

#26 @doublesharp
12 years ago

I still feel that filtering the wp_get_attachment_image_src() function is a better choice, as it is used by wp_get_attachment_image(), otherwise you will get different results if you have only filtered the latter but also need access to the image URI.

#27 @scribu
12 years ago

I tend to agree: wp_get_attachment_image_src() is right above image_downsize(), so it would be a better fit.

#28 @doublesharp
12 years ago

The patch that I attached - media.php.2.patch​ - does this, but is definitely a larger change than the other patches and requires additional testing and dev-feedback.

@jfarthing84
12 years ago

Filter wp_attachment_image_src instead.

#29 @markoheijnen
12 years ago

The problem what I have with filtering wp_get_attachment_image_src() is the array output. I guess we should rewrite the function so you can manipulate the $src. I will give it a try now.

#30 @jfarthing84
12 years ago

The problem what I have with filtering wp_get_attachment_image_src() is the array output. I guess we should rewrite the function so you can manipulate the $src. I will give it a try now.

What's wrong with passing the whole array into the filter, as I did in the patch I just posted? If you change the image source, the dimensions are more than likely different as well and should be changed just the same.

#31 @markoheijnen
12 years ago

The problem with that is that people will break stuff faster. I was thinking if we can make if safer and easier. They can pass the url and then call getimagesize() on it so you can retrieve the width/height.

I also not sure if we should apply the filter on the result of image_downsize().

#32 @jfarthing84
12 years ago

The problem with that is that people will break stuff faster.

Sorry, but if people "break" something, that's their own problem. There is no warranty, expressed or implied, when you utilize the available hooks that WordPress provides.

I was thinking if we can make if safer and easier. They can pass the url and then call getimagesize() on it so you can retrieve the width/height.

The problem with this approach, is that some servers do not allow calling getimagesize on a remote image. What if somebody wants to use a remote image, such as a YouTube thumb, and they already know the dimensions. They can then pass the known dimensions into the filter.

I also not sure if we should apply the filter on the result of image_downsize().

I would expect the filter to be applied to whatever result the function produces.

#34 @ocean90
11 years ago

  • Milestone changed from 3.6 to Future Release

@ericlewis
11 years ago

#35 @ericlewis
11 years ago

  • Keywords dev-feedback removed

attachment:20205.diff cleans up this function. Let's wrap the image_downsize() call in a wp_attachment_is_image() check, this makes more logical sense, even if that check is already in that function. Don't short-circuit there, instead carry the returned value (similar to @jfarthing74's approach) through the function to be passed into the filter at the end of the function.

This also actually fixes a separate bug: you couldn't previously get the icon if the attachment is an image, it returned early with the image_downsize() call.

On a sidenote, it's convoluted that we offer an 'icon' parameter for this function. I would like to see this deprecated, and a separate function created, perhaps wp_get_attachment_icon_src().

Last edited 11 years ago by ericlewis (previous) (diff)

#36 @ericlewis
11 years ago

#15606 was marked as a duplicate.

@ericlewis
11 years ago

#37 @ericlewis
11 years ago

In attachment:20205.1.diff, need to add backwards compatibility logic: icon will only be returned if an image was not found. If an image is found and an icon is requested, return the image.

#38 @chriscct7
9 years ago

  • Keywords needs-patch added; has-patch removed
Note: See TracTickets for help on using tickets.