Convert theme_image() to Twig

I don't know of any blockers for moving this issue forward. I'll take it.

Here is my first pass at converting this to twig. Tested with theme('image', args). My main question with these changes is the DocBlock in image.html.twig, as this patch only documents the attributes array and not all the possible values it could contain. I am not sure what is actually desired there.. Also, this issue could be affected by the outcome of #1808254: Standardize and simplify the attribute syntax in Twig template files.

Hmmm. Not sure why this didn't pass the testbot. Will look into it.

By how I am reading 1st failed test and from my debugging a class="image" is getting added to all images built from theme('image'). Not sure where this class is coming from exactly... By doing a dump of $variables at the beginning of template_preprocess_image() I am getting a class of image under the attributes key in the array, even when no class has been passed through theme('image', args)??

Example:

[uri] => my_image.png
    [width] => 
    [height] => 
    [alt] => 
    [title] => 
    [attributes] => Array
        (
            [class] => Array
                (
                    [0] => image
                )

        )

............

I think the reason why it's failing is because of your image template

It should be like so:

<img src="{{ attributes.src }}" class="{{ attributes.class }}" {{ attributes }} />

At least according to the comment here:

Re #6 - Thanks for the feedback, chrisjlee. I am using the Attributes class in template_preprocess_image() to handle building all the attributes for the image, and the width and height for images are getting passed correctly. The test results for Drupal\image\Tests\ImageDimensionsTest indicate that it is not the image dimensions that are different - but rather the class in place.

EDIT: I asked jenlampton in irc the other day what I should do here and I believe she said to just use {{ attributes }} for now. But I may have not given the proper context when I asked what to do. I don't believe passing {{attributes.class}} will resolve the issue since the unwanted class name is already in preprocess before it hits the template. But I will try it and run the test locally.

Thanks cottser. I will check out that line in template_preprocess(). But from further testing I've don locally, I am not sure it is related to the class anymore. Even when I add that class to the tests search string it still doesn't pass.

I have worked through this locally and can't seem to get it to pass the test's. I suspect that it is whitespace related, but I am not positive. Would love feedback from someone who has more experience groking the testbots messages.

+++ b/core/includes/theme.inc
@@ -1843,7 +1843,9 @@ function theme_dropbutton_wrapper($variables) {
 /**
- * Returns HTML for an image.
+ * Prepares variables for an image.
+ *
+ * Default template: image.html.twig.
  *
  * @param $variables
  *   An associative array containing:
  *   - attributes: Associative array of attributes to be placed in the img tag.
  */
-function theme_image($variables) {
+function template_preprocess_image(&$variables) {
   $attributes = $variables['attributes'];
   $attributes['src'] = file_create_url($variables['uri']);

-  return '<img' . new Attribute($attributes) . ' />';
+  $variables['attributes'] = new Attribute($attributes);

DocBlock change, first line should read:

"Prepares variables for image templates."

And to get rid of the 'image' class:

$attributes['class'] = array_diff($attributes['class'], array('image'));
$variables['attributes'] = new Attribute($attributes);

And add a comment why we're doing this.

That should probably do it for preprocess.

For the template:

1. Yes I think 'attributes' is enough for this
2. Some grammar changes:

"Default theme implementation of an image."

Available variables:
- attributes: "An array of HTML attributes for the img tag."

That's about all I see. The removal of the 'image' class should have tests pass.

...oh, and butt {{ attributes }} up to img, like this:

<img{{ attributes }}/>

Thanks for the feedback, steveoliver! I will make those grammar changes in the comments. I will also make that array_diff() addition and add test locally before pushing up.

Here is a patch that makes the changes suggested in #11 & #12. I believe it is still failing the theme functions teste. But I wanted to attach my latest changes here to get any feedback of the latest work.

As I expected the ImageThemeFunctionTest didn't pass. I was however not anticpiating the TourTest still fail. And since assertRaw() doesn't return back the value found vs. what it expected I am not really sure what's occurring.

Nice work @tlattimore! So close:)

The TourTest probably fails because it's doing a test expecting the first attribute as src. You could change that test to an xpath() test to find the attribute and it's value.

You can remove the 'array' bit from the twig docblock:

- *   - attributes: Associative array of attributes to be placed in the img tag.
+ *   - attributes: An array of HTML attributes for the img tag.

convert to

- *   - attributes: Associative array of attributes to be placed in the img tag.
+ *   - attributes: HTML attributes for the img tag.

This is my 1st crack at at the xpath() change suggested int #17. I have never implemented any of this, so it could be done completely wrong. Also updated the comment as requested in #17.e

Bot, do your thing.

@tlattimore That was a good try:)

Lost one fail, gained two exceptions. Are you running the tests with the 'testing' module on your development machine? If not that could help see things error out quicker and if you see that Tour is failing, you can have it just run that test only and save a bit of time.

I think you were close but kind of changed the original test's purpose. It was intended to find that image and you have it not finding the image. Cool that you used the params of xpath though, I didn't even know those existed! so I got to learn something too:)

