Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
Comment | File | Size | Author |
---|---|---|---|
#99 | Issue-1777332-Replace-thememenulink-and-menutreehtml.patch | 20.25 KB | hass |
#90 | 1777332-90-test-menu.png | 44.21 KB | star-szr |
#86 | interdiff.txt | 401 bytes | star-szr |
#86 | 1777332-86.patch | 20.83 KB | star-szr |
Comments
Comment #1
sunClosely 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.
Comment #2
sunClarifying title.
Comment #3
jwilson3seems 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,
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.
Comment #3.0
jenlamptonupdate
Comment #4
jenlamptonLet's go with theme_links instead of theme_item_list, so how about:
or
This may require a little re-wiring but I think it will make more sense to people.
Comment #5
jenlamptonChanging 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
Comment #5.0
jenlamptonadd menu
Comment #6
jenlamptonmore tagging
Comment #7
jenlamptonMy brain is telling me that we decided not to name menus with links. So... how about this:
All menus:
Local tasks can be themed like this:
Local actions can be themed like this:
Comment #7.0
jenlamptonmore sense
Comment #8
BarisW CreditAttribution: BarisW commentedUpdating title.
Comment #9
BarisW CreditAttribution: BarisW commentedA lot of work is already done here: #1898478: menu.inc - Convert theme_ functions to Twig.
Comment #10
hass CreditAttribution: hass commentedWe 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?
Comment #11
star-szr@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.
Comment #12
hass CreditAttribution: hass commentedI'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.
Comment #13
star-szrI 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.
Comment #14
hass CreditAttribution: hass commentedAs 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. :-)Comment #15
joelpittet*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
Macro
Page
Comment #16
hass CreditAttribution: hass commentedThere 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.Comment #17
jwilson3@Hass: I've asked for a little clarification on that issue, cause i don't understand what you're talking about there.
Comment #18
hass CreditAttribution: hass commentedI will write up some example code later here.
Comment #19
hass CreditAttribution: hass commentedComment #20
hass CreditAttribution: hass commentedComment #21
hass CreditAttribution: hass commentedAnd drupal.org needs a resubmit blocker for weak mobile connections :-)
Comment #21.0
hass CreditAttribution: hass commentedupdate issue summary
Comment #22
mgiffordThat would make this much more flexible. Thanks for writing up the example code.
Also +1 to the resubmit blocker. #2181495: Add a resubmit button
Comment #23
dawehnerHere is a first start. I am kind of interested how much it breaks (it does break toolbar but I don't give a shit)
Comment #25
dawehner#2239833: Regression: Menu contextual links no longer visible in menu blocks is kind of a problem detected while working on that.
Comment #27
catch25: menu-1777332-25.patch queued for re-testing.
Comment #29
joelpittetRe-rolled #25. This looks like a huge improvement on theme_menu. It's on my wish list for x-mas, I mean... DrupalCon Austin:)
Comment #30
dawehnerI am glad that the other issue fixed this test failure!
Let's remove that debug statement again.
On the longrun we want to use the url object here, but this it out of scope for now.
Comment #31
joelpittet@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 tomenu--{{ menu_name}}
.Hope this is cool?
Comment #32
webchickJust 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!
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.
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 ?
Comment #33
joelpittetToolbar 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
).Comment #34
dawehner@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.
Comment #35
joelpittetBecause 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?
Comment #36
mike.roberts CreditAttribution: mike.roberts commentedReroll of patch in #34
Comment #37
pwolanin CreditAttribution: pwolanin commentedCan 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.
Comment #38
dawehnerRemove postpone status
Comment #39
dawehnerLet's see how much fails after a more or less quick reroll.
Comment #43
dawehnerThis certainly fixes a couple of the failures, obviously.
Comment #45
dawehnerLet's make it green and clean up more of the code.
Comment #46
hass CreditAttribution: hass commentedGreat, we are getting the hands around the toolbar theme clashes. That looks reusable and customizable.
Comment #47
star-szrThis 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.
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.
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.Comment #48
hass CreditAttribution: hass commentedIf we are able to rename the "menu" class for all toolbar menus to "menu-toolbar" here it would be perfect, too.
Comment #49
dawehnerHa good catch!
Yeah it is, this is why I added a todo.
Does the proposed change works for you?
This actually sounds like a template override? Not sure having a twig block here would make that really handy.
I know that there exists a dedicated issue for that already.
Comment #50
dawehnermh, okay.
Comment #51
star-szrCool, we should remove the space between
<ul
and{{
.Comment #52
star-szrNit: Two spaces after
{%
Comment #54
sunWhile renaming all the things, shouldn't this actually be called
nav.html.twig
…?Comment #55
dawehner@sun
Nothing is called nav at the moment, not even in the UI.
Comment #57
joelpittetAdding a couple mildly related issues.
Comment #58
joelpittetComment #59
dawehnerOh that was more complex than I expected it to be.
Comment #60
Fabianx CreditAttribution: Fabianx commentedFWIW, Patch looks great to me!
Comment #61
dawehnerI guess as for everything we need some performance test? Would be interesting whether moving things into the twig layer actually improves things.
Comment #62
Wim LeersI think #2324661: Move menu classes from preprocess to templates will conflict with this heavily. Might want to figure out which should go in first.
Comment #63
dawehnerYou could have reviewed this issue instead and set one as RTBC instead of doing meta work.
Comment #64
Wim LeersYou 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.
Comment #65
Fabianx CreditAttribution: Fabianx commentedhref 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.
Comment #66
dawehnerIt will be now.
Comment #68
dawehnerMeh.
Comment #69
star-szrIssue 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!
I'm not sure if this has been brought up but I have to ask, why array() for items and [] for attributes?
I guess this should be removed :)
Can we reformat this to match our coding standard here please: https://www.drupal.org/node/1823416#comments
Super minor: I think there should be a blank line between namespace and use.
Minor: Trailing comma for the last array item?
Comment #70
star-szrComment #71
dawehnerThank you for your review.
Let's just not use short array syntax for now here.
Fixed the remaining bits.
Comment #72
joelpittetI'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?
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?
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()
Comment #73
Wim Leers#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…Comment #74
dawehnerNot 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
Comment #75
Fabianx CreditAttribution: Fabianx commentedRTBC - 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!
Comment #76
star-szrAttached 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.
Comment #77
Wim LeersCottser++.
Comment #78
joelpittetRe #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?
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.
Comment #79
joelpittetHere'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.
Comment #80
dawehner+1 for the changes in #79!
+2 For the fix of markup regressions in #76!!!!!!
Comment #81
mgifford@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?
Comment #82
star-szrOkay, well here is #76 + #79 then :)
Comment #83
star-szrAnd no I'm not that quick, it was a crosspost!
Comment #84
joelpittetThanks RTBC +1. This is by far my most anticipated patch at the moment!
Comment #85
Wim LeersThis 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.
Comment #86
star-szrSure, let's 86 that.
Comment #87
joelpittetOh you are too fast! Crosspost!
Comment #88
hass CreditAttribution: hass commentedSorry, 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.
Comment #89
star-szrThe use case is being able to pass in attributes that only apply to the top-level UL. So we can replicate this:
The above is from menu-tree.html.twig (removed in this patch).
Edit: See #47 to ~#55 for discussion and interdiffs around this change.
Comment #90
star-szrPerformance 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:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5422297e7f32f&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5422297e7f32f&...
Comment #91
Wim LeersSo: a nice improvement, albeit in a pretty extreme scenario. Great!
Comment #92
star-szrComment #93
star-szrBy the way, render caching was disabled for the profiling. Forgot to mention that.
Comment #94
joelpittet@Cottser didn't doubt you had that off for a second:) Nice improvement on every level!
Comment #96
hass CreditAttribution: hass commentedTried to apply patch, but has hunk in MenuLinkTree.php. Retesting.
Comment #98
hass CreditAttribution: hass commentedNeeds re-role. Hunk is caused by #2340123: Setting cache tags can be tricky: use strings instead of nested arrays to improve DX
Comment #99
hass CreditAttribution: hass commentedComment #100
hass CreditAttribution: hass commentedTried creating
menu--main.html.twig
inseven/templates
folder, cleared cache - but the file is not used. andseven_preprocess_menu__main()
is also not invoked. What I'm doing wrong here?Comment #101
star-szrI 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
Comment #102
star-szrOh, and thanks for the reroll!
Comment #103
Wim LeersI 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.
Comment #104
webchickAwesome sauce. :)
Committed and pushed to 8.x. Thanks!
Comment #106
hass CreditAttribution: hass commentedThis 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.
Comment #107
hass CreditAttribution: hass commentedFound it now. it must be named
menu--admin.html.twig
. But if I move this file tocore/toolbar/templates
this file is not used. It is only used if it is save incore/themes/seven
.How can we fix this to that
menu--admin.html.twig
is also used if it's located incore/toolbar/templates
?Comment #108
star-szr@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:
Because @dawehner did the lion's share here.
Either way, I'm so happy to see this in!
Comment #109
hass CreditAttribution: hass commented@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 thebuild(array $tree, $level = 0, $build = array()) {
function.Comment #111
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom 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.