theme_image() alt attribute cannot be passed in $variables['attributes']

Doc: theme_image($variables)

Why do you want to pass alt in $variables['attributes']?

$variables = array(
  'path' => 'path/to/img.jpg', 
  'alt' => 'Test alt',
  'title' => 'Test title',
  'width' => '50%',
  'height' => '50%',
  'attributes' => array('class' => 'some-img', 'id' => 'my-img'),
);
$img = theme('image', $variables);

Definitely not major.

I think theme_image() works as designed. This is not a bug. Re-open if I'm wrong.

Related: #1025796: Rename path to uri in image functions

I think this is a valid WTF that's worth fixing in D8. There are good use-cases for wanting to pass all HTML attributes in $variables['attributes'], especially in the context of render arrays. Given D8's default html output will be html5, it may make sense to default 'alt' to NULL like the rest of them, or else remove those 4 keys from being special at all, and simply use 'attributes' exclusively.

For D7, I think we're stuck with this, because of the need for legacy compatibility, and coddling people towards valid html4/xhtml. See #555830: Clean up theme_image() to use drupal_attributes() for all attributes and revisit defaults for "alt" and "title" for background.

1) For D7, we want to replace the isset() with array_key_exists() to allow overriding the default of '' with NULL.

2) $variables vs. $variables['attributes'], we can fix, too.

For D8, we probably want to open a separate issue to remove the totally whack separated variables and only support 'attributes'. Or similar.

