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] Convert core theme functions to Twig templates

Review Bonus

See #2094585: [policy, no patch] Core review bonus

#190466: URL filter fails on square brackets +2

Points: 2

CommentFileSizeAuthor
#108 dev_toolbar-update_render_structure.patch510 bytesstar-szr
#108 interdiff.txt3.35 KBstar-szr
#108 1898464-108.patch10.54 KBstar-szr
#105 1898464-105.patch9.49 KBstar-szr
#101 1898464-100-toolbar-twig.patch9.89 KBadnen
#99 1898464-99-toolbar-twig.patch10.97 KBadnen
#96 1898464-96-toolbar-twig.patch4.55 KBadnen
#93 1898464-90-toolbar-twig.patch5.09 KBadnen
#91 1898464-90-toolbar-twig.patch5.09 KBadnen
#85 Page_not_found___d8.drupal.dev-8.png83.02 KBjessebeach
#74 1898464-74-toolbar-twig.patch9.55 KBjoelpittet
#74 interdiff.txt522 bytesjoelpittet
#72 1898464-72-toolbar-twig.patch2.74 KBjoelpittet
#72 interdiff.txt521 bytesjoelpittet
#69 interdiff.txt1.4 KBjoelpittet
#69 1898464-69-toolbar-twig.patch9.54 KBjoelpittet
#68 Kaleidoscope_–_toolbar-markup-php.txt___toolbar-markup-twig.txt-2.png364.22 KBjoelpittet
#65 1898464-65.patch9.31 KBstar-szr
#65 interdiff.txt689 bytesstar-szr
#65 1898464-one-pencil.png7.85 KBstar-szr
#64 1898464-daisydiff.png16.56 KBstar-szr
#64 1898464-two-pencils.png8.62 KBstar-szr
#64 1898464-64.patch8.64 KBstar-szr
#64 interdiff.txt1.93 KBstar-szr
#63 1898464-63.patch8.64 KBstar-szr
#61 twig-convert_toolbar-1898464-61.patch8.88 KBjenlampton
#59 twig-convert_toolbar-1898464-59.patch8.78 KBjenlampton
#59 interdiff.txt4.61 KBjenlampton
#58 twig-convert_toolbar-1898464-58.patch9.13 KBjenlampton
#58 interdiff.txt3.1 KBjenlampton
#57 twig-convert_toolbar-1898464-57.patch10.18 KBjenlampton
#57 interdiff.txt5.84 KBjenlampton
#56 twig-convert_toolbar-1898464-56.patch10 KBjenlampton
#55 twig-toolbar_conversion-1898464-55.patch9.41 KBjenlampton
#49 1898464-49-toolbar-twig.patch9.94 KBjoelpittet
#49 interdiff.txt3.19 KBjoelpittet
#43 toolbar_twig.diff6.24 KBcafuego
#29 1898464-29.patch10.19 KBstar-szr
#28 1898464-28.patch10.1 KBstar-szr
#28 interdiff.txt576 bytesstar-szr
#27 interdiff.txt1.25 KBshanethehat
#27 toolbar-twig-1898464-27.patch10.1 KBshanethehat
#25 interdiff.txt4.26 KBshanethehat
#25 toolbar-twig-1898464-25.patch10.14 KBshanethehat
#17 interdiff.txt3.89 KBjoelpittet
#17 toolbar-twig-1898464-17.patch10.27 KBjoelpittet
#13 core-update_toolbar_to_twig-1898464-13.patch10.51 KBDustin Currie
#10 core-update_toolbar_to_twig-1898464-6.patch10.47 KBDustin Currie
#5 core-update_toolbar_to_twig-1898464-5.patch10.93 KBjenlampton
#5 interdiff.txt4.84 KBjenlampton
#4 drupal-twig-toolbar-conversion--1898464-4.patch11.02 KBsteveoliver
#3 drupal-twig-toolbar-conversion--1898464-3.patch11 KBsteveoliver
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

steveoliver’s picture

Assigned: Unassigned » steveoliver
steveoliver’s picture

Status: Active » Needs review
FileSize
11 KB
steveoliver’s picture

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.

jenlampton’s picture

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.

Dustin Currie’s picture

Status: Needs work » Needs review
Issue tags: +Twig
steveoliver’s picture

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.

steveoliver’s picture

Assigned: steveoliver » Unassigned

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

Dustin Currie’s picture

Assigned: Unassigned » steveoliver
FileSize
10.47 KB

Removed toolbar_tray_heading_wrapper per idea 1 in #5

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

Dustin Currie’s picture

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.

Dustin Currie’s picture

Assigned: Unassigned » Dustin Currie
FileSize
10.51 KB
joelpittet’s picture

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.

joelpittet’s picture

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

joelpittet’s picture

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

hass’s picture

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

joelpittet’s picture

@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:)

hass’s picture

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

joelpittet’s picture

@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

nikkubhai’s picture

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.

nikkubhai’s picture

Issue summary: View changes

added idears

star-szr’s picture

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.
star-szr’s picture

Issue tags: +Novice

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

