WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 days ago

#30180 assigned enhancement

wp_get_attachment_image_src does not return alt or meta

Reported by: joedolson Owned by: antpb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests dev-feedback
Focuses: accessibility Cc:

Description

In practical use,wp_get_attachment_image_src is very useful for displaying an attached image - but requires a separate call to get any other image meta or the alt attribute.

In addition to making any application that needs alt attr or meta data for display simpler, having the alt attribute present in this function increases the likelihood that developers will use the alt attribute in their ultimate output.

Attachments (6)

30180.patch (632 bytes) - added by joedolson 5 years ago.
Adds image alt and meta data to array output
30180-unit-test.patch (825 bytes) - added by collinsinternet 5 years ago.
Unit test to test return of alt.
30180.1.diff (8.9 KB) - added by ryanshoover 7 months ago.
Writes a new function to assist with getting image attributes.
30180.1.unit-tests.patch (7.4 KB) - added by ryanshoover 7 months ago.
Patch file to add unit tests for the new function
30180.2.patch (17.6 KB) - added by ryanshoover 7 months ago.
Updated patch to address feedback
30180.2.1.patch (17.6 KB) - added by ryanshoover 7 months ago.
Attaching improved version of the patch

Download all attachments as: .zip

Change History (60)

@joedolson
5 years ago

Adds image alt and meta data to array output

#1 @collinsinternet
5 years ago

  • Keywords reporter-feedback needs-testing added

This doesn't work since image_downsize() is returned prior to your additions (at least in my tests). Also you will need to update the PHPDoc Block @return value to reflect what is being returned.

@collinsinternet
5 years ago

Unit test to test return of alt.

#2 @joedolson
5 years ago

Yes, image_downsize will need patching as well.

#3 @chriscct7
4 years ago

  • Keywords needs-patch added; has-patch reporter-feedback removed

#4 @joemcgill
4 years ago

It may be better to create a new function that returns these attributes, rather than adding more logic to image_downsize() since image_downsize() is already pretty slow and is used in the logic chain of many other image functions, like wp_get_attachment_image() and get_post_thumbnail().

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


3 years ago

#6 @joedolson
3 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to joedolson
  • Status changed from new to accepted

This needs some resolution; which may or may not be adapting this function. Will work on this for 4.6.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


3 years ago

#8 @joemcgill
3 years ago

As discussed with @joedolson earlier this week, while I am 100% behind the intent of this change, I don't think that adding additional attributes to wp_get_attachment_image_src() is the right approach.

Instead, I think we should look into creating a new function that reuses the internals of wp_get_attachement_image() but returns the attributes as an array of values. Possibly something like wp_get_attachment_image_data() or wp_get_attachment_image_atts(). We can then make wp_get_attachment_image() a wrapper function that converts the output to HTML.

Additionally, it might be helpful to have a helper function like wp_get_attachment_image_alt() that can be used as a standard way of generating an alt attribute when someone is rolling their own image markup.

#9 follow-up: @joedolson
3 years ago

I think this is a great plan. A few questions come to mind:

Which attributes should we return?

Is the purpose of the wp_get_attachment_image_data() function to return all data about the attachment image, or only what's required to generate the img element? E.g., should we return the caption? Should we return the internal image title (as opposed to the title attribute)? Should we return any custom meta fields available for the attachment?

I'm inclined to think that it should only be the fields needed for the img element, but we should probably decide that before building a patch.

#10 in reply to: ↑ 9 @joemcgill
3 years ago

Replying to joedolson:

I think this is a great plan. A few questions come to mind:

Which attributes should we return?
...
I'm inclined to think that it should only be the fields needed for the img element...

Good question. I could see use cases for both, to be honest. A function that only returns an array of attribute values that can be used in img markup could keep the function lighter, but I could also see cases where knowing additional metadata (like which sizes are registered in the database with that image) could be valuable to JS front-ends that are using the information to do more complex calculations based on the result of API calls.

However, a more advanced use case might be better addressed by a proper WP_Image API rather than speculating and trying to do too much here, so a function that returns attributes for the img element is probably a good starting point. With that being the case, I would suggest we name the function wp_get_attachment_image_attributes() (to match the name of the enclosed filter that would need to be maintained).

