WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 81 minutes ago

#30180 assigned enhancement

wp_get_attachment_image_src does not return alt or meta

Reported by: joedolson Owned by: antpb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-dev-note
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 (7)

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 9 months ago.
Writes a new function to assist with getting image attributes.
30180.1.unit-tests.patch (7.4 KB) - added by ryanshoover 9 months ago.
Patch file to add unit tests for the new function
30180.2.patch (17.6 KB) - added by ryanshoover 9 months ago.
Updated patch to address feedback
30180.2.1.patch (17.6 KB) - added by ryanshoover 9 months ago.
Attaching improved version of the patch
30180.2.2.diff (12.1 KB) - added by antpb 27 hours ago.
Fixes tests and undefined variable of $src

Download all attachments as: .zip

Change History (78)

@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.


3 years ago

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


3 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.


9 months ago

@ryanshoover
9 months ago

Writes a new function to assist with getting image attributes.

#25 @ryanshoover
9 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.


9 months ago

@ryanshoover
9 months ago

Patch file to add unit tests for the new function

#27 @birgire
9 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
9 months ago

Updated patch to address feedback

#28 @ryanshoover
9 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
9 months ago

Attaching improved version of the patch

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


8 months ago

#30 @wingo5315
8 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
8 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
8 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
8 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.


8 months ago

#35 @afercia
8 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
7 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
7 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
7 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
7 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
7 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
7 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
7 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 7 months ago by ryanshoover (previous) (diff)

#43 @ryanshoover
7 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
7 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
7 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
7 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.


6 months ago

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


6 months ago

#49 @afercia
6 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.


6 months ago

#51 @antpb
6 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.


3 months ago

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


2 months ago

#54 @antpb
2 months ago

  • Owner changed from joedolson to antpb
  • Status changed from accepted to assigned

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


2 months ago

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


8 weeks ago

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


9 days ago

#58 @audrasjb
9 days ago

  • Keywords needs-dev-note added

Looks like it was close to be fixed in 5.2.
Any chance to get this one committed in 5.3 @antpb

(adding needs-dev-note keyword)

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


2 days ago

#60 @afercia
2 days ago

  • Type changed from enhancement to defect (bug)

Discussed during today's accessibility bug scrub. Raising from "enhancement" to "bug" as anything related to the image alt attribute is of fundamental importance for accessibility.

Also, this way this ticket can be committed also during the WordPress 5.3 release Beta phase which starts soon.

#61 @antpb
28 hours ago

After applying the patch, on line 1170 of media.php there is a reference to an undefined variable of $src This was causing a bunch of test failures. I'll be uploading a new patch with a fix. The object should now be $image_attr['src']

I also noticed that the newly introduced tests are failing in the latest patch. I'll include a fix soon in my patch upload!

/var/www/tests/phpunit/tests/media/getAttachmentAttributes.php:103

2) Tests_Media_GetAttachmentAttributes::test_should_return_attachment_attr_srcset
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.org/wp-content/uploads/2019/18/canola-3.jpg 640w, http://example.org/wp-content/uploads/2019/18/canola-3-300x225.jpg 300w'
+'http://example.org/wp-content/uploads/2019/09/canola-3.jpg 640w, http://example.org/wp-content/uploads/2019/09/canola-3-300x225.jpg 300w'

This is due to the date function used in the $expected variables: $expected .= date( 'Y/j/'); The j was using the date instead of the month.

Worth noting, I see 17 instances of the wp_get_attachment_image_src() here: https://github.com/WordPress/wordpress-develop/search?p=2&q=wp_get_attachment_image_src&unscoped_q=wp_get_attachment_image_src

In the above discussions, we deemed the deprecating the old function to be in scope of this ticket. One instance of it is in deprecated.php and is an older function that recommends using wp_get_attachment_image_src() I'll be sure to update that as well to recommend new function introduced with this ticket.

I have some concerns about making the changes to the ~16 other instances in Core (including various default themes). I'm completely on board with implementing the changes to ensure Core uses the new function, however, I must warn that being so close to beta 1, I don't feel confident that this patch should land for 5.3 without enough testing. I'm also not a fan of deprecating and having notices violating in Core. If we remove deprecation from the scope, I feel a lot better with committing this before 5.3 beta 1.

Last edited 28 hours ago by antpb (previous) (diff)

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


28 hours ago

@antpb
27 hours ago

Fixes tests and undefined variable of $src

#63 follow-up: @afercia
19 hours ago

however, I must warn that being so close to beta 1, I don't feel confident that this patch should land for 5.3 without enough testing.

@antpb thanks for looking into this. I don't want to sound unfair but that's a bit disappointing.

This ticket was created 5 years ago. 6 months ago I pinged the Media component maintainers because WordPress 5.2 Beta was approaching. The ticket was punted because it was not prioritized and at some point it was too late.

Now, 6 months later, it appears we are at the same point because it's just a few days from WordPress 5.3 Beta. That's unfortunate.

In the spirit of cross collaboration between teams and for the greater good of the WordPress users how the accessibility team can help the Media team move this ticket on?

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


13 hours ago

#65 in reply to: ↑ 63 @antpb
12 hours ago

Replying to afercia:

however, I must warn that being so close to beta 1, I don't feel confident that this patch should land for 5.3 without enough testing.

@antpb thanks for looking into this. I don't want to sound unfair but that's a bit disappointing.

This ticket was created 5 years ago. 6 months ago I pinged the Media component maintainers because WordPress 5.2 Beta was approaching. The ticket was punted because it was not prioritized and at some point it was too late.

Now, 6 months later, it appears we are at the same point because it's just a few days from WordPress 5.3 Beta. That's unfortunate.

