Problem/Motivation

Follow-up to #1944572: Remove "ul.menu" dependency to prevent theme clashes

dawehner brought up the idea to optimize code. I tried it by adding the wrapper_menu_tree and wrapper_menu_subtree blocks in

[core/modules/system/templates/menu.html.twig]

{#
/**
 * @file
 * Default theme implementation to display a menu.
 *
 * Available variables:
 * - menu_name: The machine name of the menu.
 * - items: A nested list of menu items. Each menu item contains:
 *   - attributes: HTML attributes for the menu item.
 *   - below: The menu item child items.
 *   - title: The menu link title.
 *   - url: The menu link url, instance of \Drupal\Core\Url
 *   - localized_options: Menu link localized options.
 *
 * @ingroup themeable
 */
#}
{% import _self as menus %}

{#
  We call a macro which calls itself to render the full tree.
  @see http://twig.sensiolabs.org/doc/tags/macro.html
#}
{{ menus.menu_links(items, attributes, 0) }}

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    {% if menu_level == 0 %}
      {% block wrapper_menu_tree %}<ul{{ attributes.addClass('menu') }}>{% endblock %}
    {% else %}
      {% block wrapper_menu_subtree %}<ul class="menu">{% endblock %}
    {% 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 %}

[core/modules/toolbar/templates/toolbar-menu.html.twig]

{% extends "menu.html.twig" %}
{% block wrapper_menu_tree %}<ul{{ attributes.addClass('toolbar-menu') }}>{% endblock %}
{% block wrapper_menu_subtree %}<ul class="toolbar-menu">{% endblock %}

But it looks like this is not working at all. If cache cleared I see the string menu.html.twig under the toolbar what means something like the template menu.html.twig cannot found as I know. Not sure what is wrong here. At least it is not working as expected.

Proposed resolution

Optimize the core/modules/system/templates/menu.html.twig by adding twig blocks.

Remaining tasks

Investigate why the example code does not work.

User interface changes

None.

API changes

None.

Comments

dawehner’s picture

Afaik you would have to use:

{% extends @system/menu.html.twig %}

hass’s picture

Status: Needs review » Active

Fixing wrong status.

ok. What is the @ in front of system mean? Is this documented somewhere or just drupalism?

hass’s picture

  1. {% extends @system/menu.html.twig %} Uncaught PHP Exception Twig_Error_Syntax: "Unexpected character "@" in "core/modules/toolbar/templates/toolbar-menu.html.twig" at line 1" at core\vendor\twig\twig\lib\Twig\Lexer.php line 284
  2. {% extends @system/templates/menu.html.twig %}
    Same error as #1
  3. {% extends "@system/templates/menu.html.twig" %}
    Does not work
dawehner’s picture

I shouldn't have typed it out.

You should have searched for it :)

{% extends "@block/block.html.twig" %}

This is a literal example

hass’s picture

Oh, I tested this but missed to document it.

The problem is now that the block override does not work. The menu.html.twig is used and the blocks are not overridden. But strange thing... twig debug shows that is uses the toolbar-menu template - only the blocks are not overridden.

<!-- THEME DEBUG --><!-- THEME HOOK: 'menu__admin' --><!-- FILE NAME SUGGESTIONS:
   * menu--admin.html.twig
   * menu.html.twig
--><!-- BEGIN OUTPUT from 'core/modules/toolbar/templates/toolbar-menu.html.twig' -->
hass’s picture

Maybe a reason why this code is not working.

hass’s picture

That is where the literal has been introduced.

hass’s picture

Title: Optimize theme templates with Twig template inheritance » Optimize toolbar-menu template with Twig template inheritance
davidhernandez’s picture

Issue tags: +Twig

You are extended menu.html.twig, correct? This might be one of those weirdly non-obvious things, because even though the menu template lives in the system module, it is not registered by the system module. It is registered by theme.inc. You might have to use @core or something like that.

Also, if this is extending the menu template, it would probably be named menu--toolbar.twig.html? But I believe you are encountering a different problem. Toolbar doesn't have its own menu, it uses the admin menu. That is why the twig debug output is showing menu--admin.html.twig.

hass’s picture

With @system/menu.html.twig Drupal outputs the menu.html.twig html code, but does not override the blocks. With @core/menu.html.twig a string @core/menu.html.twig is shown on the page. This means the template with this name has not been found as I know. I can safely say that toolbar-menu.html.twig is used as every char of invalid syntax in this file causes a site error. Only the blocks are not replaced... :-(

Also, if this is extending the menu template, it would probably be named menu--toolbar.twig.html? But I believe you are encountering a different problem. Toolbar doesn't have its own menu, it uses the admin menu. That is why the twig debug output is showing menu--admin.html.twig.

Correct, toolbar is not the menu name and menu template and menu_tree functions are totally inflexible, have no context, only useless suggestions and so on. Nothing helpful and that is why we need to override menu__admin in toolbar module with our own theme function.

To understand the issue you need the patch in #1944572: Remove "ul.menu" dependency to prevent theme clashes. This case here is a follow up, but until today it looks like the extends is not working at all. Maybe because of the name change I implemented in the override, but I see no obvious reason why.

I'm new to Twig and I have totally no clue why the overriding of blocks should not possible. The file names we use in Drupal are something Drupal specific as I know and Twig I guess do not have such naming structure.

Cottser’s picture

#2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders makes extending just "menu.html.twig" work.
#2369981: Not found templates are displayed literally instead of throwing an Exception due to string loader gives actual exceptions rather than printing "menu.html.twig" when the template cannot be found :)

Fabianx’s picture

The reason the override likely is not working is due to the original code being a macro, hence a function and those - unless I am mistaken cannot be 'block'-ified.

Cottser’s picture

That is also a strong possibility :) never tried it myself and not sure what the docs say on that.

Cottser’s picture

Category: Bug report » Task

Doesn't really seem like a bug, more of a refactor/optimization.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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

Status: Active » Closed (duplicate)
Related issues: +#2490308: Refactor the macro on menu.html.twig for reusability

The reason the override likely is not working is due to the original code being a macro, hence a function and those - unless I am mistaken cannot be 'block'-ified.

I've confirmed this. I put blocks inside the macro in "@classy/menu.html.twig" and then altered stable's menu--toolbar.html.twi to {% extends "menu.html.twig" %} and, while it definitely did extend the correct template, it did not recognize the blocks I added inside the macro.

I think this issue should be marked as a duplicate of #2490308: Refactor the macro on menu.html.twig for reusability because:

  • That issue also wants to make menu.html.twig reusable as this issue does.
  • The method tried in this issue doesn't work.
  • There are more followers and patches on that issue.