Problem/Motivation

Theming menu trees and menu links is currently difficult and not very flexible. One of the issues is a general lack of contextual information. The fact that menu links and menu trees are separate themeable functions does not help matters.

Proposed resolution

Remove theme_menu_tree and theme_menu_link, and instead render menus using a recursive macro in a Twig template.

The patch also adds a link generator extension to the D8 Twig implementation.

Remaining tasks

  • Manual testing
  • Profiling/benchmarking

User interface changes

None.

API changes

Removal of the following functions:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Closely related, and possibly a dependency: #1777326: Replace theme_links() with theme_item_list()

Possible dependency, because the local tasks actually rather map to the current theme_links() instead of theme_item_list().

I also created #1778992: Merge theme_menu_link/_local_task/_local_action() theme functions into theme_link() from which I'll remove the local tasks/actions part now.

sun’s picture

Title: call theme_item_list from theme_menu_local_tasks, don't write our own special UL LI tags. » Replace theme_menu_local_tasks()/_actions() with theme_item_list() or theme_links()

Clarifying title.

jwilson3’s picture

seems like there is an emerging pattern to replace the theme functions with their generic counterpart, but then expose theme suggestions in case someone in contrib does want to provide a specific override. Would that make sense here?

Eg,

theme_link__menu_local_task
theme_item_list__menu_tasks
theme_item_list__menu_actions

My only thought is that these calls to the various theme suggestions in this case are necessarily nested, (ie theme_menu_local_task would be called from within theme_menu_tasks()), I dont see how we could properly know to call that second theme suggestion inside a generic theme_item_list, unless we implemented our own first-level suggestion for theme_item_list__menu_tasks. This brings us back to the question of multi-level render arrays of doom.

jenlampton’s picture

Issue summary: View changes

update

jenlampton’s picture

Let's go with theme_links instead of theme_item_list, so how about:

theme_links
theme_links__menu
theme_links__menu__local_tasks

or

theme_links
theme_links__menu
theme_links__menu__local_actions

This may require a little re-wiring but I think it will make more sense to people.

jenlampton’s picture

Title: Replace theme_menu_local_tasks()/_actions() with theme_item_list() or theme_links() » Remove all specific menu link theme functions, use theme_links() with suggestions instead

Changing issue title to account for all menu theme functions, and I updated the issue summary to paint a clearer picture of the master plan.

Also, I closed this one as dupe since it seems to all be the same problem:
#1834022: Combine theme_menu_tree and theme_menu_link, and rename theme_menu

jenlampton’s picture

Issue summary: View changes

add menu

jenlampton’s picture

Issue tags: +Template consolidation

more tagging

jenlampton’s picture

My brain is telling me that we decided not to name menus with links. So... how about this:

All menus:

theme_menu
theme_menu__[MENU_NAME]

Local tasks can be themed like this:

theme_menu__local_tasks
theme_menu__[MENU_NAME]__local_tasks

Local actions can be themed like this:

theme_menu__local_actions
theme_menu__[MENU_NAME]__local_actions
jenlampton’s picture

Issue summary: View changes

more sense

BarisW’s picture

Title: Remove all specific menu link theme functions, use theme_links() with suggestions instead » Remove all specific menu link theme functions, use theme_menu() with suggestions instead

Updating title.

BarisW’s picture

A lot of work is already done here: #1898478: menu.inc - Convert theme_ functions to Twig.

hass’s picture

We need theme suggestion for toolbar menu to fix #1944572: Remove "ul.menu" dependency to prevent theme clashes. Will this be fixed by this patch, too?

star-szr’s picture

@hass - it sounds like you're talking about implementing specific preprocess functions for theme suggestions, the issue to follow for that is #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() which is part of updating the theme system architecture to be more logical and squash critical bugs like #939462: Specific preprocess functions for theme hook suggestions are not invoked.

hass’s picture

