Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
FileSize
1.48 KB

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

joelpittet’s picture

Status: Needs review » Needs work
FileSize
1.48 KB
712 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.

joelpittet’s picture

Status: Needs work » Needs review

run it again for good measure;)

star-szr’s picture

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.

star-szr’s picture

Status: Needs work » Needs review
FileSize
606 bytes
1.51 KB

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.

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

schiavone’s picture

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

schiavone’s picture

Issue summary: View changes

Add testing steps

star-szr’s picture

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!

star-szr’s picture

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.

star-szr’s picture

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.

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

I think this one is ready to go.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
410.2 KB

Manual testing, abusing PHP template while I can:D

d8.dev-20130412-175326.jpg.jpg

alexpott’s picture

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

Issue tags: +Needs reroll
gnuget’s picture

Issue tags: -Needs reroll
FileSize
1.52 KB

rerolled version attached.

gnuget’s picture

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.

star-szr’s picture

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.

jthorson’s picture

Status: Needs work » Needs review

Ignore #18 - bad bot. Queued for retest.

joelpittet’s picture

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

joelpittet’s picture

Issue summary: View changes

Add code tags

adamcowboy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

joelpittet’s picture

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

Fabianx’s picture

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

star-szr’s picture

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

This needs a reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.58 KB

Re-rolled

joelpittet’s picture

Status: Needs review » Needs work

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

mikl’s picture

Status: Needs work » Needs review
FileSize
864 bytes

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @mikl!

Looks like our field.html.twig.

joelpittet’s picture

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 ?

joelpittet’s picture

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

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

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.