Make WordPress Core

Opened 9 years ago

Last modified 8 months ago

#30180 assigned enhancement

wp_get_attachment_image_src does not return alt or meta

Reported by: joedolson's profile joedolson Owned by: antpb's profile 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 9 years ago.
Adds image alt and meta data to array output
30180-unit-test.patch (825 bytes) - added by collinsinternet 9 years ago.
Unit test to test return of alt.
30180.1.diff (8.9 KB) - added by ryanshoover 5 years ago.
Writes a new function to assist with getting image attributes.
30180.1.unit-tests.patch (7.4 KB) - added by ryanshoover 5 years ago.
Patch file to add unit tests for the new function
30180.2.patch (17.6 KB) - added by ryanshoover 5 years ago.
Updated patch to address feedback
30180.2.1.patch (17.6 KB) - added by ryanshoover 5 years ago.
Attaching improved version of the patch
30180.2.2.diff (12.1 KB) - added by antpb 5 years ago.
Fixes tests and undefined variable of $src

Download all attachments as: .zip

Change History (93)

@joedolson
9 years ago

Adds image alt and meta data to array output

#1 @collinsinternet
9 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
9 years ago

Unit test to test return of alt.

#2 @joedolson
9 years ago

Yes, image_downsize will need patching as well.

#3 @chriscct7
8 years ago

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

#4 @joemcgill
8 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.


8 years ago

#6 @joedolson
8 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.


8 years ago

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


7 years ago

#13 @afercia
7 years ago

  • Milestone changed from Future Release to 4.8

#14 in reply to: ↑ 11 @bhargavbhandari90
7 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.


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

#23 @joemcgill
7 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.


5 years ago

@ryanshoover
5 years ago

Writes a new function to assist with getting image attributes.

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


5 years ago

@ryanshoover
5 years ago

Patch file to add unit tests for the new function

#27 @birgire
5 years 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
5 years ago

Updated patch to address feedback

#28 @ryanshoover
5 years 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
5 years ago

Attaching improved version of the patch

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


5 years ago

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


5 years ago

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

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#54 @antpb
5 years 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.


5 years ago

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


5 years ago

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


5 years ago

#58 @audrasjb
5 years 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.


5 years ago

#60 @afercia
5 years 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
5 years 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 5 years ago by antpb (previous) (diff)

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


5 years ago

@antpb
5 years ago

Fixes tests and undefined variable of $src

#63 follow-up: @afercia
5 years 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.


5 years ago

#65 in reply to: ↑ 63 @antpb
5 years 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 5 years ago by antpb (previous) (diff)

#66 @afercia
5 years ago

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

#67 @azaozz
5 years 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 5 years ago by azaozz (previous) (diff)

#68 @joemcgill
5 years 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
5 years 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.


5 years ago

#71 @joemcgill
5 years 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 4 years ago by joemcgill (previous) (diff)

#72 follow-up: @afercia
4 years ago

@joemcgill thanks for your feedback. I do appreciate your observations and fair criticism.

I see in your reply you're mainly focusing on the technical side. I'd like to clarify my criticism isn't about technical solutions: it's definitely up to the Media team to identify the best path because of their expertise. My criticism is about the extended lack of activity on this ticket for a long time.

It appears we have different views on what the role of component maintainers and ticket owners are. I'd expect component maintainers to do their best to timely address issues that are considered serious issues by other teams. Instead, ticket owners don't necessarily have to code a patch or be technical persons.

Looking at the history of this ticket, following the chronological order of events, it's clear it didn't receive much attention. It was punted several times skipping at least 4 entire release cycles.

The ticket was milestoned again for new release cycles a few times only because the accessibility team took action after discussing the ticket in their weekly bug scrubs. And then it was regularly punted again because of lack of activity.

The accessibility team is a small team: we don't have the resources to handle large amounts of tickets. We also lack technical expertise in some areas. That's the reason why we often ask for help and collaboration from other teams. Considering the history of this ticket, seeing it punted again a few days before 5.3 Beta 1 because of lack of activity is, frankly, terribly disappointing.

Today (September 23rd) at 15:00 UTC on the #accessibility Slack channel, the accessibility team would like to discuss where things stand in this release cycle and how the cross-team collaboration can be improved to be more effective. Everybody's welcome to join and share feedback.

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


4 years ago

#74 in reply to: ↑ 72 @azaozz
4 years ago

Replying to afercia:

Looking at the history of this ticket, following the chronological order of events, it's clear it didn't receive much attention. It was punted several times skipping at least 4 entire release cycles.

I think I can answer this :)

This ticket is a minor enhancement that makes sense on some level but is also somewhat "questionable". It doesn't bring anything new, just repeats existing code/features and tries to make them a bit more convenient to use, but at the same time has some drawbacks.

Imho this is the reason it went through several iterations and is still not ready. It doesn't bring anything of significant value. Looking at the ticket itself:

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.

That's right. wp_get_attachment_image_src() is a function to retrieve the src of an image. Nothing more. Similarly other attachment data can be retrieved by other functions. This works quite well.

Having a helper/convenience function to combine all of the attachment data that may be needed when outputting an <img> tag is not a bad idea, but it repeats existing functionality that already is straightforward and easy to use.

As @joemcgill and I mention above the current implementation is "not there yet". It should probably be a refactoring of wp_get_attachment_image(). The functionality proposed in this ticket seems to belong there. And yes, it may take a long time to come up with the right solution/code, especially when the proposed change is "on the fence" and doesn't bring new features or significant improvements.

There is also the chance that automatically adding alt text that is stored in the DB to all <img> tags will introduce errors. Images can be used in different contexts, always having the same alt regardless of context may be wrong in some cases. But that's for the accessibility team to decide, just trying to think what else may be needed in the implementation :)

#75 @afercia
4 years ago

@azaozz thanks for your feedback.

I do realize you're considering this issue mainly from a technical perspective. In that vein, this ticket might be considered a "minor enhancement".

However

I'm not sure considering issues merely from a technical perspective and ignoring the practical consequences for the existence of this function is the best way to address this issue.

wp_get_attachment_image_src() is a function to retrieve the src of an image. Nothing more

The point is that WordPress provides a function that, in its practical usage, produces inaccessibile output.

Regardless of the original intent of this function or any documentation WordPress may provide to explain developers they have to add an alt attribute, this function can be (and is) abused.

This is also the reason why two experienced members of the accessibility team requested this function to be deprecated. See comments 38 and 39.

As @joemcgill and I mention above the current implementation is "not there yet"

This is unfortunate and, frankly, disappointing. Six months ago the team stated this ticket was "very close". I'm surprised it took six months to realize "it's not there yet" instead. This ticket was punted from 5.2 to 5.3 and should have been tackled at the very beginning of the 5.3 release cycle. It didn't happen. It's not a matter of taking a new technical direction (which took only a few days to be identified). It just didn't receive the necessary attention.

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


3 years ago

#77 @antpb
3 years ago

  • Milestone changed from Future Release to 5.9

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

#85 @audrasjb
2 years ago

  • Milestone changed from 5.9 to Future Release

As per today's bug scrub and since tomorrow is WP 5.9 feature freeze, I'm moving this ticket to Future release.

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


8 months ago

Note: See TracTickets for help on using tickets.