shanethehat’s picture

Status: Needs work » Needs review
FileSize
10.14 KB
4.26 KB

I think this covers everything in #23

star-szr’s picture

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.".
shanethehat’s picture

Status: Needs work » Needs review
FileSize
10.1 KB
1.25 KB
star-szr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
576 bytes
10.1 KB

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

Just rolling in one more tiny documentation tweak.

star-szr’s picture

FileSize
10.19 KB

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)

star-szr’s picture

Issue summary: View changes

Add conversion summary table

c4rl’s picture

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

Per #1757550-44: [Meta] 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.

alexpott’s picture

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

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.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Assigning to myself for the DC PDX Friday sprint

mr.baileys’s picture

Status: Needs work » Needs review

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

mr.baileys’s picture

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

mr.baileys’s picture

Issue tags: -Novice

Novice tasks have been tackled, so removing that tag.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned

Unassigning

ezeedub’s picture

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

assigning for profiling

ezeedub’s picture

Assigned: ezeedub » Unassigned
Issue tags: -Novice

er, nm

cafuego’s picture

Doing manual test of this at the DrupalCon sprint.

mr.baileys’s picture

Issue tags: -needs profiling

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

cafuego’s picture

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

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.

tlattimore’s picture

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.

tlattimore’s picture

Issue summary: View changes

Remove sandbox link

alexpott’s picture

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.

joelpittet’s picture

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.

joelpittet’s picture

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

joelpittet’s picture

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

star-szr’s picture

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.

joelpittet’s picture

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

Changing to CNW based on #50 and #47

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Assigned: gnuget » Unassigned

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

jenlampton’s picture

Assigned: Unassigned » jenlampton

I'll take a shot at this one.

jenlampton’s picture

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

jenlampton’s picture

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

jenlampton’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
10.18 KB

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.

jenlampton’s picture

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

jenlampton’s picture

Assigned: jenlampton » Unassigned
FileSize
4.61 KB
8.78 KB

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.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

passing tests locally.

star-szr’s picture

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.

star-szr’s picture

Status: Needs work » Needs review
FileSize
8.64 KB

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.

star-szr’s picture

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

star-szr’s picture

FileSize
7.85 KB
689 bytes
9.31 KB

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.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Issue summary: View changes

Update commit message and conversion table

star-szr’s picture

Issue summary: View changes

toolbar.module not Toolbar.module

star-szr’s picture

Status: Needs review » Needs work

Needs work for points 2 and 6 from #62.

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

joelpittet’s picture

Issue tags: +Novice

Tagging

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
1.4 KB

Fixes 2 & 6 from #62

joelpittet’s picture

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

jibran’s picture

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

@file missing.

joelpittet’s picture

FileSize
521 bytes
2.74 KB

Thanks #71 is fixed.

star-szr’s picture

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

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

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
522 bytes
9.55 KB

whoops, fixed and doing profiling.

joelpittet’s picture

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.

phiit’s picture

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

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

phiit’s picture

Issue summary: View changes

Update toolbar_item status

markhalliwell’s picture

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!

markhalliwell’s picture

Issue summary: View changes

review bonus

star-szr’s picture

Issue summary: View changes

Reorder suggested commit message based on Dreditor data

star-szr’s picture

Issue summary: View changes

Update remaining tasks

star-szr’s picture

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

jessebeach’s picture

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.

star-szr’s picture

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.

jessebeach’s picture

Issue tags: +Novice

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

jessebeach’s picture

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>
jenlampton’s picture

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)

jenlampton’s picture

Issue summary: View changes

Update remaining tasks

jessebeach’s picture

Issue summary: View changes
FileSize
83.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!

star-szr’s picture

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

jessebeach’s picture

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

star-szr’s picture

Issue tags: +Needs reroll

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

adnen’s picture

Assigned: Unassigned » adnen
Issue tags: +TUNIS_2013_DECEMBER

I'm working on this issue

adnen’s picture

Status: Needs work » Needs review
adnen’s picture

Status: Needs review » Needs work

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

adnen’s picture

Status: Needs work » Needs review
FileSize
5.09 KB

Status: Needs review » Needs work

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

star-szr’s picture

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.

adnen’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

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

star-szr’s picture

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.

adnen’s picture

Status: Needs work » Needs review
FileSize
10.97 KB

Status: Needs review » Needs work

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

adnen’s picture

Status: Needs work » Needs review
FileSize
9.89 KB
joelpittet’s picture

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

star-szr’s picture

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

Needs work based on the above.

star-szr’s picture

Assigned: adnen » star-szr

Working on this.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
9.49 KB

First a reroll from #74 to address #102.

joelpittet’s picture

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.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Not quite yet :)

star-szr’s picture

FileSize
10.54 KB
3.35 KB
510 bytes

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

star-szr’s picture

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.

star-szr’s picture

Status: Needs work » Needs review
jessebeach’s picture

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!

webchick’s picture

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?

star-szr’s picture

Assigned: star-szr » Unassigned

Woohoo!

Status: Fixed » Closed (fixed)

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