Issue #1939068 by joelpittet, tlattimore, Cottser, chrisjlee, rteijeiro, drupalninja99, jenlampton, DamienMcKenna, shanethehat, carsonevans, ezeedub: Convert theme_image() to Twig.

Task

Convert theme_image() to a Twig template.

Remaining

  • Replace all theme functions and templates with .html.twig equivalent templates
  • Add new preprocess functions for the .html.twig equivalent templates
  • Update all hook_theme definitions

Related

#1643894: theme('image')
#1885560: [meta] Convert everything in theme.inc to Twig

Files: 
CommentFileSizeAuthor
#112 interdiff-1939068-105-112.txt5.1 KBmdrummond
#112 convert-theme-image-1939068-112.patch12.48 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,693 pass(es). View
#105 1939068-105.patch12.43 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,335 pass(es). View
#100 interdiff.txt493 bytesCottser
#100 1939068-100.patch11.92 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,502 pass(es). View
#95 interdiff.txt5.2 KBjoelpittet
#95 1939068-95-image-twig.patch12 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,009 pass(es). View
#94 1939068-94-image-twig-reroll.patch6.96 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 58,710 pass(es), 9 fail(s), and 0 exception(s). View
#92 1939068-92-image-twig.patch6.96 KBrteijeiro
FAILED: [[SimpleTest]]: [MySQL] 58,668 pass(es), 9 fail(s), and 0 exception(s). View
#92 interdiff.txt3.95 KBrteijeiro
#87 1939068-87-image-twig.patch6.53 KBrteijeiro
FAILED: [[SimpleTest]]: [MySQL] 58,596 pass(es), 10 fail(s), and 0 exception(s). View
#87 interdiff.txt3.42 KBrteijeiro
#81 d8_twig_image_empty_alt_attribute.png16.53 KBgavin.hughes
#81 1939068-81-image-twig.patch5.8 KBgavin.hughes
FAILED: [[SimpleTest]]: [MySQL] 58,565 pass(es), 12 fail(s), and 0 exception(s). View
#80 1939068-80-image-twig.patch5.79 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,595 pass(es). View
#78 interdiff.txt547 bytesjoelpittet
#78 1939068-78-image-twig.patch5.61 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,613 pass(es). View
#71 1939068-71-image-twig.patch5.59 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 56,783 pass(es). View
#71 interdiff.txt542 bytesdrupalninja99
#68 1939068-67-image-twig.patch5.56 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 58,368 pass(es), 11 fail(s), and 0 exception(s). View
#68 interdiff.txt1.46 KBjoelpittet
#67 twig-image-1939068-67.patch5.44 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 57,787 pass(es), 74 fail(s), and 4 exception(s). View
#67 interdiff.txt580 bytesjenlampton
#57 twig-n1939068-57.patch5.47 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 57,497 pass(es). View
#52 interdiff.txt5.57 KBshanethehat
#52 1939068-52.patch5.77 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 54,684 pass(es). View
#42 1939068-42.patch10.78 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,565 pass(es). View
#41 1939068-41.patch10.74 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es). View
#41 interdiff.txt646 bytesCottser
#40 1939068-36.patch10.74 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,310 pass(es). View
#40 interdiff.txt6.12 KBCottser
#36 twig-theme-image-1939068-36.patch10.66 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 54,374 pass(es). View
#36 interdiff.txt684 bytestlattimore
#33 interdiff.txt2.53 KBchrisjlee
#33 twig-theme-image-1939068-33.patch10.67 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es). View
#30 interdiff.txt806 byteschrisjlee
#30 twig-theme-image-1939068-30.patch12.44 KBchrisjlee
PASSED: [[SimpleTest]]: [MySQL] 54,268 pass(es). View
#29 interdiff.txt2.96 KBchrisjlee
#29 twig-theme-image-1939068-29.patch12.44 KBchrisjlee
FAILED: [[SimpleTest]]: [MySQL] 54,227 pass(es), 1 fail(s), and 0 exception(s). View
#26 twig-theme-image-1939068-26.patch10.47 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 53,619 pass(es). View
#26 interdiff.txt2.68 KBjoelpittet
#25 1939068-theme-image-25-do-not-test.patch7.92 KBtlattimore
#25 interdiff.txt926 bytestlattimore
#18 1939068-theme-image-18.patch7.97 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] 53,259 pass(es), 3 fail(s), and 2 exception(s). View
#18 interdiff.txt1.36 KBtlattimore
#14 1939068-theme-image-14.patch7.07 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] 53,206 pass(es), 4 fail(s), and 0 exception(s). View
#14 interdiff.txt6.46 KBtlattimore
#2 1939068-2.patch1.87 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] 53,049 pass(es), 13 fail(s), and 0 exception(s). View