Try more along these lines:

    $elements = $this->xpath('//img[@src="http://local/image.png"]');
    $this->assertEqual(count($elements), 1, 'Image plugin tip found.');

Think of xpath, like a weird looking jquery selector. Equvelent to
$elements = $('img[src="http://local/image.png"]'); if ($elements.length == 1) { console.log('Image plugin tip found.') }

The other errors are in ImageThemeFunctionTest.php and I would expect because the "expected" results expect the attribute order in a specific order and that's why it fails. So xpath would indeed help there as well. For both the arguments that should be there and the arguments that should not be there.

Let me know if you need help there, ping me on IRC with questions.

Thanks a lot joelpittet. I have been running tests locally a lot but I didn't with this one, dont know why. Thanks a a lot for your suggestions. I will try them out and also work on xpath() with the ImageTheme tests that are failing. Thanks for the feedback!

I have gotten the tour related test to pass for this now, but still having trouble on the mageThemeFunction tests.

@tlattimore post what you think the tests should be after you replaced them with xpath in the comments or as a patch, I'll see if I can point you in the right direction. Or if you would rather I can rewrite the those tests and be more tag team on this one.

joelpittet: If you are still willing to tag team on this I would greatly appreciate it. I just can't get the ImageThemeFunction tests to pass... Here is is my latest 'stable' work. I got hardly anywhere close with the ImageThemeFunction tests, so I didn't even include it.

Un-assigning to me since I am stuck on the last set of tests that aren't passing.

Xpathified image tests, @tlattimore tag, your it;)

Ahh... drupalSetConten() was the key part I was I was missing to get that test to pass! Thanks, joelpittet!

I personally think this is ready for RTBC as I don't think there is anything left to be done here. Anyone else?

just trying to move things along. Docs tweaks.

Made a mistake.

Thanks for working on this, chrisjlee! I will make the requested change from #28.

Here as an updated patch with a doc change that was noted in #28 to the twig template.

#36: twig-theme-image-1939068-36.patch queued for re-testing.

I Looked over the code and looks good to me and looks like it works....

This looks great :)

Before:

<img class="image-style-medium" typeof="foaf:Image" src="http://d8core.dev/sites/default/files/styles/medium/public/field/image/druplicon_1.png?itok=i-_79M5m" width="194" height="220" alt="" />

after:

<img class="image-style-medium" src="http://d8core.dev/sites/default/files/styles/medium/public/field/image/druplicon_1.png?itok=i-_79M5m" width="194" height="220" alt="" typeof="foaf:Image" />

I'm not fond of the use of trim in these tests:

-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');
+    $this->assertEqual(trim($img_tag), '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');

Needing to trim shows that a markup change has occured, namely a trailing whitespace character. I've fixed the same tests in #1898420: image.module - Convert theme_ functions to Twig by using {% spaceless %} in the templates to wrap inline elements.

re #44: shanethat we used trim() here because we couldn't get it to pass the tests otherwise. I have tested using {% spaceless %} in the image.html.twig, and removing trim() in the tests where it was called, and the tests then fail..

#45: I've got something similar to pass in the issue I linked in #44. I don't think that a trailing space is a massive deal, but I'll try to replicate what I've done in this one if you like.

Shanethat: I would like to see what you got to pass here. I must have implemented wrong when I tested.

#48: This is the entirety of image-style.html.twig (minus the docs):

{% spaceless %}<img src="{{ attributes.src }}"{{ attributes }} />{% endspaceless %}

Adding those spaceless tags stopped the training space breaking the tests in ImageDimensionsTest::testImageDimensions().

re #48: Shanethat: This is what I did in image.html.twig, but it still failed testing...

{% spaceless %}
  <img{{ attributes }}/>
{% endspaceless %}

#49: I believe that you need a space after the attributes tag:

{% spaceless %}
  <img{{ attributes }} />
{% endspaceless %}

