Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#112 | interdiff-1939068-105-112.txt | 5.1 KB | RainbowArray |
#112 | convert-theme-image-1939068-112.patch | 12.48 KB | RainbowArray |
#105 | 1939068-105.patch | 12.43 KB | InternetDevels |
#100 | interdiff.txt | 493 bytes | star-szr |
#100 | 1939068-100.patch | 11.92 KB | star-szr |
Comments
Comment #1
tlattimore CreditAttribution: tlattimore commentedI don't know of any blockers for moving this issue forward. I'll take it.
Comment #2
tlattimore CreditAttribution: tlattimore commentedHere 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.Comment #4
tlattimore CreditAttribution: tlattimore commentedHmmm. Not sure why this didn't pass the testbot. Will look into it.
Comment #5
tlattimore CreditAttribution: tlattimore commentedBy 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 oftemplate_preprocess_image()
I am getting a class ofimage
under the attributes key in the array, even when no class has been passed throughtheme('image', args)
??Example:
Comment #6
chrisjlee CreditAttribution: chrisjlee commentedI think the reason why it's failing is because of your image template
It should be like so:
At least according to the comment here:
Comment #7
tlattimore CreditAttribution: tlattimore commentedRe #6 - Thanks for the feedback, chrisjlee. I am using the
Attributes
class intemplate_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 forDrupal\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.
Comment #8
star-szr@tlattimore - Thanks for your work here!
I think what you're seeing is this line from template_preprocess():
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.
More info here:
http://drupal.org/node/1727592
Comment #9
tlattimore CreditAttribution: tlattimore commentedThanks 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.
Comment #10
tlattimore CreditAttribution: tlattimore commentedI 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.
Comment #11
steveoliver CreditAttribution: steveoliver commentedDocBlock change, first line should read:
"Prepares variables for image templates."
And to get rid of the 'image' class:
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.
Comment #12
steveoliver CreditAttribution: steveoliver commented...oh, and butt {{ attributes }} up to img, like this:
<img{{ attributes }}/>
Comment #13
tlattimore CreditAttribution: tlattimore commentedThanks 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.
Comment #14
tlattimore CreditAttribution: tlattimore commentedHere 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.
Comment #16
tlattimore CreditAttribution: tlattimore commentedAs I expected the
ImageThemeFunctionTest
didn't pass. I was however not anticpiating theTourTest
still fail. And sinceassertRaw()
doesn't return back the value found vs. what it expected I am not really sure what's occurring.Comment #17
joelpittetNice 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:
convert to
Comment #18
tlattimore CreditAttribution: tlattimore commentedThis 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
Comment #19
tlattimore CreditAttribution: tlattimore commentedBot, do your thing.
Comment #21
joelpittet@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:
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.
Comment #22
tlattimore CreditAttribution: tlattimore commentedThanks 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!
Comment #23
tlattimore CreditAttribution: tlattimore commentedI have gotten the tour related test to pass for this now, but still having trouble on the
mageThemeFunction
tests.Comment #24
joelpittet@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.
Comment #25
tlattimore CreditAttribution: tlattimore commentedjoelpittet: 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.
Comment #26
joelpittetXpathified image tests, @tlattimore tag, your it;)
Comment #27
tlattimore CreditAttribution: tlattimore commentedAhh... 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?
Comment #28
star-szrThis is looking good, found some minor documentation tweaks:
@param array $variables
- attributes should be unindented by two spaces, like this:
s/themable/themeable
Comment #29
chrisjlee CreditAttribution: chrisjlee commentedjust trying to move things along. Docs tweaks.
Comment #30
chrisjlee CreditAttribution: chrisjlee commentedMade a mistake.
Comment #31
star-szrWow, 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.
Comment #32
star-szrTagging.
Comment #33
chrisjlee CreditAttribution: chrisjlee commentedComment #34
star-szr@chrisjlee - thanks! As mentioned on IRC, preprocess looks good now, just need the tweak from #28 to the Twig template docblock now please :)
Comment #35
tlattimore CreditAttribution: tlattimore commentedThanks for working on this, chrisjlee! I will make the requested change from #28.
Comment #36
tlattimore CreditAttribution: tlattimore commentedHere as an updated patch with a doc change that was noted in #28 to the twig template.
Comment #38
tlattimore CreditAttribution: tlattimore commented#36: twig-theme-image-1939068-36.patch queued for re-testing.
Comment #39
matthewmack CreditAttribution: matthewmack commentedI Looked over the code and looks good to me and looks like it works....
Comment #40
star-szrLooks good! I found two small tweaks.
<img />
instead of<img/>
.Comment #41
star-szrAnd 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.
Comment #41.0
star-szrAdding sandbox issue to related issues
Comment #42
star-szrI 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)
Comment #43
jenlamptonThis looks great :)
Before:
after:
Comment #44
shanethehat CreditAttribution: shanethehat commentedI'm not fond of the use of trim in these tests:
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.
Comment #45
tlattimore CreditAttribution: tlattimore commentedre #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..
Comment #46
shanethehat CreditAttribution: shanethehat commented#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.
Comment #47
tlattimore CreditAttribution: tlattimore commentedShanethat: I would like to see what you got to pass here. I must have implemented wrong when I tested.
Comment #48
shanethehat CreditAttribution: shanethehat commented#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().
Comment #49
tlattimore CreditAttribution: tlattimore commentedre #48: Shanethat: This is what I did in image.html.twig, but it still failed testing...
Comment #50
shanethehat CreditAttribution: shanethehat commented#49: I believe that you need a space after the attributes tag:
Comment #51
tlattimore CreditAttribution: tlattimore commentedNope. Still fails even with the space after
attributes
.Comment #52
shanethehat CreditAttribution: shanethehat commentedComment #53
tlattimore CreditAttribution: tlattimore commentedYay! All green. Thanks for this shamethat, I took the same approach as your patch did, but wasn't getting this to pass still.
Comment #54
jenlamptonI'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?
Comment #55
alexpottComment #56
DamienMcKennaICBW but I believe this needs updating now that #1938430: Don't add a default theme hook class in template_preprocess() has been committed?
Comment #57
DamienMcKennaRerolled, and I removed the part that referenced #1938430: Don't add a default theme hook class in template_preprocess().
Comment #58
carsonevans CreditAttribution: carsonevans commentedI will profile...
Comment #59
carsonevans CreditAttribution: carsonevans commentedbbranches 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
Comment #60
carsonevans CreditAttribution: carsonevans commentedI am reprofiling this. I did not have Stark theme on for the last profile.
Comment #61
carsonevans CreditAttribution: carsonevans commentedbbranches 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
Comment #63
ezeedub CreditAttribution: ezeedub commented#57: twig-n1939068-57.patch queued for re-testing.
Comment #63.0
ezeedub CreditAttribution: ezeedub commentedremove sandbox
Comment #64
ezeedub CreditAttribution: ezeedub commentedgenerate 30 articles and promote them to the front page (I tweaked the view to show full nodes, no pager)
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a459a8e59c2&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a459a8e59c2&...
Comment #65
azinoman CreditAttribution: azinoman commentedI uploaded an image and everything looks good. Changing to reviewed and tested by the community.
Comment #66
star-szrProfiling results don't look so great on this one - I think we need to improve that before commit.
Also this line should be removed per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
Comment #67
jenlamptonQuick change as per #66 but leaving as NW so we can solve the performance issues. MUF maybe? :)
Comment #68
joelpittetThis should make things a bit quicker, needs another round of performance testing.
Comment #69
joelpittet@jenlampton sorry that's the second person tonight I crossposted patches on.
Comment #71
drupalninja99 CreditAttribution: drupalninja99 commentedI don't think the test likes the newline character at the end of the image.html.twig. I have removed and am reattaching.
Comment #72
jenlamptonretagging
Comment #73
jerdavisProfiled
=== 8.x..8.x compared (51ec227a9cea0..51ec2301b5aa5):
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec227a9cea0&...
=== 8.x..1939068-image-twig compared (51ec227a9cea0..51ec243650361):
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)
Comment #74
joelpittetThis 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
Comment #75
star-szrAgreed, we'll need to use {% spaceless %} or update the tests. In this case I would probably lean towards updating the tests.
Comment #76
jenlamptonI'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 :)
Comment #77
joelpittetI 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?
Comment #78
joelpittetSo, from my local tests, comments work... haha self documenting fix :)
Comment #79
star-szrNeeds another reroll…
Comment #80
joelpittetRe-rolled but may need to fix the new test as well for xpath for the attributes order.
Comment #81
gavin.hughes CreditAttribution: gavin.hughes commentedHi 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.
Comment #82
pbz1912 CreditAttribution: pbz1912 commentedThe 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
Comment #84
gavin.hughes CreditAttribution: gavin.hughes commentedThanks 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
Comment #85
joelpittetof 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?
Comment #86
rteijeiro CreditAttribution: rteijeiro commentedWill try to fix it. Have taken a look and it does not seem hard to do ;)
Comment #87
rteijeiro CreditAttribution: rteijeiro commentedI'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 ;)
Comment #89
joelpittetYou 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"
Comment #90
rteijeiro CreditAttribution: rteijeiro commented@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! :)Comment #91
joelpittetI think this is why the alt's aren't working this should still be isset()
Comment #92
rteijeiro CreditAttribution: rteijeiro commentedNow it's much better and works! :)
Comment #94
joelpittetThanks @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.
Comment #95
joelpittetHere 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.
Comment #96
steveoliver CreditAttribution: steveoliver commentedreviewing #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++
Comment #97
joelpittet.
Comment #97.0
joelpittetUpdated issue summary.
Comment #98
joelpittet95: 1939068-95-image-twig.patch queued for re-testing.
Comment #99
star-szrhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c641a950046&...
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).
Comment #100
star-szrI'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.
Comment #103
joelpittet100: 1939068-100.patch queued for re-testing.
Comment #104
star-szrTagging for reroll.
Comment #105
InternetDevels CreditAttribution: InternetDevels commentedComment #107
InternetDevels CreditAttribution: InternetDevels commented105: 1939068-105.patch queued for re-testing.
Comment #108
manningpete CreditAttribution: manningpete commentedI 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.
Comment #109
star-szrComment #110
RainbowArrayI 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.
Comment #111
star-szrThese concatenations don't meet coding standards, need spaces around the dot. Otherwise this is looking good to me.
Comment #112
RainbowArrayHere's a patch and interdiff with those coding standard updates.
Comment #113
RainbowArrayThis patch is green. Do we need profiling next? Manual testing?
Comment #114
aboros CreditAttribution: aboros commentedI 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.
Comment #116
webchickOne small step for Twig, one giant leap for Twig kind. ;)
Committed and pushed to 8.x. Thanks!