Comments

tlattimore’s picture

Assigned: Unassigned » tlattimore

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

tlattimore’s picture

Status: Active » Needs review
FileSize
1.87 KB
FAILED: [[SimpleTest]]: [MySQL] 53,049 pass(es), 13 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1939068-2.patch, failed testing.

tlattimore’s picture

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

tlattimore’s picture

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
                )

        )

............
chrisjlee’s picture

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:

tlattimore’s picture

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.

Cottser’s picture

@tlattimore - Thanks for your work here!

I think what you're seeing is this line from template_preprocess():

// Initialize html class attribute for the current hook.
$variables['attributes']['class'][] = drupal_html_class($hook);

You should be able to get rid of this class by unsetting it in preprocess, or by creating a new Attribute object in your preprocess function and adding any attributes you need.

$variables['attributes'] = new Attribute;
$variables['attributes']['class'] = array();
$variables['attributes']['class'][] = 'my_image_class';

More info here:
http://drupal.org/node/1727592

tlattimore’s picture

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.

tlattimore’s picture

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.

steveoliver’s picture

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

steveoliver’s picture

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

<img{{ attributes }}/>

tlattimore’s picture

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.

tlattimore’s picture

Status: Needs work » Needs review
FileSize
6.46 KB
7.07 KB
FAILED: [[SimpleTest]]: [MySQL] 53,206 pass(es), 4 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1939068-theme-image-14.patch, failed testing.

tlattimore’s picture

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.

joelpittet’s picture

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.
tlattimore’s picture

FileSize
1.36 KB
7.97 KB
FAILED: [[SimpleTest]]: [MySQL] 53,259 pass(es), 3 fail(s), and 2 exception(s). View

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

tlattimore’s picture

Status: Needs work » Needs review

Bot, do your thing.

Status: Needs review » Needs work

The last submitted patch, 1939068-theme-image-18.patch, failed testing.

joelpittet’s picture

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

tlattimore’s picture

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!

tlattimore’s picture

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

joelpittet’s picture

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

tlattimore’s picture

Assigned: tlattimore » Unassigned
FileSize
926 bytes
7.92 KB

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
10.47 KB
PASSED: [[SimpleTest]]: [MySQL] 53,619 pass(es). View

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

tlattimore’s picture

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?

Cottser’s picture

This is looking good, found some minor documentation tweaks:

+++ b/core/includes/theme.incundefined
@@ -1851,7 +1851,9 @@ function theme_dropbutton_wrapper($variables) {
+ * Prepares variables for image templates.
+ *
+ * Default template: image.html.twig.
  *
  * @param $variables

@param array $variables

+++ b/core/modules/system/templates/image.html.twigundefined
@@ -0,0 +1,15 @@
+ * Available variables:
+ *   - attributes: HTML attributes for the img tag.

- attributes should be unindented by two spaces, like this:

 * Available variables:
 * - attributes: …
+++ b/core/modules/system/templates/image.html.twigundefined
@@ -0,0 +1,15 @@
+ * @ingroup themable

s/themable/themeable

chrisjlee’s picture

FileSize
12.44 KB
FAILED: [[SimpleTest]]: [MySQL] 54,227 pass(es), 1 fail(s), and 0 exception(s). View
2.96 KB

just trying to move things along. Docs tweaks.

chrisjlee’s picture

FileSize
12.44 KB
PASSED: [[SimpleTest]]: [MySQL] 54,268 pass(es). View
806 bytes

Made a mistake.

Cottser’s picture

Status: Needs review » Needs work

Wow, thanks @chrisjlee!

Sorry for the confusion - for the unindenting I was referring to the attributes in the Twig template file, the preprocess @param was indented properly.

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 54,449 pass(es). View
2.53 KB
Cottser’s picture

@chrisjlee - thanks! As mentioned on IRC, preprocess looks good now, just need the tweak from #28 to the Twig template docblock now please :)

tlattimore’s picture

Assigned: Unassigned » tlattimore
Status: Needs review » Needs work

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

tlattimore’s picture

Status: Needs work » Needs review
FileSize
684 bytes
10.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,374 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-theme-image-1939068-36.patch, failed testing.

tlattimore’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig

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

matthewmack’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

Assigned: tlattimore » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
6.12 KB
10.74 KB
PASSED: [[SimpleTest]]: [MySQL] 54,310 pass(es). View

Looks good! I found two small tweaks.

Cottser’s picture

Assigned: Unassigned » steveoliver
FileSize
646 bytes
10.74 KB
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es). View

