Issue #1898464 by jenlampton, Cottser, joelpittet, steveoliver, Dustin Currie, shanethehat, cafuego, mr.baileys: toolbar.module - Convert theme_ functions to Twig.

Task

Use Twig instead of PHPTemplate

Remaining

  • Patch needs review

Theme function name/template path Conversion status
theme_toolbar converted to Twig template: toolbar.html.twig
theme_toolbar_item theme function removed, pre-render (toolbar_pre_render_item()) kept
theme_toolbar_tab_wrapper removed and made part of toolbar template
theme_toolbar_tray_heading_wrapper Removed and refactored - it was just an <h3> prefix
theme_toolbar_tray_wrapper removed and made part of toolbar template

Consolidation ideas (all completed here for performance reasons):
- Let's take the twig code from toolbar_tray_heading_wrapper and put it inside toolbar_tray_wrapper with some logic instead.
- Would be nice if we could figure out how to get rid of toolbar-item.html.twig since it doesn't do anything. (do we need it?)
- Maybe we can do the same with toolbar_tab_wrapper, too :)

#1757550: [META-63] Convert core theme functions to Twig templates

Review Bonus

See #2094585: [meta] [experimental] Core review bonus

#190466: URL filter fails on square brackets +2

Points: 2

Files: 
CommentFileSizeAuthor
#108 dev_toolbar-update_render_structure.patch510 bytesCottser
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#108 interdiff.txt3.35 KBCottser
#108 1898464-108.patch10.54 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,339 pass(es).
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» steveoliver

Status:Active» Needs review
StatusFileSize
new11 KB
PASSED: [[SimpleTest]]: [MySQL] 50,793 pass(es).
[ View ]

StatusFileSize
new11.02 KB
PASSED: [[SimpleTest]]: [MySQL] 50,786 pass(es).
[ View ]

Attached patch addresses the following:

--- /dev/null
+++ b/core/modules/toolbar/templates/toolbar-item.html.twig
--- /dev/null
+++ b/core/modules/toolbar/templates/toolbar-tab-wrapper.html.twig

Need newlines at end of files.

+++ b/core/modules/toolbar/toolbar.module
@@ -388,26 +399,34 @@ function theme_toolbar_tab_wrapper(&$variables) {
  * @param array $variables
  *   An associative array containing:
  *   - element: An associative array containing the properties and children of
  *     the tray. Properties used: #children and #heading.
  */
-function theme_toolbar_tray_heading_wrapper(&$variables) {

Needs @see & @ingroup.

StatusFileSize
new4.84 KB
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 52,793 pass(es).
[ View ]

renamed wrapper_attributes to attributes for consistency with other templates.
renamed heading_label to label for clarity

I really hate looking at the tangled code in this module, it makes me want to kill all theme wrappers, grr... but aside from that, this conversion looks great! :)

Follow-up issue ideas:
- Let's take the twig code from toolbar_tray_heading_wrapper and put it inside toolbar_tray_wrapper with some logic instead.
- Would be nice if we could figure out how to get rid of toolbar-item.html.twig since it doesn't do anything. (do we need it?)
- Maybe we can do the same with toolbar_tab_wrapper, too :)

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

The last submitted patch, core-update_toolbar_to_twig-1898464-5.patch, failed testing.

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

Status:Needs review» Needs work

Jen #5, I think your first item makes sense, but 2 and 3:

- Would be nice if we could figure out how to get rid of toolbar-item.html.twig since it doesn't do anything. (do we need it?)
- Maybe we can do the same with toolbar_tab_wrapper, too :)

I think we want to keep these for hook_toolbar.

Assigned:steveoliver» Unassigned

Also unassigning. I think Dustin will be taking this over.

Assigned:Unassigned» steveoliver
StatusFileSize
new10.47 KB
FAILED: [[SimpleTest]]: [MySQL] 52,607 pass(es), 0 fail(s), and 15 exception(s).
[ View ]

Removed toolbar_tray_heading_wrapper per idea 1 in #5

Agree with Steve on keeping tabs and trays and even items as separate templates.

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