+++ b/includes/theme.inc
@@ -1512,9 +1512,17 @@ function theme_image($variables) {
+    if (array_key_exists($key, $variables)) {

This will always be TRUE, given the default in drupal_common_theme().

Given D8's default html output will be html5, it may make sense to default 'alt' to NULL like the rest of them, or else remove those 4 keys from being special at all, and simply use 'attributes' exclusively.

I would be a fan of using just attributes, although, using explicit variables is useful so that people can see the commonly used attributes.

I would be a huge fan of alt * not * defaulting to ''. If alt doesn't default to '' then the html5 validator will most likely throw a warning (unless it is one of the stupid exclusions the co-chairs have added to the spec). The warning was intentionally designed so that people would be made aware of the missing alt attribute on the image, but still allows the page to validate.

** note: this is from memory of the spec and associated discussions and things may have changed.

#6: drupal8.theme-image-attributes.6.patch queued for re-testing.

This is basically a re-roll. I didn't address @effulgentsia's concern in #8. Sounds like it should be just simplified and that if statement removed to simplify it.

#12: drupal8.theme-image-attributes.999338-12.patch queued for re-testing.

I'm bumping this up to major because it's inconsistent and because it restricts accessibility for no good reason. This can be fixed in D8.

This is not major as per #2

Remember, alt tags are the low hanging fruit of accessibility. That being said, this should be easier.

#12: drupal8.theme-image-attributes.999338-12.patch queued for re-testing.

This simplifies it based on @effulgentsia - but I do agree with Everett that defaulting to alt="" will make it harder to find broken alt text. That should be a conscious choice made every time.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

Not sure about removing attributes if they arn't width, height, alt, or title. What if we want a data- attribute on an image?

If we want to add data then it would have to be added to this array first - array('width', 'height', 'alt', 'title')

This only unsets the variables defined with the array above.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

This is a very simple patch. Also the test results were filled up with things like : mkdir(): No space left on device

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

This failed last time with:

Link with label Test Operation: ;::]A$7A found in EntityOperationsTest.php Line 54

Not sure hwo this would affect it:
$this->assertLinkByHref($uri['path'] . '/test_operation');

The tests look like they pass again now on the patch in 21.

#12: drupal8.theme-image-attributes.999338-12.patch queued for re-testing.

#21: drupal8.theme-image-attributes.999338-21.patch queued for re-testing.

Fixed patch to reference core/includes/theme.inc rather than includes/theme.inc

44: theme_image_alt_attribute-999338-44.patch queued for re-testing.

reroll.

Let's get this slippery customer tested again before it needs another reroll!

We need a clear test to manually test this.

We're looking into this at DrupalCon Austin.

Working with Katie, verified the problem. This module provides working and non-working (in attribute array) examples: https://drupal.org/sandbox/mlncn/2281847

The first render array displays its alt text, the second does not:

    $build['image_with_alt_working'] = array(
      '#theme' => 'image',
      '#uri' => '/core/themes/bartik/logo.png',
      '#alt' => 'Test alt',
      '#title' => 'Test title',
      '#width' => '50%',
      '#height' => '50%',
      '#attributes' => array('class' => 'some-img', 'id' => 'my-img'),
    );
    $build['image_with_alt_attribute_not_working'] = array(
      '#theme' => 'image',
      '#uri' => '/core/themes/bartik/logo.png',
      '#width' => '50%',
      '#height' => '50%',
      '#attributes' => array(
        'class' => 'some-img',
        'id' => 'my-img',
        'title' => 'New test title',
        'alt' => 'New test alt',
      ),

The attached patch from comment #53 does not change anything (a nice self-contained argument that this issue needs tests).

In Drupal 8, instead of making alt a special case which is pre-set to empty string because it is required, we should pre-set it to NULL like title, uri, width, and height. We no longer use an XHTML 1.0 or HTML 4 doctype and there is no need to complicate our code attempting to adhere to the letter, not the spirit, of accessibility to conform to its entirely unenforced .

This comment appears twice in our code and can be removed entirely along with its ensuing complication.

// HTML 4 and XHTML 1.0 always require an alt attribute. The HTML 5 draft
// allows the alt attribute to be omitted in some cases. Therefore,
// default the alt attribute to an empty string, but allow code calling
// _theme('image') to pass explicit NULL for it to be omitted. Usually,
// neither omission nor an empty string satisfies accessibility
// requirements, so it is strongly encouraged for code calling
// _theme('image') to pass a meaningful value for the alt variable.
// - http://www.w3.org/TR/REC-html40/struct/objects.html#h-13.8
// - http://www.w3.org/TR/xhtml1/dtds.html
// - http://dev.w3.org/html5/spec/Overview.html#alt

The html5 specification (that last link) properly makes no distinction between a missing and an empty alt text:

"Except where otherwise specified, the alt attribute must be specified and its value must not be empty".

Even for xhtml 1.0, do any validators people use make this distinction? Could we get rid of this in 7 also? If not, i'm happy to make two different patches, one that's almost all removing silliness for 8, and a workaround of it for 7. But dropping the pre-set to empty string is what ezufelt calls for in comment #9.

This patch fixes the problem that alt cannot be passed in with $variables['attributes'] by removing the misguided attempt to set it to "" if it isn't present. Test forthcoming.

@mlncn I don't see how the patch in #10 resolves this problem.

HTML5 specs & WCAG 2.0 AA are different things. Our goal as a community is both, but for accessibility it is the latter.

The problem isn't as much of it coming up with alt="" as much as it not coming up with alt="New test alt" - to use your earlier example.

Good catch on the duplicate help text. There should be some reference though as it's 1000 lines of code after.

Hi.
I followed the basic Test Driven Development method to solve the problem.

First, I created an automated test based on @mlncn's #56 report/sandbox, which is included in my patch.

Then I created the minimal ammount of code that makes the test to pass. At least the tests in Drupal\image\Tests\ImageThemeFunctionTest do not break. Let's see what Drupal TestBot thinks...

So, my sollution solves the issue but does not take part in the wider discussion about should the shortcut variable alt default to NULL instead of ''.

Removing "Needs tests" tag as the automated tests have been implemented.

Patch looks good!

We still need to implement a separated test patch which proves us that the tests are working. More information about Automated tests.

Here you are. I also refactored the tests as well.

I updated the commentings a bit.

@lauriii - Did you also "implement a separated test patch which proves us that the tests are working"?

Just wondering how close we're getting to RTBC and what other steps are required. Nice to see the bots like your patch.

@mgifford - I didn't create separated test patch because there's one available in comment #65. Do you think it's enough to prove the tests are working?

I've got a test environment up here - http://s22557ad640c574b.s2.simplytest.me/

Although that's not all that useful in testing this.. I'll try to get some feedback on what would be required to prove the tests are working...

I'm going to test this

By using the sandbox module referenced in comment #56 the error can be reproduced, while the first image has the correct ALT text applied, the second image has an empty ALT text:

The patch from comment #68 corrects the problem as now also the second image has the correct ALT text applied:

Doc 80 character fix and micro optimization:
@see http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

Also moved it into the existing IF for further optimization so it doesn't need to check the NULL array key if there is no value to put in there in the first place.

Summary: isset() is super fast! Like 65 times faster than array_key_exists(). So do that isset() check first.

Typo, that patch is not right.

Confirming like in #73 that patch at #75 also corrects the problem, the second image also has the correct ALT text applied:

1. +++ b/core/modules/image/src/Tests/ImageThemeFunctionTest.php
   @@ -152,4 +152,41 @@ function testImageStyleTheme() {
   +    // Test using alt directly with alt attribute.
   +    $image_with_alt_working = array(
   ...
   +    $elements = $this->xpath('//img[contains(@class, class) and contains(@alt, :alt)]', array(":class" => "image-with-regular-alt", ":alt" => "Regular alt"));
   +    $this->assertEqual(count($elements), 1, 'Regular alt displays correctly');
   ...
   +    // Test using alt attribute inside attributes.
   +    $image_with_alt_attribute_not_working = array(
   ...
   +    $elements = $this->xpath('//img[contains(@class, class) and contains(@alt, :alt)]', array(":class" => "image-with-attribute-alt", ":alt" => "Attribute alt"));
   +    $this->assertEqual(count($elements), 1, 'Attribute alt displays correctly');
   

   This is pretty minor, but did confuse me in reading the test. $image_with_alt_attribute_not_working then later has an assertion that says the alt attribute actually IS working? I think we mean something there like:

   $image_with_alt_property

   and

   $image_with_alt_attribute

   Yes?

2. +++ b/core/includes/theme.inc
   @@ -1125,6 +1125,11 @@ function template_preprocess_image(&$variables) {
   +      // If the property has already been defined in the attributes,
   +      // do not override, including NULL.
   +      if (isset($variables['attributes'][$key]) || array_key_exists($key, $variables['attributes'])) {
   +        continue;
   +      }
   

   And then additionally, I don't actually see tests for this condition.

Fixed variable names and added test for #77.2

Awesome! Thanks for test only patch!

+++ b/core/includes/theme.inc
@@ -1126,6 +1126,11 @@ function template_preprocess_image(&$variables) {
+      if (isset($variables['attributes'][$key]) || array_key_exists($key, $variables['attributes'])) {

This looks like an unnecessary micro-optimisation we'd have to be doing thousands of these per request to notice the difference. Why not just if (array_key_exists($key, $variables['attributes'])) {

<?php
class NonExistant
{
  protected $associativeArray = array(
    'one' => 'one',
    'two' => 'two',
    'three' => 'three',
    'four' => 'four',
    'five' => 'five',
    'six' => 'six',
  );

  protected $numIterations = 1000;
  
  public function issetTest()
  {
    for ($i = 0; $i < $this->numIterations; $i++) {
      isset($this->associativeArray['none']);
    }
  }

  public function arrayKeyExistsTest()
  {
    for ($i = 0; $i < $this->numIterations; $i++) {
      array_key_exists('none', $this->associativeArray);
    }
  }
}

$class = new NonExistant();

foreach (array('issetTest', 'arrayKeyExistsTest') as $func) {
  $time_start = microtime();
  $class->$func();
  $time_end = microtime();
  $time = $time_end - $time_start;

  echo "Did $func in $time seconds\n";
}

Outputs:

Did issetTest in 0.00011900000000004 seconds
Did arrayKeyExistsTest in 0.00024799999999997 seconds

So it is twice as slow as something that is really really fast - so it is still fast. Also see http://3v4l.org/hPpbU for comparison accross PHP versions.

I removed that unnecessary isset.

@alexpott regarding the micro-optimization. With a preprococess that gets called as much as this does, it is worth it.
see #74 for why I put it in. Also this optimization is in core in other places and I think @jessebeach walked me through it and convinced me it was the better way to IIRC.
From the article:

array_key_exists() is almost 5 times slower than isset()

re #81 I must have not seen those stats or you added them after, I did similar tests here and kinda came to the same conclusion, it's minimal.

The only thing that the isset() || array_key_exists() really does it make you ask your self as a developer, do you really need to have NULL keys because isset() does the exact same thing except it is FALSE on NULL.

I like the pattern because it highlights that decision.
Anyways, this is RTBC. Grievances aired, optimization minimal.

Committed 6d8e333 and pushed to 8.0.x. Thanks!