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.
Task
Use Twig instead of PHPTemplate
Twig sandbox: http://drupal.org/sandbox/pixelmord/1750250
Remaining
Manual testing
Testing steps
Visit admin/structure/menu/manage/admin/edit, look for <div class="indentation">
, these are used to display the menu depth.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
#1885560: [meta] Convert everything in theme.inc to Twig
Comment | File | Size | Author |
---|---|---|---|
#28 | 1939102-28--twig-indentation.patch | 864 bytes | mikl |
#26 | 1939102-26--twig-indentation.patch | 1.58 KB | joelpittet |
#14 | 1939102-14.patch | 1.52 KB | gnuget |
#11 | d8.dev-20130412-175326.jpg.jpg | 410.2 KB | joelpittet |
#7 | 1929102_markup_without_patch.png | 49.95 KB | schiavone |
Comments
Comment #1
star-szrHad to rework the sandbox template a bit, it was adding one indent too many for each level.
Comment #2
joelpittetLooks great, works great:)
possible 80 char limit on the twig docblock but otherwise gorgeous. Patch attached, moved two words down (couldn't widow it).
If you are ok with the formatting, it's good for RTBC from me.
Comment #3
joelpittetrun it again for good measure;)
Comment #4
star-szrThanks @joelpittet, good catch! I missed that while working on this, but that first line actually should be shortened to less than 80 chars because it is considered the "summary line" per http://drupal.org/node/1354#drupal.
Comment #5
star-szrRevised patch to address #4. The only other thing I wanted to mention: at least with this implementation, I'm not sure that 'size' is really optional. I think if you left out size, you wouldn't get any divs.
Comment #6
star-szrTagging.
Comment #7
schiavone CreditAttribution: schiavone commentedManually tested markup of patched and unpatch instances on simplytest me. Confirmed existence of ,,, in both instances. Screen shots attached.
Tested on chrome with Bartik theme
Comment #7.0
schiavone CreditAttribution: schiavone commentedAdd testing steps
Comment #8
star-szrThanks @schiavone. My mistake, I forgot
<code>
tags in the issue summary, the markup is actually<div class="indentation">
that we were looking for. Screenshot looks good though, thank you!Comment #9
star-szrI was looking at this again and the Twig template logic should be a bit different if the 'size' variable is truly optional.
Something like this should work, needs testing though:
Updating the Twig template with this code and rolling a new patch and interdiff would be a great novice task.
Comment #10
star-szrActually, 'size' is defaulted to 1 in the theme hook definition. So #5 should be sufficient. I confirmed this with @joelpittet.
I think this one is ready to go.
Comment #11
joelpittetManual testing, abusing PHP template while I can:D
Comment #12
alexpottComment #13
joelpittetComment #14
gnugetrerolled version attached.
Comment #15
gnugetWell i just change the status for make sure to test bot is ok with this reroll.
Comment #17
star-szr#14: 1939102-14.patch queued for re-testing.
Comment #19
jthorson CreditAttribution: jthorson commentedIgnore #18 - bad bot. Queued for retest.
Comment #20
joelpittetThis template isn't used much so I just added the admin menu page to the home page.
Scenario:
Stark
/admin/structure/menu/manage/admin on front page
Did 4 runs, and the numbers were around 1.4%-1.6% wall time variation and the function call difference matched pretty close to the CPU/Wall Time diff.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b89dd280f98&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b89dd280f98&...
Comment #20.0
joelpittetAdd code tags
Comment #21
adamcowboy CreditAttribution: adamcowboy commentedThe indentation works. I replied to a post and the replied text was indented.
Comment #22
alexpottThis seems to be quite a large regression for such a simple thing... how come?
Comment #23
joelpittetNot sure why the regression but it seems the function call percentage is mapping very close to the wall time/cpu percentage.
Maybe @Fabianx can help explain?
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c24fd9b971a&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c24fd9b971a&...
Comment #24
Fabianx CreditAttribution: Fabianx commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c24fd9b971a&...
explains it pretty well:
The overhead is:
15 ms
Of that time is spent for:
* 9 ms - Template rendering
* 1.3 ms - Preprocess and process
* 0.5 ms - Attribute:: overhead
* 1.6 ms - function_exists and general theme() function overhead for templates
--
9 + 3.4 = 12.4 ms = ~ 13 ms
The rest is fluctuation. => 1.2% overhead
The template has a rather large overhead, because for loops have some overhead in twig:
* Container, template loading, settings: ~ 4 ms
* Template render: 5 ms
Template render:
* OB, error handling: 2.5 ms
* Template handling: 2.5 ms
Template itself:
* PHP code: 1.3 ms
* loop overhead: 1.2 ms
Given this template is likely not often needed to be overwritten / used only seldomly I opt for:
* Keep theme() function
* Add unused template like field.html.twig for designers to add to their theme
Comment #25
star-szrThis needs a reroll.
Comment #26
joelpittetRe-rolled
Comment #27
joelpittetWe should do what @Fabianx suggested in #24 so changing this back to CNW.
Comment #28
mikl CreditAttribution: mikl commentedAdapted the patch from #26 with the changes suggested in #24.
Comment #29
joelpittetThank you @mikl!
Looks like our field.html.twig.
Comment #30
joelpittetActually, just to make sure, can we do a manual test that this does indeed get rendered when put into the theme ?
Comment #31
joelpittetTested and yes it does work as advertised. Back to RTBC.
Comment #32
alexpottCommitted c96cedf and pushed to 8.x. Thanks!
Comment #33.0
(not verified) CreditAttribution: commentedUpdated issue summary.