Nope. Still fails even with the space after attributes.

Yay! All green. Thanks for this shamethat, I took the same approach as your patch did, but wasn't getting this to pass still.

I'm not sure why we need the spaceless tag when we could just delete the spaces... but glad you got it passing tests :) good enough for me! any objections?

ICBW but I believe this needs updating now that #1938430: Don't add a default theme hook class in template_preprocess() has been committed?

Rerolled, and I removed the part that referenced #1938430: Don't add a default theme hook class in template_preprocess().

I will profile...

bbranches 519ff7f3e6734 twig-n1939068-57.patch
=== twig-n1939068-57.patch..twig-n1939068-57.patch compared (519ff7f3e6734..519ff8a50a409):

ct : 40,386|40,469|83|0.2%
wt : 450,720|452,686|1,966|0.4%
cpu : 436,622|436,229|-393|-0.1%
mu : 40,759,592|40,798,208|38,616|0.1%
pmu : 40,892,112|40,931,952|39,840|0.1%

Profiler output

=== twig-n1939068-57.patch..twig-n1939068-57.patch compared (519ff7f3e6734..519ff8d5eb136):

ct : 40,386|40,469|83|0.2%
wt : 450,720|452,551|1,831|0.4%
cpu : 436,622|436,140|-482|-0.1%
mu : 40,759,592|40,798,208|38,616|0.1%
pmu : 40,892,112|40,931,952|39,840|0.1%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

I am reprofiling this. I did not have Stark theme on for the last profile.

bbranches 51a002186bd48 twig-n1939068-57.patch
=== 8.x..8.x compared (51a002186bd48..51a00299ecbf2):

ct : 48,210|48,210|0|0.0%
wt : 502,700|500,764|-1,936|-0.4%
cpu : 483,055|480,191|-2,864|-0.6%
mu : 42,527,144|42,527,144|0|0.0%
pmu : 42,667,048|42,667,048|0|0.0%

Profiler output

=== 8.x..twig-n1939068-57.patch compared (51a002186bd48..51a002d3645c2):

ct : 48,210|48,338|128|0.3%
wt : 502,700|502,258|-442|-0.1%
cpu : 483,055|481,231|-1,824|-0.4%
mu : 42,527,144|42,573,384|46,240|0.1%
pmu : 42,667,048|42,716,160|49,112|0.1%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

#57: twig-n1939068-57.patch queued for re-testing.

remove sandbox

generate 30 articles and promote them to the front page (I tweaked the view to show full nodes, no pager)

=== 8.x..8.x compared (51a459a8e59c2..51a45a3ea1945):

ct  : 119,579|119,579|0|0.0%
wt  : 958,042|958,463|421|0.0%
cpu : 952,059|952,059|0|0.0%
mu  : 9,614,976|9,614,976|0|0.0%
pmu : 9,730,876|9,730,876|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a459a8e59c2&...

=== 8.x..image-1939068-57 compared (51a459a8e59c2..51a45b36a618d):

ct  : 119,579|120,427|848|0.7%
wt  : 958,042|971,784|13,742|1.4%
cpu : 952,059|964,060|12,001|1.3%
mu  : 9,614,976|9,641,564|26,588|0.3%
pmu : 9,730,876|9,768,456|37,580|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a459a8e59c2&...

I uploaded an image and everything looks good. Changing to reviewed and tested by the community.

Quick change as per #66 but leaving as NW so we can solve the performance issues. MUF maybe? :)

This should make things a bit quicker, needs another round of performance testing.

@jenlampton sorry that's the second person tonight I crossposted patches on.

I don't think the test likes the newline character at the end of the image.html.twig. I have removed and am reattaching.

retagging

Profiled

=== 8.x..8.x compared (51ec227a9cea0..51ec2301b5aa5):

ct  : 761,520|761,520|0|0.0%
wt  : 2,798,555|2,795,811|-2,744|-0.1%
cpu : 2,771,046|2,765,369|-5,677|-0.2%
mu  : 19,861,784|19,861,784|0|0.0%
pmu : 20,090,008|20,090,008|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec227a9cea0&...

=== 8.x..1939068-image-twig compared (51ec227a9cea0..51ec243650361):

ct  : 761,520|762,538|1,018|0.1%
wt  : 2,798,555|2,807,164|8,609|0.3%
cpu : 2,771,046|2,777,615|6,569|0.2%
mu  : 19,861,784|19,891,768|29,984|0.2%
pmu : 20,090,008|20,122,056|32,048|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec227a9cea0&...