#11 follow-up: @joemcgill
3 years ago

  • Milestone changed from 4.6 to Future Release
  • Type changed from defect (bug) to enhancement

Now that we have identified a path forward, this is more of an enhancement than a bug, so I'm going to move this out of the 4.6 milestone now that we're in beta. Let's see what can be done for the next release.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 years ago

#13 @afercia
3 years ago

  • Milestone changed from Future Release to 4.8

#14 in reply to: ↑ 11 @bhargavbhandari90
3 years ago

Replying to joemcgill:

Now that we have identified a path forward, this is more of an enhancement than a bug, so I'm going to move this out of the 4.6 milestone now that we're in beta. Let's see what can be done for the next release.

So we can make function like get_post_meta for image attributes.

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 years ago

#20 @afercia
2 years ago

  • Milestone changed from 4.8 to 4.8.1

We're running out of time for the 4.8 release, so: punting.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 years ago

#23 @joemcgill
2 years ago

  • Keywords needs-testing removed
  • Milestone changed from 4.8.1 to Future Release

@joedolson I'm cleaning up the 4.8.1 milestone and am moving this out to future release for now. Feel free to move into the next major if you plan on working on it.

This ticket was mentioned in Slack in #accessibility by ryanshoover. View the logs.


7 months ago

@ryanshoover
7 months ago

Writes a new function to assist with getting image attributes.

#25 @ryanshoover
7 months ago

I've just added a patch that adds a new function wp_get_attachment_image_attr( $attachment_id, $attribute ). The function will return all the significant attribute values that get added to attachment image tags. This should be a nice catch-all for getting the alt text and any other values that a user would want to add to an image tag.

This ticket was mentioned in Slack in #accessibility by ryanshoover. View the logs.


7 months ago

@ryanshoover
7 months ago

Patch file to add unit tests for the new function

#27 @birgire
7 months ago

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

@ryanshoover thanks for the patch.

I just skimmed over 30180.1.unit-tests.patch and noticed few things:

  • The patch uses shorthand array notation [] that's only supported in PHP 5.4+. It's on the plan to bump the minimum version to 5.6 and then 7 next year, but I don't think there's a date yet. So I would suggest using array() until then.
  • The concatenation in the PHP code example uses the Javascript plus + instead of the dot ., i.e. $alt = $alt + ' Extra content';
  • Shouldn't we use null type in the @return as null is a primitive type in PHPDoc?
  • The patch does not contain all the changes made in the patch 30180.1.diff. Is there a reason for that?
  • @joemcgill suggested above the function name wp_get_attachment_image_attributes() but the patch uses wp_get_attachment_image_attr() and same for the filter.
  • @joemcgill mentioned above that the wp_get_attachement_image() could be a wrapper for wp_get_attachment_image_attributes(), but I didn't see that in the patch.
  • It would be nice to have test cases for all the supported nine attributes. Maybe there's a way to use a @dataProvider?

Hope it helps

@ryanshoover
7 months ago

Updated patch to address feedback

#28 @ryanshoover
7 months ago

@birgire Providing responses to your comments below one-by-one.

The patch uses shorthand array notation [] that's only supported in PHP 5.4+.

Fixed!

The concatenation in the PHP code example uses the Javascript plus + instead of the dot ., i.e. $alt = $alt + ' Extra content';

Fixed!

Shouldn't we use null type in the @return as null is a primitive type in PHPDoc?

Fixed!

The patch does not contain all the changes made in the patch 30180.1.diff​. Is there a reason for that?

Not particularly. The unit test patch contained only the code necessary for the new tests to pass. I've included both the new code and the new tests in a single patch this time.

@joemcgill suggested above the function name wp_get_attachment_image_attributes() but the patch uses wp_get_attachment_image_attr() and same for the filter.

Discussed this with @bamadesigner at WCUS18 contributor day. My opinion is that wp_get_attachment_image_attr() is more inline with the other function names that use the abbreviated verbiage for the values they retrieve. i.e. wp_get_attachment_image_src() and wp_get_attachment_image_srcset(). But if you feel the function should spell out attributes I'm happy to make that change.

@joemcgill mentioned above that the wp_get_attachement_image() could be a wrapper for wp_get_attachment_image_attributes(), but I didn't see that in the patch.

Good call. I've reworked wp_get_attachment_image to simply piece together the attributes from the new function rather than generating them itself.