In the spirit of cross collaboration between teams and for the greater good of the WordPress users how the accessibility team can help the Media team move this ticket on?

@afercia sorry! I can see how that can be misread. I think this patch absolutely can make it in for 5.3. I am just proposing we remove the deprecation of the old function from the scope as it could have some unintended consequences(and would require the testing I was unclear about above.) I am all for getting this in the next week (pending another committer approval) if we can all agree to that.

Last edited 11 hours ago by antpb (previous) (diff)

#66 @afercia
10 hours ago

@antpb thanks for clarifying! I read that as punting the whole patch, which triggered my disappointment. Sorry for that.

#67 @azaozz
10 hours ago

  • Keywords has-patch has-unit-tests dev-feedback removed
  • Milestone changed from 5.3 to Future Release

Frankly I tend to agree with @antpb here. Looking at the new wp_get_attachment_image_attr() it tries to "do everything for everybody" and introduces strange/inconsistent behaviour and lots of complexity.

The new function is just a helper function to do several things that can be done in few other ways. It is "nice to have", but it should at least be easy to use/understand, and ...helpful :)

After a quick look, thinking that wp_get_attachment_image_attr() should be simplified significantly. It should return all available image attachment attributes, and should not support passing an array of unidentified parameters. That just adds lots of complexity and makes the function harder to use, especially if that array is changed in the future.

We definitely don't want to end up with "monstrous", inconsistent, weird behaviour stuff like the misused image_constrain_size_for_editor() which should never be used outside the classic editor context.

Thinking that to make this new function useful it will need to be changed:

  • Remove the $attribute param. Doesn't make sense to limit it. Seems that all needed data would be cached by that time.
  • Remove the $args param. "Unidentified" parameters are never a good idea imho.
  • Remove the second/duplicate filter with dynamic name. Dynamically named filters are always a bad idea and it is not needed.

To make it possible to limit it to a particular sub-size (which is not a good idea, but may make sense in some contexts), a $size array with max width and height might be useful, but needs more thinking/testing.

Also, introducing a filter wp_get_attachment_image_attr when there is wp_get_attachment_image_attributes in wp_get_attachment_image() is really misleading. Thinking that naming in the patch needs review.

Last edited 9 hours ago by azaozz (previous) (diff)

#68 @joemcgill
7 hours ago

  • Type changed from defect (bug) to enhancement

I agree with @azaozz.

I'd like to see an approach to this function that has a similar signature to wp_get_attachment_image_src(), that accepts $attachment_id and $size and returns a full set of attributes for creating an img tag. This new function should probably be used in the internals of wp_get_attachment_image() to generate a default set of attributes and should be the place where the wp_get_attachment_image_attributes filter is applied, rather than having confusing duplicate filters.

I'm also changing this ticket back to an enhancement ticket, because that's really what it is. There isn't a bug being fixed by this ticket, but instead is providing a new public function that can be used as a best practice by people building interfaces on top of WordPress.

#69 @afercia
6 hours ago

I'm also changing this ticket back to an enhancement ticket, because that's really what it is. There isn't a bug being fixed by this ticket,

I'm sorry I have to say I'm pretty uncomfortable with this. I appreciate the technical discussion happening in the latest comments but, whatever the technical decision is, the point is this ticket is waiting since months. It was already punted for 5.2 After that, the media team had more than 6 months to explore and refine a new solution. I'm surprised nothing relevant was done in such a long time period.

Now, moving it back to "enhancement" implies a couple things:

  • It needs to be fixed by Monday (Beta 1 is September 23rd): that's only 4 days from now (2 work days) and it's unlikely to happen.
  • As said in a previous comment: anything related to the alt attribute, which is a so basic accessibility feature, is considered a bug from the accessibility team. The ticket type was changed to "bug" after a decision of the accessibility team that discussed this ticket during the latest bug-scrub. Changing it back to "enhancement" without at least a cross-team discussion is a bit unfair and frankly doesn't help cross-teams collaboration.

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


6 hours ago

#71 @joemcgill
4 hours ago

As said in a previous comment: anything related to the alt attribute, which is a so basic accessibility feature, is considered a bug from the accessibility team. The ticket type was changed to "bug" after a decision of the accessibility team that discussed this ticket during the latest bug-scrub. Changing it back to "enhancement" without at least a cross-team discussion is a bit unfair and frankly doesn't help cross-teams collaboration.

This is a fair criticism, but I would likewise state that changing the ticket from an enhancement to a bug without conversation with folks on the Media team is a bit unfair, given that the functionality is a part of the Media component and we had previously marked this as an enhancement intentionally.

Here's the state of things as I understand them:

  1. The ticket was originally a request to add data for the alt as a return value for wp_get_attachment_image_src(). After reviewing with the Media team and discussing with @joedolson, the decision was that we should not add the alt attribute data to that particular function for all the reasons previously stated in this thread.
  2. Rather than close this ticket as a wontfix, we proposed adding a new function that could be used by theme authors and others that includes the alt attribute so that we could promote the usage of this new function when building image markup, rather than using wp_get_attachment_image_src() which isn't fit for the purpose of building image markup—this new function is an enhancement to WordPress because it is providing new functionality, not fixing broken functionality.
  3. @ryanshoover created an initial implementation for the new functionality, which has since been iterated on, but frankly hasn't received feedback in a timely manner. Now that @antpb has picked it up, we've identified significant problems with the implementation that we'd like to see addressed before adding this new function to WordPress.

For this to move forward, we'll need an updated implementation that takes into account feedback from @azaozz and I earlier today. This may, indeed, cause this ticket to slip out of scope of this release—which is regrettable, but I believe to be better than introducing this new functionality in its current form.

Last edited 81 minutes ago by joemcgill (previous) (diff)
Note: See TracTickets for help on using tickets.