And since we're updating the hunk anyway, I can't resist fixing the grammar on the comment in TourTest.

I don't see anything else to change here, assigning to @steveoliver for review.

Cottser’s picture

Issue summary: View changes

Adding sandbox issue to related issues

Cottser’s picture

FileSize
10.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,565 pass(es). View

I messed up, the actual patch from #41 doesn't include the changes from the interdiff. I was experimenting with http://hojtsy.hu/blog/2012-apr-13/quick-and-simple-interdiffs and missed a step. Here's the correct patch with docs tweak (patch + interdiff).

(edited to fess up)

jenlampton’s picture

Assigned: steveoliver » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

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" />
shanethehat’s picture

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

-    $this->assertEqual($img_tag, 'Only local images are allowed.');
+    $this->assertEqual(trim($img_tag), 'Only local images are allowed.');

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.

tlattimore’s picture

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

shanethehat’s picture

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

tlattimore’s picture

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

shanethehat’s picture

#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().

tlattimore’s picture

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

{% spaceless %}
  <img{{ attributes }}/>
{% endspaceless %}
shanethehat’s picture

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

{% spaceless %}
  <img{{ attributes }} />
{% endspaceless %}
tlattimore’s picture

Nope. Still fails even with the space after attributes.

shanethehat’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 54,684 pass(es). View
5.57 KB
tlattimore’s picture

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

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?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs profiling
DamienMcKenna’s picture

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

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,497 pass(es). View

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

carsonevans’s picture

I will profile...

carsonevans’s picture

Issue tags: -Needs profiling

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

carsonevans’s picture

Issue tags: +Needs profiling

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

carsonevans’s picture

Issue tags: -Needs profiling

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

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, twig-n1939068-57.patch, failed testing.

ezeedub’s picture

Status: Needs work » Needs review
Issue tags: +Twig

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

ezeedub’s picture

Issue summary: View changes

remove sandbox

ezeedub’s picture

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

azinoman’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Profiling results don't look so great on this one - I think we need to improve that before commit.

+++ b/core/modules/system/templates/image.html.twigundefined
@@ -0,0 +1,17 @@
+ * @see template_preprocess()

Also this line should be removed per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

jenlampton’s picture

FileSize
580 bytes
5.44 KB
FAILED: [[SimpleTest]]: [MySQL] 57,787 pass(es), 74 fail(s), and 4 exception(s). View

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

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Needs profiling
FileSize
1.46 KB
5.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,368 pass(es), 11 fail(s), and 0 exception(s). View

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

joelpittet’s picture

Issue tags: -Needs profiling

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

Status: Needs review » Needs work

The last submitted patch, 1939068-67-image-twig.patch, failed testing.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
542 bytes
5.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,783 pass(es). View

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

jenlampton’s picture

Issue tags: +Needs profiling

retagging

jerdavis’s picture

Issue tags: -Needs profiling

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)

joelpittet’s picture

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

Cottser’s picture

Status: Needs review » Needs work

Agreed, we'll need to use {% spaceless %} or update the tests. In this case I would probably lean towards updating the tests.

jenlampton’s picture

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 :)

joelpittet’s picture

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?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,613 pass(es). View
547 bytes

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

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs another reroll

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,595 pass(es). View

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

gavin.hughes’s picture

Priority: Normal » Minor
FileSize
5.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,565 pass(es), 12 fail(s), and 0 exception(s). View
16.53 KB

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.