It would be nice to have test cases for all the supported nine attributes. Maybe there's a way to use a @dataProvider?

Good idea! I've added tests for the other attributes using an image stored in the tests/phpunit/data/images directory. Is there a better practice I should follow to provide a real file to the factory?

@ryanshoover
7 months ago

Attaching improved version of the patch

This ticket was mentioned in Slack in #accessibility by ryanshoover. View the logs.


6 months ago

#30 @wingo5315
6 months ago

The function name wp_get_attachment_image_src suggests that it's going to get the URL of the attachment image, not any alt text. If they wanted alt text as well, they would have used a different function.

Although if you wanted the alt text, you could make it so if it said wp_get_attachment_image_src('alt', true); or something along the lines of that.

#31 @afercia
6 months ago

On the other hand, the function already returns "meta" information by default:

@return false|array Returns an array (url, width, height, is_intermediate), or false, if no image is available.

 * The returned array contains four values: the URL of the attachment image src,
 * the width of the image file, the height of the image file, and a boolean
 * representing whether the returned array describes an intermediate (generated)
 * image size or the original, full-sized upload.

I'd agree the function name isn't ideal but I don't see any harm in adding the alt value to the returned array.

#32 @joedolson
6 months ago

It's not inconceivable to add a wrapper function to improve the name, but this function can't be changed just to return a URL. Right now, the function returns three of the four most critical img attributes, but is missing the alt attribute. The motivation behind this change is so that a single function can return all the necessary attributes for displaying the image, rather than requiring a second call to get the alt attribute.

Is the patch provided good to go? It looks reasonably solid to me.

#33 @ryanshoover
6 months ago

The existing wp_get_attachment_image_src fucntion is oddly named as it returns 4 different values, only one of which corresponds to the function name.

The mindset with the new wp_get_attachment_image_attr is that it's a single function that can retrieve a number of relevant image attributes. Rather than continue to "tweak" a function that should be renamed, themers can now call wp_get_attachment_image_attr( $attachment_id, 'alt' ); to get the image alt tag. If the themer wants to do more complex creation of an image tag, wp_get_attachment_image_attr( $attachment_id ); will return all attributes needed to create a full <img> tag.

$attrs = wp_get_attachment_attr( $attachment_id );

// Echo the image.
?>
<img
    src="<?php echo esc_url( $attrs['src'] ); ?>"
    alt="<?php echo esc_attr( $attrs['alt'] ); ?>"
    class="<?php echo esc_attr( $attrs['class'] ); ?>"
    width="<?php echo esc_attr( $attrs['width'] ); ?>"
    height="<?php echo esc_attr( $attrs['height'] ); ?>"
    srcset="<?php echo esc_attr( $attrs['srcset'] ); ?>"
    sizes="<?php echo esc_attr( $attrs['sizes'] ); ?>"
>
<?php

Can I propose that we accept this patch adding a new function? Or do I have clear direction to rework wp_get_attachment_image_src to also return the alt attribute as a 5th element in an indexed array?

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 months ago

#35 @afercia
6 months ago

  • Milestone changed from Future Release to 5.2

Discussed during today's accessibility bug-scrub and agreed to try this for 5.2.

#36 @joedolson
5 months ago

I think that going ahead and adding this new function is a satisfactory solution to the issue, leaving wp_get_attachment_image_src untouched.

Does anybody have any further concerns about this? If not, I'd recommend this for review & commit.

#37 @joemcgill
5 months ago

I much prefer the new function as described by @ryanshoover in comment 33. If 30180.2.1.patch is ready for review, I am happy to take a look at getting it into 5.2.

#38 @afercia
5 months ago

  • Keywords 2nd-opinion added

@joemcgill do you see any harm in adding alt to the values returned by wp_get_attachment_image_src()?

A new function is welcome but then the existing wp_get_attachment_image_src() will still be largely insufficient for accessibility needs and at that point it should be deprecated.

#39 @joedolson
5 months ago

I'd rather see wp_get_attachment_image_src() deprecated. I don't see a value to keeping two very similar functions - the new function adds the functionality we're looking for, is more accurately named, and fulfills all the needs of the old function. Deprecation seems like a better path.

#40 @joemcgill
5 months ago