Scenario:
- Stark theme
- Block with node in left sidebar
- 30 nodes displayed on home page with view, full node shown, all with images (devel generate)

This looks good but not sure if we are allowed to be submitting patches with newlines errors in them.

@see https://drupal.org/coding-standards#indenting

I'm actually going to vote for using the spaceless tag here. We don't really want newlines in our markup every time there's an image :)

I had my hopes up that EOF new lines in twig files were ignored from output as it says in their docs.
http://twig.sensiolabs.org/doc/templates.html#whitespace-control

But after re-reading that for the umph-teen time, it's removing newline characters after the template tag... so any template should do the trick. Spaceless has some performance implications, maybe we could use simpler template tag? or if feasible, remove the last newline from all twig template's output in the engine?

So, from my local tests, comments work... haha self documenting fix :)

Re-rolled but may need to fix the new test as well for xpath for the attributes order.

Hi I noticed the #80 the output includes and empty alt attribute regardless if the alt attribute is permitted in the field configuration. (See the image attached)
I added a fix for this and attached for patch for this.

The empty alt attribute is better for screen readers as if it wasn't there they sometimes read out the src url, which isn't good.

See:
http://accessibility.psu.edu/imageshtml

Thanks for the info @pbz912 i think the alt & accessibility is probably something that may deserve a separate issue. Back to https://drupal.org/node/1939068#comment-7719149 then

+++ b/core/modules/system/templates/image.html.twig
@@ -0,0 +1,14 @@
+<img{{ attributes }} />{# This comment removes newline character. #}

of course this does work, but we decided to change the tests for this to pass so we don't have to explain this hack/workaround. Whitespace changes in tests are deemed acceptable. So could someone remove this comment and put the newline character into the tests that are comparing the theme_image output?

Will try to fix it. Have taken a look and it does not seem hard to do ;)

I'm not happy with the solution but now it's working. I have added line-breaks and removed empty alt elements that were breaking the tests.

Suggestions are welcome ;)

You shouldn't have to change the xpath ones because they don't care about whitespace. Also it may be better to convert the failing ones to xpath for the same reason. Or just append "\n"

@joelpittet: The xpath ones aren't working if I include alt="". I will check it again. Also I will move the others to xpath and include the carriage return. Thanks! :)

+++ b/core/includes/theme.inc
@@ -1761,17 +1763,14 @@ function theme_links($variables) {
-    if (isset($variables[$key])) {
-      $attributes[$key] = $variables[$key];
+    if (!empty($variables[$key])) {
+      $variables['attributes'][$key] = $variables[$key];

I think this is why the alt's aren't working this should still be isset()

Now it's much better and works! :)

Thanks @rteijeiro those xpaths are looking good. Here's a reroll as it looks things moved a bit.
Looks like just a few more of those need to be done for the Drupal\image\Tests\ImageDimensionsTest test.

Here is the quick fix for those dimension ones, just a \n appended to the tests because they aren't added to the page so xpath won't work readily and may be a bit slower.

reviewing #95:
- tests look good.
- passes tests by expecting \n's because #2082845: Remove end of file newline from twig files automatically.. all good.
- +.3% wt from benchmark in #73 may need another look. let's re-bench and discuss. if the diff is acceptable (edit: I think it is), RTBC++

.

Updated issue summary.

95: 1939068-95-image-twig.patch queued for re-testing.

100: 1939068-100.patch queued for re-testing.

105: 1939068-105.patch queued for re-testing.

I tried to reroll this patch yesterday. The conflicted code is in core/modules/image/lib/Drupal/image/Tests/ImageThemeFunctionTest.php

The tests in head checks for width and height attributes on two lines. The test in the patch needs to also add a test for those attributes before conflict can be resolved (lines 112 and 119 in the patched file).

I am not sure how to test for size attributes using xpath. I looked for an example elsewhere in drupal core but couldn't find it.

I see that someone else rerolled the patch so not sure if this message is helpful or not.

I tested the patch in #105, and it applied cleanly.

The test image of bacon that I uploaded had proper markup and looked delicious. :)

Hopefully all set.

Here's a patch and interdiff with those coding standard updates.

This patch is green. Do we need profiling next? Manual testing?

I tested the patch in #112 and it applied cleanly.
Created a new article node and uploaded an image. The image was printed with proper markup.

One small step for Twig, one giant leap for Twig kind. ;)

Committed and pushed to 8.x. Thanks!