Status:Needs review» Needs work

The last submitted patch, core-update_toolbar_to_twig-1898464-6.patch, failed testing.

Assigned:Unassigned» Dustin Currie
StatusFileSize
new10.51 KB
FAILED: [[SimpleTest]]: [MySQL] 53,054 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs work» Needs review

Running testbot again.

Can you add a newline to the end of that one file for git's sake?

Also could pull out attributes.class like this:

<div class="{{ attributes.class }}"{{ attributes }}>
  {{ content }}
</div>

@see http://drupal.org/node/1823416

Status:Needs review» Needs work

The last submitted patch, core-update_toolbar_to_twig-1898464-13.patch, failed testing.

@Dustin Currie scratch that attributes change, it's a moving target and a little discussion on irc seemed to land that the class will be pulled out for bartik I believe due to it's audience. All the rest may be clean like that one...

This looks close! Just that new line and the testbot error in EntityTranslationUITest which I haven't looked into yet.

Status:Needs work» Needs review
StatusFileSize
new10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 53,220 pass(es).
[ View ]
new3.89 KB

@Dustin Currie It passed locally for me, I just cleaned up a few things in the comments and light formatting and we'll run this sucker again. There may be some more nitpicky things to look at in the preprocess docblocks as per http://drupal.org/node/1913208

Please also look into #1944572: Remove "ul.menu" dependency to prevent theme clashes.

Aside - are we really removing phptemplate now or do we keep both? I heard rumours about this and like to know for my themes...

@hass I don't think #1944572: Remove "ul.menu" dependency to prevent theme clashes will have any effect on this conversion issue but thanks for sharing nontheless because that sounds like a good thing to see happen! Also, yes we are really removing phptemplate now:)

@joelpittet: Thanks for this details. I think it's very bad that phptemplate get's removed, so I'm very unhappy about this. This is not positive and nothing to smile about.

@hass PHPTemplate may end up in contrib... but you should hear some of the reasons why twig was chosen:
http://www.youtube.com/watch?v=QGIqu_Te0PA

I tested the patch using simpletest and everything worked fine. I didn't notice any differences during manual testing.

Before:
Only local images are allowed.Only local images are allowed.

After:

Only local images are allowed.Only local images are allowed.

Issue summary:View changes

added idears

Assigned:Dustin Currie» Unassigned
Status:Needs review» Needs work

This is looking good, thanks everyone. Found some tweaks but this is pretty close to RTBC in my opinion.