pbz1912’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1939068-81-image-twig.patch, failed testing.

gavin.hughes’s picture

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

joelpittet’s picture

+++ 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?

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

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

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
3.42 KB
6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,596 pass(es), 10 fail(s), and 0 exception(s). View

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 ;)

Status: Needs review » Needs work

The last submitted patch, 1939068-87-image-twig.patch, failed testing.

joelpittet’s picture

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"

rteijeiro’s picture

@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! :)

joelpittet’s picture

+++ 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()

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
3.95 KB
6.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,668 pass(es), 9 fail(s), and 0 exception(s). View

Now it's much better and works! :)

Status: Needs review » Needs work

The last submitted patch, 1939068-92-image-twig.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,710 pass(es), 9 fail(s), and 0 exception(s). View

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.

joelpittet’s picture

Assigned: rteijeiro » Unassigned
FileSize
12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,009 pass(es). View
5.2 KB

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.

steveoliver’s picture

Status: Needs review » Needs work

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++

joelpittet’s picture

Issue tags: +Needs profiling

.

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs work » Needs review

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

Cottser’s picture

Priority: Minor » Normal
Issue summary: View changes
Issue tags: -Needs profiling
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
=== 8.x..8.x compared (52c641a950046..52c6423bf12de):

ct  : 441,010|441,010|0|0.0%
wt  : 1,150,826|1,151,311|485|0.0%
cpu : 1,132,131|1,132,563|432|0.0%
mu  : 22,216,544|22,216,544|0|0.0%
pmu : 22,704,368|22,704,368|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c641a950046&...

=== 8.x..image-1939068-95 compared (52c641a950046..52c642960e9a6):

ct  : 441,010|442,164|1,154|0.3%
wt  : 1,150,826|1,153,751|2,925|0.3%
cpu : 1,132,131|1,134,749|2,618|0.2%
mu  : 22,216,544|22,244,632|28,088|0.1%
pmu : 22,704,368|22,737,008|32,640|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c641a950046&...

Profiled this with the same scenario as #73:

30 nodes displayed on home page view, full node shown, all with images (generated via devel generate).

Cottser’s picture

FileSize
11.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,502 pass(es). View
493 bytes

I'm thinking this conversion might need the same treatment as #1939102: Convert theme_indentation() to Twig, at least until we can optimize in other areas.

Attached patch fixes up one line of documentation.

The last submitted patch, 100: 1939068-100.patch, failed testing.

The last submitted patch, 100: 1939068-100.patch, failed testing.

joelpittet’s picture

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

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.43 KB
PASSED: [[SimpleTest]]: [MySQL] 63,335 pass(es). View

Status: Needs review » Needs work

The last submitted patch, 105: 1939068-105.patch, failed testing.

InternetDevels’s picture

Status: Needs work » Needs review

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

manningpete’s picture

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.

Cottser’s picture

Issue tags: +Twig conversion
mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
+++ b/core/modules/image/lib/Drupal/image/Tests/ImageDimensionsTest.php
@@ -71,7 +71,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="120" height="60" alt="" />'."\n");

@@ -92,7 +92,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="60" height="120" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="60" height="120" alt="" />'."\n");

@@ -114,7 +114,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />'."\n");

@@ -136,7 +136,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />'."\n");

@@ -154,7 +154,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="45" height="90" alt="" />'."\n");

@@ -175,7 +175,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />'."\n");

@@ -195,7 +195,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="30" height="30" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" width="30" height="30" alt="" />'."\n");

@@ -216,7 +216,7 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />'."\n");

@@ -235,6 +235,6 @@ function testImageDimensions() {
-    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />');
+    $this->assertEqual($img_tag, '<img class="image-style-test" src="' . $url . '" alt="" />'."\n");

These concatenations don't meet coding standards, need spaces around the dot. Otherwise this is looking good to me.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
12.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,693 pass(es). View
5.1 KB

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

mdrummond’s picture

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

aboros’s picture

Status: Needs review » Reviewed & tested by the community

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.

  • Commit b933a5b on 8.x by webchick:
    Issue #1939068 by joelpittet, tlattimore, Cottser, chrisjlee, rteijeiro...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.