I'm not really sure yet. Today in D7 it's the main problem that we are not able to theme the menu completely. The reason is that a lot of functions are nested and you cannot inject/remove standard classes in the menu tree. This patch here looks like you remove this limitations and bring the background/hidden functions to to front what allow alterations. And we are not able to easily override the theme menu tree in a module for one specific menu (e.g. toolbar menu). This is very complicated to override and requires a duplication of a lot of core functions.

star-szr’s picture

I agree, I had to override the menu tree very recently and it's painful. I would like to help improve this for D8 and this issue seems like it could do that.

hass’s picture

As a test if it works easy I suggest a test that removes the .menu class from the menu tree and add .toolbar-menu, but only if it's the toolbar menu. This will show is if it's flexible or not. :-)

joelpittet’s picture

*I'm Dreaming* but I'd love to move the tree rendering to a twig recursive macro.

http://twig.sensiolabs.org/doc/tags/macro.html

Sample data

$links = array(
  array(
    'name' => 'link 1',
    'href' => '#',
  ),
  array(
    'name' => 'link 2',
    'href' => '#',
    'links' => array(
      array(
        'name' => 'sub link 2.1',
        'href' => '#',
      ),
      array(
        'name' => 'sub link 2.2',
        'href' => '#',
      ),
    ),
  ),
);

Macro

<!--menu/menu.html.twig-->
{% macro menu_links(links) %}
  {% if links %}
    <ul>
      {% for link in links %}
        <li>
          <a href="{{ link.href }}">{{ link.name }}</a>
          {% if link.links %}
            {{ _self.menu_links(link.links) }}
          {% endif %}
        </li>
      {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

Page

{% import "menu.twig" as menu %}

{{ menu.menu_links(links) }}
hass’s picture

There is more Drupal theme_menu hell in the house. See #2119989: Add navbar_menu_tree() to prevent theme clashes and keep in mind that hook_theme_menu() has no context.

jwilson3’s picture

@Hass: I've asked for a little clarification on that issue, cause i don't understand what you're talking about there.

hass’s picture

I will write up some example code later here.

hass’s picture

hass’s picture

hass’s picture

And drupal.org needs a resubmit blocker for weak mobile connections :-)

hass’s picture

Issue summary: View changes

update issue summary

mgifford’s picture

That would make this much more flexible. Thanks for writing up the example code.

Also +1 to the resubmit blocker. #2181495: Add a resubmit button

dawehner’s picture

Status: Active » Needs review
FileSize
7.24 KB

