WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#27399 reopened enhancement

Filter the HTML output of wp_get_attachment_image

Reported by: fgirardey Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Hi everyone, I've got an idea, maybe a bad one, but i think it could be useful to add a filter to the wp_get_attachment_image function for the html output.
In my case, a plugin that i use for lazy load is filtering html output of post thumbnails, i would like to let this plugin filter my wp_get_attachment_image output.

Something like that :

	return apply_filters( 'attachment_image_html', $html, $attachment_id, $size, $attr );

Attachments (4)

media.php (69.5 KB) - added by fgirardey 4 years ago.
/wp-includes/media.php core file to update
27399.patch (644 bytes) - added by fgirardey 4 years ago.
Patch proposal for a 'attachment_image_html' filter
27399.2.patch (825 bytes) - added by FlorianBrinkmann 8 weeks ago.
Refreshed patch
27399.diff (832 bytes) - added by FlorianBrinkmann 8 weeks ago.

Download all attachments as: .zip

Change History (30)

@fgirardey
4 years ago

/wp-includes/media.php core file to update

@fgirardey
4 years ago

Patch proposal for a 'attachment_image_html' filter

#1 @tomauger
4 years ago

  • Keywords reporter-feedback added

Hi! Thanks for posting this suggestion and patch. Can you provide more background on what you expect the filter to be used for? Are you replacing the HTML wholesale? or just moving the image SRC into a data-attribute?

Have you considered the wp_get_attachment_image_attributes filter for adding attributes to the image tag?

#2 @fgirardey
4 years ago

  • Keywords has-patch added; reporter-feedback removed

Yeah i've consider it, the fact is there is an existing HTML filter "post_thumbnail_html" for the get_the_post_thumbnail function, so it could be logic to have a similar filter for attachments image.
In my case, the plugin use a regex to replace the src attribute and move it to a data-* attribute, this is maybe not the better choice but it works on "get_avatar", "post_thumbnail_html", "the_content" and "widget_text". So it can be useful for other features in my opinion =)

Edit: oups sorry i've removed the reporter-feedback keywords :$

Last edited 4 years ago by fgirardey (previous) (diff)

#3 @fgirardey
4 years ago

  • Keywords reporter-feedback added

#4 @tomauger
4 years ago

Nacin hinted that srcset and possibly <picture> might be coming soon.

See the spec on the Chromium blog.

#5 @fgirardey
4 years ago

Yay, a great thing, especially the <picture> element.
Otherwise, this patch is legitimate since this kind of filter is already existing for post thumbnail but not for attachment image, i can't understand why it can be awkward, any idea ? :)

Last edited 4 years ago by fgirardey (previous) (diff)

#6 @fgirardey
3 years ago

  • Keywords reporter-feedback removed

#7 follow-up: @asadowski10
3 years ago

Hi @fgirardey, @tomauger,

This patch is very interesting.
It would be very useful on my projects.

In my case, when I try to recover from a media ID but that it is no longer I'd like to display a default image instead.

In the present state of things, I can do this.
Thanks

#8 @DrewAPicture
3 years ago

  • Keywords close added

I'm not really a fan of introducing a filter on built-HTML content like this. We talk a lot about the usefulness (or lack thereof) of filtering built SQL statements in much the same light. The stated use-case comment:2 can already be accomplished with the wp_get_attachment_image_attributes filter.

I don't think the existence of a similarly-intentioned filter in another function is enough of a justification for the added complexity in this context.

#9 @tomauger
3 years ago

As for <srcset> support, there is now a solid plugin for that: https://wordpress.org/plugins/truenorth-srcset/

#10 in reply to: ↑ 7 @SergeyBiryukov
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to asadowski10:

In my case, when I try to recover from a media ID but that it is no longer I'd like to display a default image instead.

#20205 might help with that, it has a patch for filtering wp_get_attachment_image_src().

Closing as wontfix, per comment:8.

#11 @spacedmonkey
2 years ago

I really don't agree. There are a number of reasons to have a filter like this. At the moment, there is no way to support the picture on attached images, unless you simple build the tag yourself. That isn't good enough. There are other places in core where tags are filters like this, here are three for example.

script_loader_tag
https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/class.wp-scripts.php#L159

style_loader_tag
https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/class.wp-styles.php#L91

post_thumbnail_html
https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/post-thumbnail-template.php#L159

I don't see how this is any different

#12 @joemcgill
2 years ago

FWIW, even though there are valid use cases for filtering the HTML output of some functions, I agree with not filtering the HTML output of this particular function. The purpose of this function is to output an img element based on an attachment id and size and that's how it's used throughout core. As it stands now, every attribute of the element can be filtered, just not the element itself. Seems to me that this is by design and allowing this to modified through the introduction of an output filter would introduce more problems than it solves.

#13 @markoheijnen
13 months ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Reopening based on the feedback of @spacedmonkey as I do agree with him.wp_get_attachment_image() is the only location where you could modify the output to add the picture element. Maybe filters as post_thumbnail_html would help but it would miss some instances.

If the purpose really is to output an img element then it would mean that we need an additional function that is between wp_get_attachment_image() and functions like the_post_thumbnail().

