Make WordPress Core

Opened 4 years ago

Closed 6 months ago

#48149 closed enhancement (wontfix)

Suggestion: Optional parameter to remove width and height attribute from attachment image markup

Reported by: subrataemfluence's profile subrataemfluence Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.2.3
Component: Media Keywords: has-patch 2nd-opinion
Focuses: Cc:


First of all I am sorry if this had already been addressed and enhanced.
I am on 5.2.3.

Like post_thumbnail_html and image_send_to_editor filters I was unable to find one for attachment images for posts.

If we can add an additional Boolean type argument in wp_get_attachment_image function (/wp-includes/media.php, line 872) in the following manner and create $hwstring string based on this argument's value, it will give us the control to decide whether or not to render those two attributes.

Here is my suggestion:

function wp_get_attachment_image( $attachment_id, $size = 'thumbnail', $render_hw = false, $icon = false, $attr = '' ) {
     $hwstring = $render_hw ? image_hwstring( $width, $height ) : '';

The default function call will be:

wp_get_attachment_image( $att->ID,  'infotravel-post-page-thumb' );

And, if width and height attributes need to be added, the function call will change to:

wp_get_attachment_image( $att->ID, true,  'infotravel-post-page-thumb' );

There are two reasons why I am proposing this:

  1. Responsiveness. Image width is controlled from CSS and media-queries
  2. The function already accepts a $size. Since we have the ability to pass a specific known size, we do not need to add width and height attributes.

Based on the above approach I have proposed a patch. Please let me know whether it is useful!

Attachments (2)

48149.diff (1.5 KB) - added by subrataemfluence 4 years ago.
Proposed patch
48149-2.diff (1.4 KB) - added by subrataemfluence 4 years ago.
New parameter has been pushed to the end of current argument list.

Download all attachments as: .zip

Change History (6)

4 years ago

Proposed patch

#1 @SergeyBiryukov
4 years ago

Previously: #14110, #20358, #30525.

Thanks for the patch! Just a quick note that new parameters cannot be added in the middle of an existing function signature, as that breaks backward compatibility. They can only be added at the end.

It would also be good to revisit the discussions from the previous tickets linked above.

#2 @subrataemfluence
4 years ago

It was my mistake to put the new argument in middle of existing function signature. It has to be at the end. Thank you for pointing out.

I have gone through the discussions in earlier tickets and here is my observation (I may be wrong):

#14110: The patches suggested there use a lot of code modification which to me looks unnecessary.

#20358 and #30525 both are suggesting a filter. But I think an additional filter for this purpose might not be a good idea, specially when just adding a single parameter (at the end of current argument list of course!) is sufficient to handle the situation.

Please let me know whether I may upload a modified patch putting the new argument at the end of current list.

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

4 years ago

New parameter has been pushed to the end of current argument list.

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

6 months ago

#4 @antpb
6 months ago

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

Considering the long history of discussion on this change and the advancements in ways to override via css using responsive logic, it seems high risk with low reward to make a potentially breaking change to a popular function.

It was agreed in the Media component meeting that this should be a wontfix.

Note: See TracTickets for help on using tickets.