@Dustin Currie - I'm unassigning since it's been a while but if you'd like to work on updating this patch please reassign. Thanks!

  1. +++ b/core/modules/toolbar/templates/toolbar-tab-wrapper.html.twigundefined
    @@ -0,0 +1,19 @@
    + * Toolbar tabs have a common styling and placement with the toolbar's bar area.

    This seems like it should be "within the toolbar's bar area".

  2. +++ b/core/modules/toolbar/templates/toolbar.html.twigundefined
    @@ -0,0 +1,26 @@
    + * - wrapper_attributes: HTML attributes for the the wrapper.

    Two "the", pick one to remove :)

  3. All the preprocess functions should be updated to meet #1913208: [policy] Standardize template preprocess function documentation:
    1. +++ b/core/modules/toolbar/toolbar.moduleundefined
      @@ -248,24 +249,30 @@ function ($object) {
      + * Prepares variables for the administration toolbar.
        *
        * @param array $variables
        *   An associative array containing:
        *   - element: An associative array containing the properties and children of
        *     the tray. Properties used: #children, #attributes and #bar.
      + *
      + * @see toolbar.html.twig
      + *
      + * @ingroup themeable

      The @see should be removed and "Default template: toolbar.html.twig." added between the summary line (Prepares…) and the @param.

      Remove @ingroup themeable.

    2. +++ b/core/modules/toolbar/toolbar.moduleundefined
      @@ -322,46 +329,32 @@ function toolbar_pre_render_item($element) {
      + * Prepares variables for toolbar-item.html.twig and variant templates.
        *
      - * This theme function only renders the tab portion of the toolbar item. The
      - * tray portion will be rendered later.
      + * Prepares the tab portion of the toolbar item. Tray portion is rendered later.
        *
        * @param array $variables
        *   An associative array containing:
        *   - element: An associative array containing the properties and children of
        *     the tray. Property used: tab.
        *
      - * @see toolbar_pre_render_item().
      - * @see theme_toolbar().
      + * @see toolbar_pre_render_item()
      + * @see toolbar-item.html.twig
      + *
      + * @ingroup themeable

      Prepares variables for toolbar item templates.

      Prepares the tab portion of the toolbar item, the tray portion is rendered
      later. (combine into one sentence).

      Default template: …

      The @see toolbar_pre_render_item() can be kept.
      Remove the other @see and the @ingroup themeable.

    3. And so on, update all preprocess functions in this manner.

Issue tags:+Novice

The changes in #23 would make a good novice task.

Status:Needs work» Needs review
StatusFileSize
new10.14 KB
PASSED: [[SimpleTest]]: [MySQL] 54,239 pass(es).
[ View ]
new4.26 KB

I think this covers everything in #23

Status:Needs review» Needs work

Great work, thanks @shanethehat :)

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -322,92 +327,75 @@ function toolbar_pre_render_item($element) {
+ * Prepares variables for toolbar item templates.
+ * Prepares variables for the administration toolbar.
+ * Prepares variables for toolbar-tab-wrapper.html.twig and variant templates.
+ * Prepares variables for toolbar-tray-wrapper.html.twig and variant templates.
  • The first one is perfect.
  • The second one should end in "templates." - "…for administration toolbar templates."
  • The third and fourth end in templates, but shouldn't mention the filename (that's what the "Default template:" line is for) and we don't need the word "variant" in there. So something like "…for toolbar tab wrapper templates.".

Status:Needs work» Needs review
StatusFileSize
new10.1 KB
PASSED: [[SimpleTest]]: [MySQL] 54,356 pass(es).
[ View ]
new1.25 KB

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new576 bytes
new10.1 KB
PASSED: [[SimpleTest]]: [MySQL] 54,270 pass(es).
[ View ]

This is ready to go. Tested manually, markup matches up, code looks good.

Just rolling in one more tiny documentation tweak.

StatusFileSize
new10.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,192 pass(es).
[ View ]

I messed up, the actual patch from #28 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)

Issue summary:View changes

Add conversion summary table

Title:Convert toolbar module to Twigtoolbar.module - Convert theme_ functions to Twig

Per #1757550-44: [META-63] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

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

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

#29: 1898464-29.patch queued for re-testing.

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

The last submitted patch, 1898464-29.patch, failed testing.

Assigned:Unassigned» mr.baileys

Assigning to myself for the DC PDX Friday sprint

Status:Needs work» Needs review

#29: 1898464-29.patch queued for re-testing.

Issue tags:-Needs profiling

Gave the anonymous user permissions to the toolbar and a ton of admin stuff to make sure there are enough items in the toolbar, then profiled it and uploaded the results (I ran a couple of batches, results are consistent):

=== 8.x..8.x compared (519fb8e35d279..519fb917058ac):
ct  : 49,249|49,249|0|0.0%
wt  : 263,876|262,835|-1,041|-0.4%
cpu : 256,016|260,016|4,000|1.6%
mu  : 13,977,944|13,978,096|152|0.0%
pmu : 14,108,272|14,108,568|296|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fb8e35d279&...

=== 8.x..1898464-twig-toolbar compared (519fb8e35d279..519fb952126aa):
ct  : 49,249|49,952|703|1.4%
wt  : 263,876|268,648|4,772|1.8%
cpu : 256,016|264,017|8,001|3.1%
mu  : 13,977,944|14,242,368|264,424|1.9%
pmu : 14,108,272|14,369,056|260,784|1.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fb8e35d279&...

Issue tags:-Novice

Novice tasks have been tackled, so removing that tag.

Assigned:mr.baileys» Unassigned

Unassigning

Assigned:Unassigned» ezeedub
Issue tags:+Novice, +Needs profiling

assigning for profiling

Assigned:ezeedub» Unassigned
Issue tags:-Novice

er, nm

Doing manual test of this at the DrupalCon sprint.

Issue tags:-Needs profiling

Removing the "Needs profiling" tag, profiling results in #36

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new6.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch toolbar_twig.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Output looks good! Checked visually and diffed the original and twiggified output (after piping through tidy to remove whitespace issues). Diff is attached.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, toolbar_twig.diff, failed testing.

Status:Needs work» Reviewed & tested by the community

Tagging this back as RTBC. The diff from #43 should have had "*--do-not-test.*" in the filename as to not have it run by the test bot, it tried to apply it, and of course it failed and put back to needs work. The patch from #29 looks good.

Issue summary:View changes

Remove sandbox link

Status:Reviewed & tested by the community» Needs review

So the profiling numbers show a 2% wt time regression. We need to work out why this is. 2% on top of the already large regression from D7 is quite large.

Assigned:Unassigned» joelpittet

@alexpott I see some easy performance wins we could make in the attributes bits. The best performance improvement would be to move all the 4 templates into one template. Or at the very least drop toolbar-item.html.twig.

But first I am going to confirm #36 performance.

Scenario:

  • Stark Theme on standard install profile
  • All anonymous user permissions turned on.

First Run

=== 8.x..8.x compared (51ad01387670b..51ad018db619c):
ct  : 51,454|51,454|0|0.0%
wt  : 384,347|384,689|342|0.1%
cpu : 353,700|354,845|1,145|0.3%
mu  : 29,154,296|29,154,296|0|0.0%
pmu : 29,328,384|29,328,384|0|0.0%

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

=== 8.x..1898464-toolbar-twig compared (51ad01387670b..51ad01d342500):
ct  : 51,454|52,319|865|1.7%
wt  : 384,347|388,485|4,138|1.1%
cpu : 353,700|358,690|4,990|1.4%
mu  : 29,154,296|29,313,536|159,240|0.5%
pmu : 29,328,384|29,486,808|158,424|0.5%

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

Second Run

=== 8.x..8.x compared (51ad024086f90..51ad028f5e6a3):
ct  : 51,454|51,454|0|0.0%
wt  : 384,603|385,068|465|0.1%
cpu : 354,671|354,458|-213|-0.1%
mu  : 29,154,296|29,154,296|0|0.0%
pmu : 29,328,384|29,328,384|0|0.0%

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

=== 8.x..1898464-toolbar-twig compared (51ad024086f90..51ad02c47d041):
ct  : 51,454|52,319|865|1.7%
wt  : 384,603|388,949|4,346|1.1%
cpu : 354,671|357,778|3,107|0.9%
mu  : 29,154,296|29,313,536|159,240|0.5%
pmu : 29,328,384|29,486,808|158,424|0.5%

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

Issue tags:+Needs manual testing
StatusFileSize
new3.19 KB
new9.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,839 pass(es).
[ View ]

Same Scenario, but with the attached patch.

Seems to be 1% regression down to 0.7% regression with just the attributes creation touch-ups.

=== 8.x..8.x compared (51ad0610ca11a..51ad06a537e44):
ct  : 98,581|98,581|0|0.0%
wt  : 598,025|598,928|903|0.2%
cpu : 554,783|555,836|1,053|0.2%
mu  : 38,374,064|38,374,064|0|0.0%
pmu : 38,539,472|38,539,472|0|0.0%

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

=== 8.x..1898464-toolbar-twig compared (51ad0610ca11a..51ad06ef28a53):
ct  : 98,581|99,445|864|0.9%
wt  : 598,025|603,396|5,371|0.9%
cpu : 554,783|558,723|3,940|0.7%
mu  : 38,374,064|38,539,024|164,960|0.4%
pmu : 38,539,472|38,705,736|166,264|0.4%

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

Agreed with #47, there is a lot of template nesting going on here that I think we should try to reduce to at least 3 layers if not 2.

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

Changing to CNW based on #50 and #47

Assigned:Unassigned» gnuget

Assigned:gnuget» Unassigned

I didn't had the chance to work on this, maybe someone else can take it

Assigned:Unassigned» jenlampton

I'll take a shot at this one.

StatusFileSize
new9.41 KB
FAILED: [[SimpleTest]]: [MySQL] 57,340 pass(es), 4 fail(s), and 985 exception(s).
[ View ]

The patch needed a reroll too, so here's a reroll before the refactor.

StatusFileSize
new10 KB
PASSED: [[SimpleTest]]: [MySQL] 57,115 pass(es).
[ View ]

this one passes tests, and has some more minor cleanup.

Status:Needs work» Needs review
StatusFileSize
new5.84 KB
new10.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,634 pass(es).
[ View ]

Here's a patch that removes toolbar-tab-wrapper.html.twig. Changing to NR to get testbot involved, but I'll remove at least one more template in the next patch.

StatusFileSize
new3.1 KB
new9.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,521 pass(es).
[ View ]

this patch also removes toolbar-item.html.twig and also has a little more cleanup.

Assigned:jenlampton» Unassigned
StatusFileSize
new4.61 KB
new8.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,170 pass(es), 0 fail(s), and 22 exception(s).
[ View ]

and this one removes toolbar-tray-wrapper.html.twig too :) We're ready for a new review now.