#14 follow-up: @andreamk
13 months ago

Hello
Another reason to add the filter is to insert a noscript tag.

I would like to integrate the library interchange in wordpress (http://foundation.zurb.com/sites/docs/interchange.html)
The html that I would make is the following

<img data-interchange="[assets/img/interchange/small.jpg, small], [assets/img/interchange/medium.jpg, medium], [assets/img/interchange/large.jpg, large]">
<noscript>
<img src="assets/img/interchange/large.jpg">
</noscript>

Without the proposed filter is impossible

#15 follow-up: @joemcgill
13 months ago

I still think adding an additional filter on the built HTML here is unnecessary and could lead to more problems than it solves, since this is the function used throughout the admin for displaying images. It seems to me that if you're trying to use wp_get_attachment_image for anything other than outputting an img element based on an attachment ID and size, then you're using the wrong tool.

However, I could see a need to abstract the internals of wp_get_attachment_image into a function that returns the attributes of an image as an array so it's easier to build your own custom HTML. We could then make wp_get_attachment_image a wrapper that is based on the new function.

#16 @netweb
13 months ago

  • Milestone set to Awaiting Review

Moving reopened tickets without a milestone back to Awaiting Review for review

#17 in reply to: ↑ 14 @tabrisrp
12 months ago

Same as @andreamk, our current issue with this function is that we're not able to append a noscript tag to the html. Having a filter on the html output, or any other way that would help us to do would be very helpful.

#18 @SergeyBiryukov
11 months ago

  • Keywords needs-patch added; has-patch close removed

Needs a new patch, per comment:15.

#19 @pcfreak30
9 months ago

I am making a new plugin that removes the width and height string since it is really not needed if using srcset, but I can't do it easily since it hardcoded in

$hwstring

and not passed via filter

wp_get_attachment_image_attributes

So the statement that all attributes can be changed is false. I have to now resort to output buffering to solve this.

Thanks.

#20 @jeppeskovsgaard
3 months ago

This issue is holding back all lazy load plugins from working with wp_get_attachment_image, since adding a noscript tag is not possible.

#21 in reply to: ↑ 15 @glueckpress
3 months ago

Replying to joemcgill:

I still think adding an additional filter on the built HTML here is unnecessary and could lead to more problems than it solves, since this is the function used throughout the admin for displaying images. It seems to me that if you're trying to use wp_get_attachment_image for anything other than outputting an img element based on an attachment ID and size, then you're using the wrong tool.

That’s exactly what people do. And then they hear about performance, activate a plugin to lazy-load their images, and notice that images rendered via wp_get_attachment_image are not lazy-loaded.

Why? Because plugins can’t hook into the function as they can with other functions in order to append the required <noscript> tag to the image HTML output.

The patch proposed here would solve the dilemma perfectly fine. Fwiw, I don’t see why further abstraction would be necessary? It’s a simple filter. It would enable plugin developers to solve a common problem. It would really help, and it seems to be low hanging fruit, doesn’t it?

#22 follow-up: @johnbillion
3 months ago

  • Version changed from 3.8.1 to 2.5

If someone wants to do a new patch as per the comments above, I'm in favour of getting this in.

@FlorianBrinkmann
8 weeks ago

Refreshed patch

#23 in reply to: ↑ 22 @FlorianBrinkmann
8 weeks ago

Replying to johnbillion:

If someone wants to do a new patch as per the comments above, I'm in favour of getting this in.

attachment:27399.2.patch refreshes @fgirardey’s patch for the latest trunk version.

#24 @FlorianBrinkmann
8 weeks ago

  • Keywords has-patch added; needs-patch removed

#25 @birgire
8 weeks ago

@FlorianBrinkmann It looks like:

* @param string       $attr          Query string of attributes.

should be

* @param array        $attr          Attributes for the image markup.

But I read the ticket that such a filter wouldn't be the approach here, instead move the internals of wp_get_attachment_image() into a new function with an array output?

Maybe I misread ;-)

If the filter is the way to go, then I wonder about the filter's name attachment_image_html - why not e.g. the same as the function's name, wp_get_attachment_image ?

Last edited 8 weeks ago by birgire (previous) (diff)

#26 @FlorianBrinkmann
8 weeks ago

@birgire regarding the param type: Yes, you are right, thanks for the hint!

But I read the ticket that such a filter wouldn't be the approach here, instead move the internals of wp_get_attachment_image() into a new function with an array output?

Yes, I was unsure here what @johnbillion means with »as per the comments above« (maybe just the ones directly above? ;) ), but then just created the patch, as it was not that time consuming :)

If the filter is the way to go, then I wonder about the filter's name attachment_image_html - why not e.g. the same as the function's name, wp_get_attachment_image?

It was inspired by the naming of the post_thumbnail_html filter, but if I am looking at it now; there are already filters like wp_get_attachment_image_attributes, so yes, maybe it would be better to keep it consistent with these filters, include the function’s name, and only name it wp_get_attachment_image.

I will update the patch accordingly, maybe it is the approach John means :)

Note: See TracTickets for help on using tickets.