Here is a first start. I am kind of interested how much it breaks (it does break toolbar but I don't give a shit)

Status: Needs review » Needs work

The last submitted patch, 23: menu-1777332-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.06 KB

#2239833: Regression: Menu contextual links no longer visible in menu blocks is kind of a problem detected while working on that.

Status: Needs review » Needs work

The last submitted patch, 25: menu-1777332-25.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

25: menu-1777332-25.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: menu-1777332-25.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
17.03 KB

Re-rolled #25. This looks like a huge improvement on theme_menu. It's on my wish list for x-mas, I mean... DrupalCon Austin:)

dawehner’s picture

I am glad that the other issue fixed this test failure!

  1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
    @@ -53,6 +53,9 @@ public function __construct(Connection $connection, $bin) {
    +    if ($this->bin == 'render') {
    +      return NULL;
    +    }
    

    Let's remove that debug statement again.

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -395,26 +407,42 @@ public function renderTree($tree) {
    +      if (isset($data['link']['route_name'])) {
    +        $path = $this->urlGenerator->getPathFromRoute($data['link']['route_name'], isset($data['link']['route_parameters']) ? $data['link']['route_parameters'] : array());
    +      }
    +      else {
    +        $path = $data['link']['link_path'];
    +      }
    

    On the longrun we want to use the url object here, but this it out of scope for now.

joelpittet’s picture

@dawehner removed that debug statement from #30-1. Also cleaned up some minor issues and added the variables that I could see in the template to the docblock. Not totally sold on the function name 'link_path', maybe just as simple as renaming to 'path_link' so it reads "The path's link" and not "The link's path". Though if you disagree, I'm not going to bike shed this because it's way better than "l()".

Fixed a typo menu-{{ menu-name}} and BEMized the menu_name variable to menu--{{ menu_name}}.

Hope this is cool?

webchick’s picture

Just a very small drive-by review looking at the template file only. Overall it's a huge improvement to have the menu + links in the same file, context-wise, so great!

  1. +++ b/core/modules/system/templates/menu.html.twig
    @@ -0,0 +1,37 @@
    +{% macro menu_links(items, menu_level) %}
    +  {% import _self as menus %}
    

    This is a really clever trick. But maybe add a comment above it to explain what's happening so themers know not to mess with it.

  2. +++ b/core/modules/system/templates/menu.html.twig
    @@ -0,0 +1,37 @@
    +  {{ menus.menu_links(items, 0) }}
    ...
    +            {{ menus.menu_links(item.below, menu_level + 1) }}
    

    Hm. No way to nicen that up a bit in preprocess is there? For example menus.menu_links.root and menus.menu_links.next_child ?

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

Toolbar doesn't display because I think it uses menu_links some how.

Also a comment above the macro would be great, I'd encourage them to play with it. Maybe reference the twig docs
http://twig.sensiolabs.org/doc/tags/macro.html

That level + 1 is a very common menu recursive function pattern, I'm so glad I've got access to those variables in the template! allows me to add classes based on the level so I can reflow the menu items into say second level stacked and first level horizontal(classic dropdown menu but with less css nesting ul.menu>li>ul.menu becomes .menu--level2).

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
19.05 KB

@joelpittet
Thank you for fixing the stuff I had no clue about :P
Would it be somehow possible to document the menu_level stuff?

Here is a fix for the display of the toolbar, but I have no * idea for the subtree toggles.

joelpittet’s picture

Because toolbar renders each level individually it seems through an ajax call, it's rendering the menu's wrapper as well as menu. So instead of <nav><ul><li><ul><li> it's rendering <nav><ul><li><nav><ul><li>

So we need a way to render the subtree without the wrapper nav tag. Maybe 'menu.html.twig' just renders the nav wrapper and an optional header and includes menu-tree.html.twig which is what toolbar would use and would just be {{ _self.menu_links(items, 0) }} + the {% macro menu_links(items, menu_level) %}...{% endmacro %}

That way you an avoid all those css/js changes, menu.html.twig would still get the same data as the tree would + maybe {{ attributes }} for the <nav> and a header.

That's my 2 cents, hope it's sensible?

mike.roberts’s picture

FileSize
18.86 KB

Reroll of patch in #34

pwolanin’s picture

Status: Needs review » Postponed

Can we postpone this on: #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module

In particular, the l() function is deprecated and we are converting to the use of the Url object which needs perhaps to be supported in Twig

Or at least, let's split out the menu link portions of this to for now if you want to move ahead with the others.

dawehner’s picture

Status: Postponed » Needs work

Remove postpone status

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.58 KB

Let's see how much fails after a more or less quick reroll.

Status: Needs review » Needs work

The last submitted patch, 39: menu_template-1777332-39.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: menu_template-1777332-39.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
1.44 KB

This certainly fixes a couple of the failures, obviously.

Status: Needs review » Needs work

The last submitted patch, 43: menu_template-1777332-43.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.06 KB
3.67 KB

Let's make it green and clean up more of the code.

hass’s picture

Great, we are getting the hands around the toolbar theme clashes. That looks reusable and customizable.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +TX (Themer Experience)

This is so cool. When testing with Stark there are very few markup differences even and it's fantastic to have the whole menu in one template. This really does deserve the TX tag I think :)

This issue needs a retitle and issue summary update to reflect the current proposal.

  1. We should be removing menu-tree.html.twig in this patch shouldn't we?
  2. +++ b/core/modules/system/templates/menu.html.twig
    @@ -0,0 +1,40 @@
    +{# @TODO Add a nav element around.#}
    

    This is maybe more of a dream markup thing? I don't think think we need to add a nav element to preserve current core markup so if we want this to happen it should be in a followup.

  3. +++ b/core/modules/system/templates/menu.html.twig
    @@ -0,0 +1,40 @@
    +    <ul class="menu">
    

    We probably need a way to add attributes here, at least to the top-level <ul>. Maybe we can add a wrapper_attributes Attribute object and add the menu class with addClass() but only on the first level. For example bartik_preprocess_page() adds ID attributes to menus.

  4. We might need to add the 'heading' variable from menu_tree to menu to support things like this (from template_preprocess_page()) - see this slightly related issue #2285493: Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays:
      // Pass the main menu and secondary menu to the template as render arrays.
      if (!empty($variables['main_menu'])) {
        $variables['main_menu']['#heading'] = array(
          'text' => t('Main menu'),
          'class' => array('visually-hidden'),
          'attributes' => array('id' => 'links__system_main_menu'),
        );
      }
      if (!empty($variables['secondary_menu'])) {
        $variables['secondary_menu']['#heading'] = array(
          'text' => t('Secondary menu'),
          'class' => array('visually-hidden'),
          'attributes' => array('id' => 'links__system_secondary_menu'),
        );
      }
hass’s picture

If we are able to rename the "menu" class for all toolbar menus to "menu-toolbar" here it would be perfect, too.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.88 KB
625 bytes

We should be removing menu-tree.html.twig in this patch shouldn't we?

Ha good catch!

This is maybe more of a dream markup thing? I don't think think we need to add a nav element to preserve current core markup so if we want this to happen it should be in a followup.

Yeah it is, this is why I added a todo.

We probably need a way to add attributes here, at least to the top-level

    . Maybe we can add a wrapper_attributes Attribute object and add the menu class with addClass() but only on the first level. For example bartik_preprocess_page() adds ID attributes to menus.

Does the proposed change works for you?

We might need to add the 'heading' variable from menu_tree to menu to support things like this (from template_preprocess_page()) - see this slightly related issue #2285493: Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays:

This actually sounds like a template override? Not sure having a twig block here would make that really handy.

If we are able to rename the "menu" class for all toolbar menus to "menu-toolbar" here it would be perfect, too.

I know that there exists a dedicated issue for that already.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

mh, okay.

star-szr’s picture

+++ b/core/modules/system/templates/menu.html.twig
@@ -26,7 +26,11 @@
+      <ul {{ attributes.addClass('menu') }}>

Cool, we should remove the space between <ul and {{.

star-szr’s picture

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,44 @@
+    {%  endif %}

Nit: Two spaces after {%

Status: Needs review » Needs work

The last submitted patch, 49: menu_links-1777332-49.patch, failed testing.

sun’s picture

While renaming all the things, shouldn't this actually be called nav.html.twig…?

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
15.67 KB

@sun
Nothing is called nav at the moment, not even in the UI.

Status: Needs review » Needs work

The last submitted patch, 55: menu_links-1777332-55.patch, failed testing.

joelpittet’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.73 KB
1.39 KB

Oh that was more complex than I expected it to be.

Fabianx’s picture

FWIW, Patch looks great to me!

dawehner’s picture

Issue tags: +Needs benchmarks

I guess as for everything we need some performance test? Would be interesting whether moving things into the twig layer actually improves things.

Wim Leers’s picture

I think #2324661: Move menu classes from preprocess to templates will conflict with this heavily. Might want to figure out which should go in first.

dawehner’s picture

I think #2324661: Move menu classes from preprocess to templates will conflict with this heavily. Might want to figure out which should go in first.

You could have reviewed this issue instead and set one as RTBC instead of doing meta work.

Wim Leers’s picture

You could have reviewed this issue instead and set one as RTBC instead of doing meta work.

You could be more considerate.

I'm not qualified to review either, since I'm no Twig expert. I just encountered the other issue, and thought I'd prevent potential frustration for you by preventing a conflict.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,44 @@
+ *   - title: The menu link title.
+ *   - href: The menu link url.
+ *   - localized_options: Menu link localized options.

href is wrong here and should be url, also should (not sure how) also annotate that this is class URL.

Will the following work?

{{ link_from_url('My menu item', url_from_path('node/1')) }}

Just curious.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.75 KB
4.11 KB

Will the following work?
{{ link_from_url('My menu item', url_from_path('node/1')) }}

It will be now.

Status: Needs review » Needs work

The last submitted patch, 66: menu-1777332-66.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.09 KB
2.34 KB

Meh.

star-szr’s picture

Title: Remove all specific menu link theme functions, use theme_menu() with suggestions instead » Replace theme_menu_tree and theme_menu_link with a single Twig template
Issue summary: View changes
Status: Needs review » Needs work

Issue summary could probably use some expanding but this at least reflects more what is happening now.

Patch is looking really good overall, points below are minor. Thanks for your continuous work on this @dawehner!

  1. +++ b/core/includes/theme.inc
    @@ -2361,12 +2361,9 @@ function drupal_common_theme() {
    +    'menu' => array(
    +      'variables' => array('items' => array(), 'attributes' => []),
    +      'template' => 'menu',
    

    I'm not sure if this has been brought up but I have to ask, why array() for items and [] for attributes?

  2. +++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
    @@ -63,4 +64,24 @@ public function testTwigUrlGenerator() {
    +    debug($content);
    

    I guess this should be removed :)

  3. +++ b/core/modules/system/templates/menu.html.twig
    @@ -0,0 +1,44 @@
    +{#We call a macro which calls itself to render the full tree.
    +@see http://twig.sensiolabs.org/doc/tags/macro.html
    +#}
    

    Can we reformat this to match our coding standard here please: https://www.drupal.org/node/1823416#comments

  4. +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    @@ -6,6 +6,7 @@
     namespace Drupal\twig_theme_test;
    +use Drupal\Core\Url;
    

    Super minor: I think there should be a blank line between namespace and use.

  5. +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    @@ -37,4 +38,14 @@ public function urlGeneratorRender() {
    +    return array(
    +      '#theme' => 'twig_theme_test_link_generator',
    +      '#test_url' => new Url('user.register')
    +    );
    

    Minor: Trailing comma for the last array item?

star-szr’s picture

Title: Replace theme_menu_tree and theme_menu_link with a single Twig template » Replace theme_menu_link() and menu-tree.html.twig with a single Twig template
dawehner’s picture

Status: Needs work » Needs review
FileSize
18.08 KB
2.56 KB

Thank you for your review.

I'm not sure if this has been brought up but I have to ask, why array() for items and [] for attributes?

Let's just not use short array syntax for now here.

Fixed the remaining bits.

joelpittet’s picture

I've been trying to follow your lead on the [] syntax, I love it, but maybe you're keeping it out to keep the patch cleaner?

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,45 @@
+{# @TODO Add a nav element around. #}

I'm not sure this is needed. Seems like a better place for the nav element is the block that this is rendered into, or the template that it's rendered into. That way if you are getting creative with doing some disjointed menus or fancy mega menu you can do this easily {{ primary }}{{ secondary }}

Maybe this is short sighted... but I do know that is where my head was going over on this issue #1869476-184: Convert global menus (primary links, secondary links) into blocks

Maybe we can get rid of the @todo and let that get decided elsewhere / followup?

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,45 @@
+          {{ link_from_url(item.title, item.url) }}

Is it possible to add attributes as well (aka through options maybe like l() used to have)? I'd be tempted to call that function just link()

Wim Leers’s picture

#72: I also told dawehner in IRC that the <nav> problem is being fixed in a complementary way in #1869476: Convert global menus (primary links, secondary links) into blocks :) But of course, we want to prevent a regression in the mean time…

dawehner’s picture

Is it possible to add attributes as well (aka through options maybe like l() used to have)? I'd be tempted to call that function just link()

Not at the moment, can we please defer this into a future issue? I think this doesn't belong into this issue but certainly would be really nice

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - Looks great to me, test coverage is good, syntax is clean and thanks a lot for making it possible to do, e.g.:

link_from_url('x', url_from_path('node/1'));

Cool!

star-szr’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs issue summary update
FileSize
20.91 KB
2.98 KB

Attached is one way to prevent markup regressions, adding this markup as a prefix:

<h2 id="links__system_main_menu" class="visually-hidden">Main menu</h2>

This patch also fixes a small markup regression in Bartik and updates some docs. There was still bartik_menu_tree__shortcut_default() but I removed that because it looks to be dead code, it was originally added in #1937926: Shortcuts toolbar tray does not properly float the "Edit shortcuts" link in Bartik and appears to be unused.

Wim Leers’s picture

Cottser++.

joelpittet’s picture

Re #74 It would be an API change if that needs a rename after it gets into core, no?

link_from_url = which is how all links work. (well href I guess)
link()

Should it work for all cases like?

link('title', '')
link('title', 'path/to/url')
link('title', '#history')
link('title', 'http://domain.com/path/to/url')
link('title', UrlObject)

Sorry if I'm rehashing old things here, and maybe that's a reason it needs to stay link_from_url but I think those arguments should work, no?

I've opened up a followup to add attributes to the methods. #2342745: Allow Twig link function to pass in HTML attributes

Also a bit strange the Url Object carries the 'options'/'attributes'. That must be form some backwards compatibility though seems like the link's responsibility to keep track of those and use the Url Object to determine things like 'is-active' class.

joelpittet’s picture

Here's an patch, but I'm not sure if I'm hosing something else with it...

If you agree, I'll gladly open another issue to add tests for those cases if we can get the name change in there or do them here... you're call.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1 for the changes in #79!
+2 For the fix of markup regressions in #76!!!!!!

mgifford’s picture

@dawehner - Sorry, are you RTBC'ing @joelpittet patch? It might be perfect, but doesn't it have to run by the bots before it can be RTBC?

star-szr’s picture

FileSize
20.87 KB

Okay, well here is #76 + #79 then :)

star-szr’s picture

And no I'm not that quick, it was a crosspost!

joelpittet’s picture

Thanks RTBC +1. This is by far my most anticipated patch at the moment!

Wim Leers’s picture

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,45 @@
+{# @TODO Add a nav element around. #}

This should still be removed since Cottser added a work-around. Leaving at RTBC: the committer might want to remove this on commit. Otherwise this could use a reroll in the mean time.

star-szr’s picture

joelpittet’s picture

Oh you are too fast! Crosspost!

hass’s picture

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,44 @@
+    {% if menu_level == 0 %}
+      <ul{{ attributes.addClass('menu') }}>
+    {% else %}
+      <ul class="menu">
+    {% endif %}
+      {% for item in items %}

Sorry, but what is the use case here? Why are we not using only one UL class for all sub menus? That looks like a new feature to me.

star-szr’s picture

The use case is being able to pass in attributes that only apply to the top-level UL. So we can replicate this:

+++ /dev/null
@@ -1,40 +0,0 @@
-  <ul{{ attributes }}>
-    {{ tree }}
-  </ul>

The above is from menu-tree.html.twig (removed in this patch).

Edit: See #47 to ~#55 for discussion and interdiffs around this change.

star-szr’s picture

Issue tags: -Needs benchmarks
FileSize
44.21 KB

Performance is looking great to me! I set up a scenario with 20 instances of menu-tree.html.twig on the page, most of which was this made-up menu with everything set to expanded:

=== 8.0.x..8.0.x compared (5422297e7f32f..542229efabc5e):

ct  : 89,445|89,445|0|0.0%
wt  : 577,681|577,197|-484|-0.1%
cpu : 561,966|561,581|-385|-0.1%
mu  : 47,049,216|47,049,216|0|0.0%
pmu : 47,128,624|47,128,624|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5422297e7f32f&...

=== 8.0.x..menu-macro-1777332-86 compared (5422297e7f32f..54222a51d4172):

ct  : 89,445|85,207|-4,238|-4.7%
wt  : 577,681|560,857|-16,824|-2.9%
cpu : 561,966|545,026|-16,940|-3.0%
mu  : 47,049,216|46,731,216|-318,000|-0.7%
pmu : 47,128,624|46,806,120|-322,504|-0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5422297e7f32f&...

Wim Leers’s picture

So: a nice improvement, albeit in a pretty extreme scenario. Great!

star-szr’s picture

star-szr’s picture

By the way, render caching was disabled for the profiling. Forgot to mention that.

joelpittet’s picture

@Cottser didn't doubt you had that off for a second:) Nice improvement on every level!

hass queued 86: 1777332-86.patch for re-testing.

hass’s picture

Tried to apply patch, but has hunk in MenuLinkTree.php. Retesting.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 86: 1777332-86.patch, failed testing.

hass’s picture

hass’s picture

Status: Needs work » Needs review
FileSize
20.25 KB
hass’s picture

Tried creating menu--main.html.twig in seven/templates folder, cleared cache - but the file is not used. and seven_preprocess_menu__main() is also not invoked. What I'm doing wrong here?

star-szr’s picture

I was able to create menu--main.html.twig in Bartik without any issue, have you set Seven as your default theme?

Re: seven_preprocess_menu__main() not being invoked - #939462: Specific preprocess functions for theme hook suggestions are not invoked

star-szr’s picture

Oh, and thanks for the reroll!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I downloaded the patches in #86 and #90, diffed them, and only found changes in context. That explains the different patch sizes. Looks like a clean reroll :) Hence back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome sauce. :)

Committed and pushed to 8.x. Thanks!

  • webchick committed 35fcfc4 on 8.0.x
    Issue #1777332 by hass, Cottser, mike.roberts, joelpittet, dawehner |...
hass’s picture

This seems very limited code or I just cannot find a way how I can add a theme function to toolbars. build() set's $build['#theme'] = 'menu__' . strtr($menu_name, '-', '_'); and does not allow me to inject my own theme key. If a toolbar menu is build we need $build['#theme'] = 'menu-toolbar'; and maybe more suggestions like $build['#theme'] = 'menu-toolbar__' . strtr($menu_name, '-', '_');. Any idea how this can be accomplished or what I'm doing wrong?

On the other side I'm not sure how I should add a suggestion only to toolbar menus if they are build for toolbar.

hass’s picture

Found it now. it must be named menu--admin.html.twig. But if I move this file to core/toolbar/templates this file is not used. It is only used if it is save in core/themes/seven.

How can we fix this to that menu--admin.html.twig is also used if it's located in core/toolbar/templates?

star-szr’s picture

@hass - to add a template suggestion to a module you need to register it in hook_theme(), see comment_theme() or node_theme() for examples.


I've asked @Mark Carver if he can make a new Dreditor release to include the fix for https://github.com/dreditor/dreditor/pull/227/, the commit message on this one would normally be more like:

Issue #1777332 by dawehner, Cottser, joelpittet, hass, mike.roberts | jenlampton: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template.

Because @dawehner did the lion's share here.

Either way, I'm so happy to see this in!

hass’s picture

@Cottser: It is not that easy as this would affect all menus. We need a custom menu theme for all menus in the toolbar. If the default template is used we end with the "ul.menu" class that causes theme clashes. Therefore I think we need the ability to inject the theme function in MenuLinkTree build() like $build['#theme'] = 'menu-toolbar'; plus suggestions... I tried to implement this several hours but I do not get it. I guess we need one more param in the build(array $tree, $level = 0, $build = array()) { function.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

+++ b/core/modules/system/templates/menu.html.twig
@@ -0,0 +1,44 @@
+ * - menu_name: The machine name of the menu.

From what I can tell, nothing before this patch, in this patch, or since this patch, set or used this variable. Please see #2609400: menu.html.twig says that menu_name is an available variable, but it's not for either removing it from the docs or clarifying its purpose and making it work.