@afercia I disagree that wp_get_attachment_image_src() should include the alt attribute. That function in its current form is used in many situations where the goal is not to build img markup attributes, and in those cases, including the alt attribute adds an unnecessary post meta lookup. Even so, it's understandable, given the name of the function, that developers confuse its purpose and use it to build image markup that excludes the alt attribute. Promoting the usage of the new function should help.

@joedolson I'm open to deprecating it, but that seems to be outside the scope of this ticket, and it's such a widely used function—with 13853 matches in the plugin directory—that I think actual deprecation/removal of the function is unlikely, without risking a lot of breakage.

#41 @afercia
5 months ago

and it's such a widely used function

I guess this is the whole point :) it's widely used but leads to far from ideal accessibility, as there's no alt attribute returned.

In its current state, when used to build img markup attributes, it should be considered a bad practice.

#42 @ryanshoover
5 months ago

@afercia We made the call to go with a new function for getting the alt text because of limitations in wp_get_attachment_image_src. As @joemcgill points out, the function's name doesn't lend itself to getting extra information about an image other than its src attribute. Additionally, it returns results in an indexed array where the developer has to know which "magic number" index attribute refers to which property.

In terms of expanding the current function vs adding a new one - any developer looking get the alt attribute from this ticket's specs will need to write new code anyway. I don't see it as a big leap to ask them to use a new function, rather than have them look for a 5th entry in wp_get_attachment_image_src.

Is there a chance that expanding the return value of wp_get_attachment_image_src may break poorly written code? If someone is simply looping through the results to generate their image tag, we may inadvertantly break their tag generation with a 5th option.

Last edited 5 months ago by ryanshoover (previous) (diff)

#43 @ryanshoover
5 months ago

In terms of deprecating wp_get_attachment_image_src, agreed that's definitely outside the scope of this ticket. It's a valid conversation to have. From a purist, philosophical standpoint, simply because a function is heavily used doesn't mean it shouldn't be deprecated in favor of a better method. We just need to provide plenty of leeway, flexibility, and communication to the community about the change.

I personally would be in favor of deprecating it — at some point — in large part because its behavior is anomalous to most other templating functions.

#44 @joedolson
5 months ago

I can see some issues with the eventual removal of wp_get_attachment_image_src, since it's a very commonly used function, but that shouldn't stop us from deprecating it and discouraging its use.

I also agree that deprecating that function is out of this ticket's scope, and should be a separate ticket.

I don't see it as very likely that expanding that function's return value would break code - it seems like just iterating blindly over the values wouldn't work, since the values are numerically keyed - they'd have to call specific key values. Regardless, I'd still prefer deprecation.

In an ideal world, I'd want to remove the extra attributes from wp_get_attachment_image_src and only return src, but that's definitely not an option!

#45 @afercia
5 months ago

deprecating that function is out of this ticket's scope

I'm not fully sure it is actually. If this ticket introduces a new function with the specific purpose of providing a better way to get image metas, it also means the new function should be recommended especially when building the image markup.

It also automatically implies the old function is not ideal, at least for the purpose of building the image markup, and I'd say also for the reasons mentioned by @ryanshoover.

At the very least, the wp_get_attachment_image_src documentation should be updated to clearly state in uppercase, blinking, characters :) "do not use this function to build the image markup, use xyz instead."

#46 @joedolson
5 months ago

My feeling that the deprecation is out of scope is more that making that decision does not block us from adding a new function that serves our needs better, nor does it block us from updating documentation to recommend the new function.

Whether the function should be deprecated can be continued after adding the new function. These changes don't depend on it.

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


4 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 months ago

#49 @afercia
4 months ago

  • Keywords dev-feedback added; 2nd-opinion removed

@joemcgill @antpb pinging you as component maintainers. The patch needs your review. Kind reminder we're at 5 days from beta 1. Thanks!

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


4 months ago

#51 @antpb
4 months ago

  • Milestone changed from 5.2 to 5.3

With the deadline being reached, we will need to punt this to 5.3. The work here is VERY close. All that is remaining is the deprecation mentioned above which the Media team has determined to be in the scope of this task.

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


2 weeks ago

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


3 days ago

#54 @antpb
3 days ago

  • Owner changed from joedolson to antpb
  • Status changed from accepted to assigned
Note: See TracTickets for help on using tickets.