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-63] Convert core theme functions to Twig templates
#1885560: [meta] Convert everything in theme.inc to Twig

Files: 
CommentFileSizeAuthor
#28 1939102-28--twig-indentation.patch864 bytesmikl
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]
#26 1939102-26--twig-indentation.patch1.58 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,856 pass(es).
[ View ]
#14 1939102-14.patch1.52 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 57,318 pass(es).
[ View ]
#11 d8.dev-20130412-175326.jpg.jpg410.2 KBjoelpittet
#7 1929102_markup_without_patch.png49.95 KBschiavone
#7 1939102_with_patch.png38.11 KBschiavone
#5 1939102-5.patch1.51 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,638 pass(es).
[ View ]
#5 interdiff.txt606 bytesCottser
#2 interdiff.txt712 bytesjoelpittet
#2 twig-indentation-1939102-2.patch1.48 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 53,585 pass(es).
[ View ]
#1 1939102-1.patch1.48 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,056 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 53,056 pass(es).
[ View ]

Had to rework the sandbox template a bit, it was adding one indent too many for each level.

Status:Needs review» Needs work
StatusFileSize
new1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 53,585 pass(es).
[ View ]
new712 bytes

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

Status:Needs work» Needs review

run it again for good measure;)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new606 bytes
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,638 pass(es).
[ View ]

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

Issue tags:+Needs manual testing

Tagging.

Issue tags:-Needs manual testing
StatusFileSize
new38.11 KB
new49.95 KB

Manually 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

Issue summary:View changes

Add testing steps

Thanks @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!

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

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

{% spaceless %}
  {% if size > 1 %}
    {% for i in 1..size %}
      <div class="indentation">&nbsp;</div>
    {% endfor %}
  {% else %}
    <div class="indentation">&nbsp;</div>
  {% endif %}
{% endspaceless %}

Updating the Twig template with this code and rolling a new patch and interdiff would be a great novice task.

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

Actually, 'size' is defaulted to 1 in the theme hook definition. So #5 should be sufficient. I confirmed this with @joelpittet.

<?php
   
'indentation' => array(
     
'variables' => array('size' => 1),
     
'template' => 'indentation',
    ),
?>

I think this one is ready to go.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new410.2 KB

Manual testing, abusing PHP template while I can:D

d8.dev-20130412-175326.jpg.jpg

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs profiling

Issue tags:+Needs reroll

Issue tags:-Needs reroll
StatusFileSize
new1.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,318 pass(es).
[ View ]

rerolled version attached.

Status:Needs work» Needs review

Well i just change the status for make sure to test bot is ok with this reroll.

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

The last submitted patch, 1939102-14.patch, failed testing.

Status:Needs work» Needs review

#14: 1939102-14.patch queued for re-testing.

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

The last submitted patch, 1939102-14.patch, failed testing.

Status:Needs work» Needs review

Ignore #18 - bad bot. Queued for retest.

Issue tags:-Needs profiling

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

=== 8.x..8.x compared (51b89dd280f98..51b89eaf4e36e):
ct  : 163,719|163,719|0|0.0%
wt  : 952,799|953,500|701|0.1%
cpu : 881,583|879,318|-2,265|-0.3%
mu  : 16,626,344|16,626,344|0|0.0%
pmu : 17,636,904|17,636,904|0|0.0%.

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

=== 8.x..1939102-twig-indentation compared (51b89dd280f98..51b89ec98250c):
ct  : 163,719|166,316|2,597|1.6%
wt  : 952,799|966,616|13,817|1.5%
cpu : 881,583|895,851|14,268|1.6%
mu  : 16,626,344|16,689,616|63,272|0.4%
pmu : 17,636,904|17,691,088|54,184|0.3%

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

Issue summary:View changes

Add code tags

Status:Needs review» Reviewed & tested by the community

The indentation works. I replied to a post and the replied text was indented.

Status:Reviewed & tested by the community» Needs review

This seems to be quite a large regression for such a simple thing... how come?

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

=== 8.x..8.x compared (51c24fd9b971a..51c2508074568):
ct  : 172,339|172,339|0|0.0%
wt  : 1,168,790|1,169,811|1,021|0.1%
cpu : 1,101,882|1,103,662|1,780|0.2%
mu  : 38,608,488|38,608,488|0|0.0%
pmu : 39,643,968|39,643,968|0|0.0%

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

=== 8.x..1939102-twig-indentation compared (51c24fd9b971a..51c25102ed66d):
ct  : 172,339|174,744|2,405|1.4%
wt  : 1,168,790|1,183,443|14,653|1.3%
cpu : 1,101,882|1,115,003|13,121|1.2%
mu  : 38,608,488|38,669,504|61,016|0.2%
pmu : 39,643,968|39,704,568|60,600|0.2%

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

http://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

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

This needs a reroll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,856 pass(es).
[ View ]

Re-rolled

Status:Needs review» Needs work

We should do what @Fabianx suggested in #24 so changing this back to CNW.

Status:Needs work» Needs review
StatusFileSize
new864 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,505 pass(es).
[ View ]

Adapted the patch from #26 with the changes suggested in #24.

Status:Needs review» Reviewed & tested by the community

Thank you @mikl!

Looks like our field.html.twig.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs manual testing

Actually, just to make sure, can we do a manual test that this does indeed get rendered when put into the theme ?

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs manual testing

Tested and yes it does work as advertised. Back to RTBC.

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs manual testing

Committed c96cedf and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.