Status:Needs review» Needs work

The last submitted patch, twig-convert_toolbar-1898464-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,503 pass(es).
[ View ]

passing tests locally.

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

This is exciting! Great work @jenlampton :)

Going to do some manual testing and this will need re-profiling. In the meantime I found some things that can be cleaned up:

  1. +++ b/core/modules/toolbar/templates/toolbar.html.twig
    @@ -0,0 +1,38 @@
    + * - tabs: List of tabs for the toolbar.
    + *   - tab.attributes: HTML Attributes for the tab container.
    + *   - tab.link: Link or button for the menu tab.

    We should remove the 'tab.' - indenting in a list means drill down/"add a dot" and 'tab' is just the loop variable.

  2. +++ b/core/modules/toolbar/templates/toolbar.html.twig
    @@ -0,0 +1,38 @@
    + * - trays: Toolbar trays, each associated with a tab.

    We should doc the attributes, label and links variables contained within trays, and we can call trays a list too.

  3. +++ b/core/modules/toolbar/templates/toolbar.html.twig
    @@ -0,0 +1,38 @@
    +      <div {{ tab.attributes }}>{{ tab.link }}</div>

    Extra space inside div before attributes are printed.

  4. +++ b/core/modules/toolbar/toolbar.module
    @@ -45,19 +45,12 @@ function toolbar_permission() {
    -  $items['toolbar_tab_wrapper'] = array(
    -    'render element' => 'element',
    -  );
    -  $items['toolbar_tray_wrapper'] = array(
    -    'render element' => 'element',
    -  );
    -  $items['toolbar_tray_heading_wrapper'] = array(
    -    'render element' => 'element',
    -  );

    So awesome :D

  5. +++ b/core/modules/toolbar/toolbar.module
    @@ -217,24 +207,53 @@ function ($object) {
    +  // Prepaare the trays and tabs for each toolbar item.

    Typo here, Prepare.

  6. +++ b/core/modules/toolbar/toolbar.module
    @@ -217,24 +207,53 @@ function ($object) {
    +      unset($element[$key]['tray']);

    I think we should document why we're unsetting this.

Status:Needs work» Needs review
StatusFileSize
new8.64 KB
PASSED: [[SimpleTest]]: [MySQL] 57,855 pass(es).
[ View ]

Okay, this needed a reroll so here is a reroll after #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, with no other changes.

StatusFileSize
new1.93 KB
new8.64 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new8.62 KB
new16.56 KB

Completed 1, 3, and 5 from #62. Nothing needs to be done for 4 :) 2 and 6 are remaining documentation tasks.

This should get the markup closer, I did a DaisyDiff comparison. The only difference I'm seeing now in DaisyDiff is that there are two "Edit" shortcut buttons. I rolled back and couldn't reproduce this with the patch in #61 though.

DaisyDiff showing two edit buttons:
1898464-daisydiff.png

Visually seeing two edit buttons:
1898464-two-pencils.png

StatusFileSize
new7.85 KB
new689 bytes
new9.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,800 pass(es).
[ View ]

1898464-one-pencil.png
Better :)

Also wanted to note that there is documentation referring to the toolbar_tab_wrapper theme function that will need to be updated.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Update commit message and conversion table

Issue summary:View changes

toolbar.module not Toolbar.module

Status:Needs review» Needs work

Needs work for points 2 and 6 from #62.

Edit: And the documentation updates mentioned in my last comment.

Issue tags:+Novice

Tagging

There is only a number change on the tray item id's
"toolbar-item--6-tray" and whitespace diffs. I think this is fine...

Kaleidoscope_–_toolbar-markup-php.txt___toolbar-markup-twig.txt-2.png

Status:Needs work» Needs review
StatusFileSize
new9.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,687 pass(es).
[ View ]
new1.4 KB

Fixes 2 & 6 from #62

I'll profile this if someone can confirm that the id changes are fine in #68?

+++ b/core/modules/toolbar/templates/toolbar.html.twig
@@ -0,0 +1,42 @@
+ * Default theme implementation for the administrative toolbar.

@file missing.

StatusFileSize
new521 bytes
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,929 pass(es).
[ View ]

Thanks #71 is fixed.

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

Looks like we lost some stuff from #69 to #72.

Assigned:Unassigned» joelpittet
Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new522 bytes
new9.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,889 pass(es).
[ View ]

whoops, fixed and doing profiling.

Assigned:joelpittet» Unassigned
Issue tags:-Needs profiling

Scenario:

Seven theme
Full permissions for anonymous
Front page set to /node

=== 8.x..8.x compared (523f01900df5c..523f01e38fa84):
ct  : 72,856|72,856|0|0.0%
wt  : 473,180|472,840|-340|-0.1%
cpu : 434,487|438,537|4,050|0.9%
mu  : 35,253,048|35,253,048|0|0.0%
pmu : 35,363,016|35,363,016|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f01900df5c&...

=== 8.x..1898464-toolbar-twig compared (523f01900df5c..523f024cca254):
ct  : 72,856|72,909|53|0.1%
wt  : 473,180|473,778|598|0.1%
cpu : 434,487|434,917|430|0.1%
mu  : 35,253,048|35,295,120|42,072|0.1%
pmu : 35,363,016|35,400,440|37,424|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f01900df5c&...

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

The last submitted patch, 1898464-74-toolbar-twig.patch, failed testing.

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

#74: 1898464-74-toolbar-twig.patch queued for re-testing.

Issue summary:View changes

Update toolbar_item status

Status:Needs review» Reviewed & tested by the community

Patch reviewed, nothing stands out. Several rounds of profiling with negligible impact. All feedback has been addressed. Let's get this in :)

@joelpittet, if we could get an issue summary update and issues created for the follow-ups, that'd be great!

Great job everyone!

Issue summary:View changes

review bonus

Issue summary:View changes

Reorder suggested commit message based on Dreditor data

Issue summary:View changes

Update remaining tasks

Issue tags:+Novice

+++ b/core/modules/toolbar/templates/toolbar.html.twig
@@ -0,0 +1,43 @@
+ *   - attributes: HTML Attributes for the tab container.

This should be "HTML attributes" (lowercase A on attributes). Other than that I agree with @Mark Carver, this looks good and is still RTBC. Issue summary updated as well.

Tagging for BADCamp sprinting :)

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/toolbar/toolbar.module
@@ -287,103 +307,12 @@ function toolbar_pre_render_item($element) {
-    if (!isset($element['tray']['#theme_wrappers'])) {
-      $element['tray']['#theme_wrappers'] = array();
-    }
-    // Add the standard theme_wrapper for trays.
-    array_unshift($element['tray']['#theme_wrappers'], 'toolbar_tray_wrapper');
-    // Add the theme wrapper for the tray heading.
-    array_unshift($element['tray']['#theme_wrappers'], 'toolbar_tray_heading_wrapper');

By eliminating these theme functions, we've removed the ability of module developers and themers to create Toolbar components that do not have a tray. The tray structure is meant to be a convenience, not a fixed structural imposition. Not doing this would represent a regression is flexibility of this module.

We need to reintroduce these theme functions and atomize the toolbar.html.twig file.

Can we do partial template includes? I'm sure TWIG proper can do this, just not sure if our implementation of TWIG has that inclusion.

I can help to break down single template into sub-templates.

Issue tags:-Novice

Ok, thanks @jessebeach. By partial template includes do you mean including only part of a template into another template? Maybe we can convene in the next couple days to discuss this issue.

Issue tags:+Novice

Ya! We can get this resolved during the sprints tomorrow. I don't think the changes will be difficult or blocking.

cottser and I just discussed this problem. We can probably put a catch-all variable that prints the rest of the render array content (non-tab, non-tray renderables) that were passed to hook_toolbar.

<nav{{ attributes }}>
  <div{{ toolbar_attributes }}>
    <h2 class="visually-hidden">{{ toolbar_heading }}</h2>
    {% for tab in tabs %}
      <div{{ tab.attributes }}>{{ tab.link }}</div>
    {% endfor %}
  </div>
  {% for tray in trays %}
    {% spaceless %}
    <div{{ tray.attributes }}>
      <div class="toolbar-lining clearfix">
        {% if tray.label %}
          <h3 class="toolbar-tray-name visually-hidden">{{ tray.label }}</h3>
        {% endif %}
        {{ tray.links }}
      </div>
    </div>
    {% endspaceless %}
  {% endfor %}
  {# print anything that isn't a tab or tray #}
  {{ random_stuff }}
</nav>

we talked about doing the same thing for nodes, forms, and other renderables that need to render some things explicitly, but then still need to render "the rest" and the variable name we came up with for that was {{ remainder }}. We should probably be consistent with that naming convention and do the same here. (@see #2004252: node.html.twig template for {{ content.remainder }} example)

Issue summary:View changes

Update remaining tasks

Issue summary:View changes
StatusFileSize
new83.02 KB

At BADCamp I promised cottser a sandbox module that implements hook_toolbar. Here it is:

https://drupal.org/sandbox/jessebeach/dev_toolbar

The item it adds is a mock shopping cart. It includes a tab that toggles a div that would contain the shopping cart info. But this div is not in a tray. It's just rendered to the Toolbar wrapper.

EXCEPT, in building this module, I noticed bad structuring. wah wah.

The shopping cart container info div should be a child of .toolbar, not .toolbar-bar. This is something we can correct in the template.

I hope this helps!

Awesome, thanks @jessebeach! The markup in that screenshot is coming from the existing theme functions right?

Yes, the module is built from off-the-shelf parts. The markup is just a string in a #markup key.

Issue tags:+Needs reroll

Tagging for reroll and then we can update the Twig template as discussed.

Assigned:Unassigned» adnen
Issue tags:+TUNIS_2013_DECEMBER

I'm working on this issue

Status:Needs work» Needs review

StatusFileSize
new5.09 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 91: 1898464-90-toolbar-twig.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.09 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 93: 1898464-90-toolbar-twig.patch, failed testing.

Thanks @adnen. The patches are missing the Twig template for toolbar.html.twig (use git apply --index to stage additions when applying a patch) and missing the removal of the theme_toolbar_* functions and template_preprocess_toolbar_tab_wrapper(). The patch file size having shrunk by about half was my first clue.

Status:Needs work» Needs review
StatusFileSize
new4.55 KB
FAILED: [[SimpleTest]]: [MySQL] 59,242 pass(es), 23 fail(s), and 451 exception(s).
[ View ]

i added --index when appling the patch , i also staged the toolbar.html.twig file

Status:Needs review» Needs work

Great, it has the toolbar.html.twig template now! But the patch still needs to remove the theme_toolbar_* functions and the preprocess function. Please check and compare to the earlier patches.

The last submitted patch, 96: 1898464-96-toolbar-twig.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.97 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 99: 1898464-99-toolbar-twig.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,280 pass(es).
[ View ]

@adnen great that's looking better. Green! Just a couple things:

+++ b/core/modules/toolbar/toolbar.module
@@ -287,99 +307,24 @@ function toolbar_pre_render_item($element) {
+function toolbar_system_info_alter(&$info, $file, $type) {
+  if ($type == 'theme') {
+    $info['overlay_supplemental_regions'][] = 'page_top';

I think this got added by accident because it wasn't in the previous patch.

+++ b/core/modules/toolbar/toolbar.module
@@ -287,99 +307,24 @@ function toolbar_pre_render_item($element) {
+ * Implements hook_system_info_alter().
...
+ * Indicate that the 'page_top' region (in which the toolbar will be displayed)
+ * is an overlay supplemental region that should be refreshed whenever its
+ * content is updated.
...
+ * This information is provided for any module that might need to use it, not
+ * just the core Overlay module.

These bits look like they got added by accident too.

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

Needs work based on the above.

Assigned:adnen» Cottser

Working on this.

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new9.49 KB
PASSED: [[SimpleTest]]: [MySQL] 59,605 pass(es).
[ View ]

First a reroll from #74 to address #102.

Status:Needs review» Reviewed & tested by the community

This has been profiled, works with @jbeach's module, manual tested and the re-roll is identical.

Status:Reviewed & tested by the community» Needs work

Not quite yet :)

StatusFileSize
new10.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,339 pass(es).
[ View ]
new3.35 KB
new510 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here are the changes to add {{ remainder }}, this makes @jessebeach's module (https://drupal.org/sandbox/jessebeach/dev_toolbar) work. I'm also attaching a small patch to @jessebeach's module that modifies the render array to match up with what's in core now and prevents double tab rendering when tested along with this patch.

This also takes care of the minor change pointed out in #79, and I re-profiled and we are actually calling less functions now than before because there are less calls to theme().

Scenario is the same as #75, a fresh install with the standard profile and gave anonymous users full permissions.

=== 8.x..8.x compared (52c8629919553..52c862c126d67):
ct  : 68,823|68,823|0|0.0%
wt  : 231,459|230,815|-644|-0.3%
cpu : 217,072|216,300|-772|-0.4%
mu  : 17,439,392|17,439,392|0|0.0%
pmu : 17,595,584|17,595,584|0|0.0%

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

=== 8.x..toolbar-1898464-106 compared (52c8629919553..52c862ea26981):
ct  : 68,823|68,715|-108|-0.2%
wt  : 231,459|231,523|64|0.0%
cpu : 217,072|216,894|-178|-0.1%
mu  : 17,439,392|17,466,936|27,544|0.2%
pmu : 17,595,584|17,621,504|25,920|0.1%

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

Status:Needs work» Needs review

And I forgot -do-not-test for the dev_toolbar patch :P

Status:Needs review» Needs work

The last submitted patch, 108: dev_toolbar-update_render_structure.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

The #108 patch looks great!!!

Cottser, I added your patch to the dev_toolbar module, btw, although I don't think we'll need it any more.

I tested the patch with the dev_toolbar module and had a look through the rendered HTML. Everything is where it should be.

I'm the one who dropped this from RTBC in #80. I have no more objections. Cottser provided profiling, which has caused alexpott to drop this from RTBC. So I think we can safely re-up this patch to RTBC now.

I'M SO EXCITED ABOUT THIS! Awesome work!

Status:Reviewed & tested by the community» Fixed

Holy cow. This is a GREAT clean-up!!

Committed and pushed to 8.x. Thanks!

Jesse, does it make sense to backport this to the D7 toolbar module too?

Assigned:Cottser» Unassigned

Woohoo!

Status:Fixed» Closed (fixed)

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