[REGRESSION] Replace theme() with drupal_render() in picture module

We are working today with this issue during Code Sprint UA.

Patch attached.

#2 looks good
RTBC

+++ b/core/modules/picture/picture.moduleundefined
@@ -142,7 +142,7 @@ function picture_theme() {
-        'path' => NULL,
+        'uri' => NULL,

@@ -200,7 +206,11 @@ function theme_picture_formatter($variables) {
   if (!isset($item['path']) && isset($variables['uri'])) {
     $item['path'] = $variables['uri'];
   }

Are you sure? Seems like we have both path and uri...

#2: picture-replace_theme_with_drupal_render-2009662-1.patch queued for re-testing.

expecting this needs a reroll, tagging.

Bringing this back up to speed with reroll.

Reroll to current head and removed out of scope changes. Fixed everything in #5 EXCEPT:

I did not change this part because I did not understand...sorry

-  $output = theme('picture', $item);
+  $picture = array('#theme' => 'picture');
+  foreach ($item as $key => $variable) {
+    $picture["#{$key}"] = $variable;
+  }
+  $output = drupal_render($picture);

-      $output[] = theme('picture_source', $source);
+      $picture_source = array('#theme' => 'picture_source');
+      foreach ($source as $key => $variable) {
+        $picture_source["#{$key}"] = $variable;
+      }
+      $output[] = drupal_render($picture_source);
     }

Please use https://drupal.org/node/2006152#comment-7487406 rather than https://drupal.org/node/2006152#comment-7487410 if possible.

#11 failing due to moving out of scope changes to this issue - Undefined index: uri and Undefined index: dimensions in picture.module

I'm getting some weird results here... I started by trying to work on the patch in #11, and I couldn't get pictures to render at all, so i returned to the patch in #2 but realised that never even passed tests, so I started over.

It appears that when we change

$output = theme('picture', $item);

to

  $picture = array(
    '#theme' => 'picture',
    '#uri' => $item['uri'],
    '#width' => $item['width'],
    '#height' => $item['height'],
    '#alt' => $item['alt'],
    '#style_name' => $item['style_name'],
    '#breakpoints' => $item['breakpoints'],
  );
$output = drupal_render($picture);

everything goes all snafu with the image URI. Instead of getting ...sites/default/files/styles/large/public/field/image/IMG_1123.jpg?itok=E4nZ42Bn like it should, calling drupal_render gets a URI of ...sites/default/files/styles/large/public?itok=yQEzZDg9

The only difference I can see between $picture and $item is that $item had a key of entity. I tried setting a #entity key on the picture - just as a longshot - but it doesn't seem to have any effect.

Here's a patch if anyone else wants to play with this. Please note, I've commented out some lines in here that you can use for testing the theme -> drupal_render issues, but they will need to be removed eventually.

Changed status for run tests

Rerolled #15

Removed some debug code from #18 and run test

Remember, this should keep failing until #13 is completed. This was out of scope for this issues and was moved. See also #4 and #5. It was my understanding that #2 test passed the first time but someone re-tested and then failed due to needing a re-roll.

Like @pwieck said, this is stuck on #2026319: Mis-named variables in picture_theme().

Once that goes through, this slightly modified patch should cover it:

#2026319: Mis-named variables in picture_theme() is in.

Here's the re-rolled patch.

Updated issue summary.

Adding how to test.

It works! Also, I added a How To Test section.

There seems to be considerable overlap with #1898442: responsive_image.module - Convert theme_ functions to Twig... personally I think the issues should be merged - will leave it to the twig team to decide what to do. Afaics #1898442: responsive_image.module - Convert theme_ functions to Twig converts more to twig...

Setting to "needs review" so people know there's a decision to make :)

I think this needs to happen regardless of what happens in #1898442: responsive_image.module - Convert theme_ functions to Twig.

All instances of theme() need to be removed somehow, and conflicts are easily resolved. Trying to figure out which conversions to do in which issue is only going to waste time, in my opinion.

Happy to do it.

Committed e21ba43 and pushed to 8.x. Thanks!

I just saw this in the commit log. I'm assuming that being able to leave out the alt attribute here is as valid an option for theme_picture_formatter() as for theme_image_formatter() so I guess this will need the same repair work as is being done now for theme_image_formatter() in #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter().

Here's a patch that fixes the regression introduced here the same way as in #50 from #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter() but without test assertions. Does someone familiar with (the output from) theme_picture_formatter() and theme_picture() feels like copying and adapting the alt parts from core/modules/image/lib/Drupal/image/Tests/ImageThemeFunctionTest.php into methods testPictureFormatterTheme() and testPictureTheme() in a new file core/modules/picture/lib/Drupal/picture/Tests/PictureThemeFunctionTest.php?

Missed one in theme_image_formatter().

Looks good to me. Not sure that the new tests necessarily need to be done here.

Since #34 is a straight up bug fix, and according to #33, a regression caused by the earlier refactor here, then we do need tests here to prevent a similar regression happening next time this code is refactored.

working on tests.

The tests should pass, but they don't. The fix is still including an alt attribute to the picture element whether its null or not. The title attribute is included in both cases as well. The title attribute if null shouldn't be there anyways, since it's not required by html standards.
Here is prior to #34

<picture>
<!-- <source src="http://drupal8.dev/sites/default/files/simpletest/320040/styles/test/public/image-test_0.png?itok=dbE64R10"  width="" height="" /> -->
<source src="http://drupal8.dev/sites/default/files/simpletest/320040/styles/test/public/image-test_0.png?itok=dbE64R10"  width="" height=""/>
  <noscript><img class="image-style-test" src="http://drupal8.dev/sites/default/files/simpletest/320040/styles/test/public/image-test_0.png?itok=dbE64R10" alt="" /></noscript>
</picture>

and with fix from #34

<picture alt="" title="">
<!-- <source src="http://drupal8.dev/sites/default/files/simpletest/542539/styles/test/public/image-test_0.png?itok=MKxODVdw"  width="" height="" /> -->
<source src="http://drupal8.dev/sites/default/files/simpletest/542539/styles/test/public/image-test_0.png?itok=MKxODVdw"  width="" height=""/>
  <noscript><img class="image-style-test" src="http://drupal8.dev/sites/default/files/simpletest/542539/styles/test/public/image-test_0.png?itok=MKxODVdw" /></noscript>
</picture>

I worked on the tests with @travis-echidna. I suspect getting a passing patch is not a Novice task at this point, removing tag. @Eric_A if you are able to pick this up again that would be very helpful.

We suspected this might be part of the problem with the empty title attribute always being added:

+++ b/core/modules/picture/picture.module
@@ -279,7 +300,7 @@ function theme_picture($variables) {
     foreach (array('alt', 'title') as $key) {
-      if (isset($variables[$key])) {
+      if (isset($variables[$key]) || array_key_exists($key, $variables)) {

Indeed.

Reverted part of the code in #34, because while (NULL) values should pass through to other theme hooks, once the actual rendering takes place the NULL values are there to be ignored when building the attributes.

Fixed test: we're re-using part of the element and therefor render() instead of drupal_render(), just like in ImageThemeFunctionTest.

I reviewed the test and there is still some work to do:

A test for theme_picture_formatter() is needed.

+  function testPictureThemeFunction() {

Compared to the test method names in ImageThemeFunctionTest this is a change in the wrong direction I think. "Function" is not correct anymore when the function is converted to a template. Better to stay consistent and use testPictureTheme. (We might change the class names too at some point, but not here.)

I forgot to attach the test-only patch. Note the difference in fails.

Gotcha.

@travis-echidna, can you take it from here?

We did have the alt removal right at one point :) thanks for spotting that @Eric_A.

I had a look at the test and it seems that rendered element is returning:

<picture>
<!-- <source src="http://d8.localhost/sites/default/files/simpletest/610332/styles/test/public/image-test_0.png?itok=80jIozhS"  width="" height="" /> -->
<source src="http://d8.localhost/sites/default/files/simpletest/610332/styles/test/public/image-test_0.png?itok=80jIozhS"  width="" height=""/>
  <noscript><img class="image-style-test" src="http://d8.localhost/sites/default/files/simpletest/610332/styles/test/public/image-test_0.png?itok=80jIozhS" alt="" /></noscript>
</picture>

While the expected result is

  <img class="image-style-test" src="http://d8.localhost/sites/default/files/simpletest/610332/styles/test/public/image-test_0.png?itok=80jIozhS" /> 

So it appears the expected result assumes a missing alt attribute while the rendered version simply has an empty alt attribute.

Wait, this seems to be fine, right? The test-only patch should fail and the normal patch should come out fine? So nothing is the matter?

Marking this as needs review, not needs work.

Removing `needs tests` tag.

Still needs a test for theme_picture_formatter(). See #41 and #33.

Doh! Missed that. Thanks, @Eric_A.

I've apply #45 patch and added a new test for theme_picture_formatter().

Looks good to me!

Kudos to jeanfei for looking into this and adding assertions. If everything is well with both the alt changes for theme_picture_formatter() and the new assertions then in a test-only patch the alt === NULL related ones will fail and the bonus assertions will pass.

#56: drupal8.theme-system.2009662-54-test-only.patch queued for re-testing.

The test-only patch did not fail on "theme_picture_formatter() correctly renders with a NULL value for the alt option.".
I took another look at the code and concluded that the change I made tries to fix the scenario with breakpoints involved. So we need to add an assertion with breakpoints involved regarding theme_image_formatter()theme_picture_formatter().

@jeanfei, do you feel like finishing up? (If not, then it's best to unassign.)

I also spotted a documentation issue.

+  /**
+   * Tests usage of the theme_picture_formatter() function.
+   */
+  function testPictureFormatterThemeFunction() {
+    // Create an image.
+    $files = $this->drupalGetTestFiles('image');
+    $file = reset($files);
+    $original_uri = file_unmanaged_copy($file->uri, 'public://', FILE_EXISTS_RENAME);
+
+    // Create a style.
+    $style = entity_create('image_style', array('name' => 'test', 'label' => 'Test'));
+    $style->save();
+    $url = $style->buildUrl($original_uri);
+
+    // Test using theme_picture() with a NULL value for the alt option.

This is testing theme_picture_formatter(), not theme_picture().

The test coverage may be just good enough for now for our purposes, when taking into account that for the breakpoints case the current implementation code delegates to theme_picture() which has coverage. Looking into that now.

The input in #38 and #48 by @travis-echidna and @c4rl is the kind that will help this issue further.

What is the expected markup for a suitable breakpoint ($variables['breakpoints'] as $breakpoint_name => $multipliers; $multipliers as $multiplier => $image_style)?

I'd be happy to add the test coverage for that in testPictureFormatterThemeFunction().

We need that because the fact of the matter is that the current incarnation of the added testPictureFormatterThemeFunction() is not testing the fix in theme_picture_formatter() and thus will not prevent us from regressing again. From this point of view I would suggest to postpone other refactorings of the picture module (like #1898442: responsive_image.module - Convert theme_ functions to Twig) on this issue.

From this point of view I would suggest to postpone other refactorings of the picture module (like #1898442: picture.module - Convert theme_ functions to Twig) on this issue.

I'm taking this back, because the regression here should not be another issues problem. We'll just have to reroll here if other refactorings get in first.

Doc fix and more precise assertion message as per #58. Needs review to let the bot test again.

#63: drupal8.theme-system.2009662-63.patch queued for re-testing.

Editing How To Test section.

1. +++ b/core/modules/picture/picture.module
   @@ -189,7 +201,7 @@ function theme_picture_formatter($variables) {
   +  if (isset($variables['item']['alt']) || array_key_exists('alt', $variables['item'])) {
   

   This bit is redundant.

2. +++ b/core/modules/picture/picture.module
   @@ -309,7 +330,7 @@ function theme_picture($variables) {
   +      if (isset($variables[$key]) || array_key_exists($key, $variables)) {
   

   same here.

3. +++ b/core/modules/picture/picture.module
   @@ -214,7 +226,16 @@ function theme_picture_formatter($variables) {
   + *   - alt: The alternative text for text-based browsers. 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, this variable defaults
   + *     to an empty string, but can be set to NULL for the attribute to be
   + *     omitted. Usually, neither omission nor an empty string satisfies
   + *     accessibility requirements, so it is strongly encouraged for code calling
   + *     theme('picture') to pass a meaningful value for this 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
   
   @@ -122,6 +122,18 @@ function picture_mapping_load($id) {
   +      // default.
   ...
   +      // 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('picture') 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('picture') 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 title attribute is optional in all cases, so it is omitted by
   

   This sounds a bit to heavy and duplicated to be of much use. Also it's advocating calling theme() directly which we are trying to avoid.

The tests look nice. Good work @Eric_A

The "redundant" part is a pattern for performance used by core in more places.
I don't think there is a way right now to get around the duplicating of the docs. It's already in core in more places that use these attributes this way.

The most important thing right now is for somebody to respond to what I said in #61.

What is the expected markup for a suitable breakpoint ($variables['breakpoints'] as $breakpoint_name => $multipliers; $multipliers as $multiplier => $image_style)?

I'd be happy to add the test coverage for that in testPictureFormatterThemeFunction().

I've added #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter() to the related issue. It has all the reasons for why everything is done exactly this way. (RTBC'd by @pplantinga and committed by @alexpott).

@Eric_A the performance bit is the isset(), and isset is used to help shortcut out of the condition quicker. That was what it was by default, no need to also check if the key exists because the isset() does just that.

No idea on the breakpoint question.

The big alt comment is hitting people over the head with information they should already know and doesn't directly explain why or what is happening in the code directly beneath it. Better off finding one good place, likely in a docblock someplace to add that comment once and be done with it.

@Eric_A

Not sure I'm 100% understanding the question you're asking about breakpoints, but here's what I've found...

in theme_picture_formatter looks like $variables['breakpoints'] is an array in the structure like this:

[theme.bartik.mobile] => Array
        (
            [1x] => thumbnail
        )

    [theme.bartik.narrow] => Array
        (
            [1x] => medium
        )

    [theme.bartik.wide] => Array
        (
            [1x] => large
        )

While the output created looks like this:

<picture alt="Squarer image">
<!-- <source src="http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/imagefield_0K4E4J.jpg?itok=3qQUH6kn"  width="75" height="100" /> -->
<source height="100" width="75" src="http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/imagefield_0K4E4J.jpg?itok=3qQUH6kn"></source>
<!-- <source media="(min-width: 0px)" src="http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/imagefield_0K4E4J.jpg?itok=3qQUH6kn"  width="75" height="100" /> -->
<source height="100" width="75" src="http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/imagefield_0K4E4J.jpg?itok=3qQUH6kn" media="(min-width: 0px)"></source>
<!-- <source media="all and (min-width: 560px) and (max-width: 850px)" src="http://d8.dev/sites/default/files/styles/medium/public/field/image/imagefield_0K4E4J.jpg?itok=Yaa6-FoI"  width="164" height="220" /> -->
<source height="220" width="164" src="http://d8.dev/sites/default/files/styles/medium/public/field/image/imagefield_0K4E4J.jpg?itok=Yaa6-FoI" media="all and (min-width: 560px) and (max-width: 850px)"></source>
<!-- <source media="all and (min-width: 851px)" src="http://d8.dev/sites/default/files/styles/large/public/field/image/imagefield_0K4E4J.jpg?itok=K_fEyMnG"  width="358" height="480" /> -->
<source height="480" width="358" src="http://d8.dev/sites/default/files/styles/large/public/field/image/imagefield_0K4E4J.jpg?itok=K_fEyMnG" media="all and (min-width: 851px)"></source>
  <noscript>&lt;img class="image-style-thumbnail" typeof="foaf:Image" src="http://d8.dev/sites/default/files/styles/thumbnail/public/field/image/imagefield_0K4E4J.jpg?itok=3qQUH6kn" width="75" height="100" alt="Squarer image" /&gt;</noscript>
<img height="480" width="358" alt="Squarer image" src="http://d8.dev/sites/default/files/styles/large/public/field/image/imagefield_0K4E4J.jpg?itok=K_fEyMnG"></picture>

Sorry if this doesn't help, maybe it will be a nudge for someone more experienced. :)

Thanks for the info, @mariacha1.

Note that things have changed now because of #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline. The diff there makes me think that the bug fixing here may have gotten easier; now may be the time to move the assertions that are not related to this specific bug into their own issue.

The previous title no longer applies. theme() is no longer invoked in the Picture module.

@jessebeach, are you saying that the regression does not exist anymore, possibly as a side effect of #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline? If so, please share some information here. For now I'm changing the title to what it should have been since #31. The point of quickly reopening this issue in #31/#33 was to fix a regression introduced by the conversion. Which was the same regression that had just happened in #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter(). If it was just about docs and adding test coverage it would not have made much sense to keep this issue open.

Sorry, I hadn't realized this issue concerned fixing a regression. I was going off the title and the fact that grep -r -E 'theme\(' ./core/modules/picture wasn't turning up any theme function invocations. I might suggest in the future that we open a followup bug for a regression rather than tacking on to a committed issue. But that's not really important now, since you largely have the regression addressed here. I'll give your patch a review. Thank you for staying on top of this.

Here's an article that expounds on the performance pattern that Eric_A mentions in #67: http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

tl:dr $arr = array('myKey' => NULL); isset($arr['myKey']); //returns false even though the key is technically set.

Here's the HTML for a picture element:

<picture alt="">
<!-- <source src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t"  width="100" height="99" /> -->
<source src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t" width="100" height="99">
<!-- <source media="(min-width: 0px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t"  width="100" height="99" /> -->
<source media="(min-width: 0px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t" width="100" height="99">
<!-- <source media="all and (min-width: 560px) and (max-width: 850px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/medium/public/field/image/dog-walker.jpg?itok=kK0L7RML"  width="220" height="218" /> -->
<source media="all and (min-width: 560px) and (max-width: 850px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/medium/public/field/image/dog-walker.jpg?itok=kK0L7RML" width="220" height="218">
<!-- <source media="all and (min-width: 851px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/large/public/field/image/dog-walker.jpg?itok=RXZsaNWI"  width="480" height="475" /> -->
<source media="all and (min-width: 851px)" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/large/public/field/image/dog-walker.jpg?itok=RXZsaNWI" width="480" height="475">
  <noscript>&lt;img class="image-style-thumbnail" typeof="foaf:Image" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t" width="100" height="99" alt="" /&gt;</noscript>
<img alt="" src="http://d8.drupal.dev/sites/d8.drupal.dev/files/styles/thumbnail/public/field/image/dog-walker.jpg?itok=y2N0Wm0t" width="100" height="99"></picture>

Personally, I would expect the alt attrs not to print if they're empty.

PictureFieldDisplayTest tests that the HTML for the formatter display is rendering properly.

I'm not 100% certain what exactly the regression is. Eric_A, is it just that the tests weren't passing?

In any case, I've rerolled the patch so that the additional tests pass. I used ImageThemeFunctionTest as a guide.

+++ b/core/modules/picture/picture.module
@@ -269,7 +290,7 @@ function theme_picture($variables) {
+      if (isset($variables[$key]) || array_key_exists($key, $variables)) {

@@ -299,7 +320,7 @@ function theme_picture($variables) {
+      if (isset($variables[$key]) || array_key_exists($key, $variables)) {
...
-      if (isset($variables[$key])) {

@@ -269,7 +290,7 @@ function theme_picture($variables) {
-      if (isset($variables[$key])) {

This is not necessary, the isset() is the performance part, the || array_key_exists() does the same thing again if the first condition is true, making it slower.

+++ b/core/modules/picture/picture.module
@@ -204,7 +216,16 @@ function theme_picture_formatter($variables) {
+ *   - alt: The alternative text for text-based browsers. 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, this variable defaults
+ *     to an empty string, but can be set to NULL for the attribute to be
+ *     omitted. Usually, neither omission nor an empty string satisfies
+ *     accessibility requirements, so it is strongly encouraged for code calling
+ *     theme('picture') to pass a meaningful value for this variable.

Maybe this is a silly idea or I'm missing something (it would be an API change) but what about using FALSE instead of NULL to omit the alt attribute? Seems more intentional and easier to test for.

nm #76, the array_key_exist is checking for NULL so value can be omitted. @Cottser's right. Thanks for the link @Cottser http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

The isset() is for performance like I said, though the array_key_exists will allow NULL values through the condition.

The link was from @jessebeach :)

Ah, sorry @jessebeach didn't see that link earlier.

@Eric_A want to give #75 a go to see if it solve the regression you are mentioning?

So I hate to rain on the parade of all the work that has been done on this issue, but looks as if this is about outputting an alt attribute on the picture element, but there shouldn't be an alt attribute on the picture element. The alt attribute goes on the img element inside of the picture element.

The specification for the picture element has changed in the last couple months, so it's possible alt was on picture in the past, but that's no longer the case.

Here's the current spec: http://picture.responsiveimages.org

I misread this patch. Took a closer look, and I can see now that part of the tests relate to having alt on the picture element (which has now been removed in #2211831: Removal of alt attribute from [picture] tag). The other tests to relate to the img element within the picture element, which are still valid.

So this patch needs to be rerolled, also because of #2124377: Rename "Picture" module to "Responsive Image" module.

Once that rerolling is done, then I guess we can test the use of FALSE instead of NULL. I do wonder if NULL was used in #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter(), why we'd use FALSE here instead. It seems confusing to use two different methods. If FALSE is faster, that's great, but then we should change the image module to the FALSE method too, after this.

There was a commit here in July 2013 and this issue is still open. This is a great example of why re-opening issues after commit is so confusing (even moreso to newcomers to the issue queue) and opening a follow-up issue is 99.9% of the time the best thing to do (hint: Dreditor gives you a clone issue button).

I agree with @mdrummond that if we use FALSE to not show the empty alt="" we should be consistent about it, and I would suggest that API change could be handled in a separate issue to keep focus here and close this issue out as soon as possible.

Working on rerolling the test. I think that means that we add to this existing test.

I'm working on the reroll to join the patch #85 with #75

I talked with @lokapujya via IRC and he told me "..it should have been opened as a new issue, instead of trying to fix the regresssion in the already applied patch."

I talked personally with @yesct and she proposed opening a new issue (agreeing with @lokapujya), so we are opening #2425493: Allow picture to not have a 'tag' attribute and add tests for responsive_image module. so we can apply the patches to reponsive_image core module.

The patches that we are moving from here are #75 and #85 so they can be re-rolled.

I'm rolling back to the original state when this issue was a normal task in #30.