Follow-up to #2443361: Remove theme_book_link, make book tree align with MenuLinkTree build, where the need for this became obvious.

Problem/Motivation

The template file core/modules/system/templates/menu.html.twig includes this twig macro:

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    {% if menu_level == 0 %}
      <ul{{ attributes.addClass('menu') }}>
    {% else %}
      <ul class="menu">
    {% endif %}
      {% for item in items %}
        <li{{ item.attributes }}>
          {{ link(item.title, item.url) }}
          {% if item.below %}
            {{ menus.menu_links(item.below, attributes, menu_level + 1) }}
          {% endif %}
        </li>
      {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

This is very useful for rendering not only menus, but any tree of links. However, this macro is not currently reusable, as this file is actually a template.
We could all make good use of the code if we tackle this, both within core and contrib.

See documentation on twig macros: http://twig.sensiolabs.org/doc/tags/macro.html

Proposed resolution

Exact solution is up for discussion. But any solution must be re-usable by book-tree.html.twig and menu--toolbar.html.twig at the very least.

Remaining tasks

  • Generalize the macro. Ensure backwards-compatibility with old version.
  • Figure out where else we can use this in core. For now we know:
    • core/modules/book/templates/book-tree.html.twig
    • core/modules/system/templates/menu.html.twig
    • core/modules/toolbar/templates/menu--toolbar.html.twig
    • + core/themes/stable versions of the above
    • core/themes/classy/templates/navigation/book-tree.html.twig
    • core/themes/classy/templates/navigation/menu.html.twig
    • core/themes/classy/templates/navigation/menu--toolbar.html.twig
  • Make the changes on all those places.

User interface changes

None

API changes

None, adding a general core twig macro, which means a new tool for themers to use.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug/Task/Feature because ...
Issue priority Major because ... Critical/Not critical because ...

Comments

joelpittet’s picture

Since all other template files are defined via hook_theme() and this macro would likely live outside of that? we may need to explain how people can override it by likely copying it to their template and make sure the @bartik/menu-macro.html.twig macro would work the same way if possible.

Thanks for making this follow-up @Manuel Garcia!

I think it should work well. Still debating the attributes parameter in my head...

Manuel Garcia’s picture

Issue summary: View changes
Manuel Garcia’s picture

Yeah it would be outside of hook_theme... this would be just a twig macro. I could see other modules using it as well, think taxonomy_menu for example.

Cottser’s picture

Just want to mention, based on all the template overrides of this already the macro would still need to support setting classes on the top level ul, that seems to be the primary thing that changes, at least in core :)

joelpittet’s picture

Good point, @sqndr were pondering that too.

Anybody in contrib could just replace the macro include with their own which is cool if they want additional functionality but we should cover our core use case like @Cottser mentioned.

Wim Leers’s picture

#4++ — exactly!

davidhernandez’s picture

So, if this thing exists essentially outside the theme registry, where does it live? If someone wants to change it, they need to hack core to change the import path or override every template that uses it just to change all the paths.

...or are you talking about having one per theme? That sounds a bit less reusable.

Cottser’s picture

Haven't thought about this too much but we may actually want to just add it to the theme registry and then let the registry loader load it so it can be overridden like you would any other template.

If we don't want to do that we should still be able to use the @namespace/menu-macro.html.twig approach.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev

I could be wrong but I think we may be able to tackle this in 8.1.x if not then 9 depending on the scope of this.

lauriii’s picture

Status: Active » Postponed

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

JohnAlbin’s picture

Issue summary: View changes
Status: Postponed » Needs review
FileSize
2.28 KB

However, this macro is not currently reusable, as this file is actually a template.

The docs for importing macros say you can put macros in templates. http://twig.sensiolabs.org/doc/tags/import.html

I just tested this and it works fine:

{% import "menu.html.twig" as menus %}
{{ menus.menu_links(items, attributes, 1) }}

So that solves that problem. The macro can stay where it is and can be overridden like any other theme hook.

I actually wrote a much more extensible menu macro a couple days ago for a project, but I threw it away in favor of using a more powerful recursive template that includes twig blocks. Here it is in case someone finds it useful.

Status: Needs review » Needs work

The last submitted patch, 12: 2490308-12-menu-macro.patch, failed testing.

JohnAlbin’s picture

Issue summary: View changes

The macro needs to be backwards compatible.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.