The menu_links macro in menu.html.twig wraps its {% for item in items %} in a {% if items %} to prevent the opening and closing ul from outputing. That's a standard PHP pattern. But it is NOT a standard Twig pattern. Twig has a special loop variable that is built specifically for cases like this, the loop variable. http://twig.sensiolabs.org/doc/tags/for.html#the-loop-variable

Currently we do this:

  {% if items %}
    {% if menu_level == 0 %}
      <ul{{ attributes.addClass('menu') }}>
    {% else %}
      <ul class="menu">
    {% endif %}
    {% for item in items %}
      <li>…
    {% endfor %}
    </ul>
  {% endif %}

But we should use Twig's loop variable like this instead:

  {% for item in items %}
    {% if loop.first %}
      {% if menu_level == 0 %}
        <ul{{ attributes.addClass('menu') }}>
      {% else %}
        <ul class="menu">
      {% endif %}
    {% endif %}
    <li>…
    {% if loop.last %}
      </ul>
    {% endif %}
  {% endfor %}

Additionally, our if-ul-else-ul-endif construct means we have 2 opening ul tags and 1 closing ul tags. This causes a Twig analysis "error" in IDEs like PhpStorm:

Element ul is not closed

 Element ul is not closed

So this is even better:

  {% for item in items %}
    {% if loop.first %}
      <ul
        {% if menu_level == 0 %}
          {{ attributes.addClass('menu') }}
        {% else %}
          class="menu"
        {% endif %}
      >
    {% endif %}
    <li>…
    {% if loop.last %}
      </ul>
    {% endif %}
  {% endfor %}

Comments

JohnAlbin created an issue. See original summary.

JohnAlbin’s picture

Status: Active » Needs review
FileSize
9 KB
Cottser’s picture

Issue tags: +Twig

Cool :)

JohnAlbin’s picture

Issue summary: View changes

Whoops. Fixing final code snippet in the issue summary. I copied and pasted the wrong thing.

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.

Sutharsan’s picture

Patch looks and works good. But personally I do not like the white space it introduces in the <ul>. This is an HTML snippet:

<!-- BEGIN OUTPUT from 'core/themes/classy/templates/navigation/menu.html.twig' -->

              <ul
                   class="clearfix menu"
              >
            <li class="menu-item menu-item--expanded">
      <a href="/" data-drupal-link-system-path="&lt;front&gt;">Home</a>
                            <ul
                  class="menu"
              >
            <li class="menu-item menu-item--collapsed">
      <a href="/user" data-drupal-link-system-path="user">sub</a>
          </li>

The attached patch adds {% spaceless %}.

Sutharsan’s picture

Result after the patch:

<!-- BEGIN OUTPUT from 'core/themes/classy/templates/navigation/menu.html.twig' -->

              <ul class="clearfix menu">
            <li class="menu-item menu-item--expanded">
      <a href="/" data-drupal-link-system-path="&lt;front&gt;">Home</a>
                            <ul class="menu">
            <li class="menu-item menu-item--collapsed">
      <a href="/user" data-drupal-link-system-path="user">sub</a>
          </li>
Sumit kumar’s picture

@Sutharsan patch is working it showing ul and li as you define in your comment #7

<ul class="clearfix menu">
     <li class="menu-item menu-item--expanded">
        <a href="/" data-drupal-link-system-path="&lt;front&gt;">Home</a>
        <ul class="menu">
            <li class="menu-item menu-item--collapsed">
            <a href="/user" data-drupal-link-system-path="user">Test</a>
     </li>
vaplas’s picture

Thanks for this issue, but it does not look like a cool. IMHO, here it looks cool:

{% if items %}
    # set attributes by conditions
    <ul{{ attributes }}>
    {% for item in items %}
        <li>…
    {% endfor %}
    </ul>
{% endif %}

And the page of TWIG documentation does not describe 'for + loop variable' as a pattern for this case. But I found another page, where in the second sample uses.. if for this case :)

I hope my comment will understand correctly.

Sutharsan’s picture

I agree that # set attributes by conditions would be cool. But I have not found a way to create an attributes object in a twig template, i.e. an attribute with class="menu".

vaplas’s picture

Yeah, you're right! Although I have a few ways to create it, but their implementation is not real in this world)

1. The Ugly: just clone, and unset all property

{% set new_attributes = attributes %}
{% set new_attributes = new_attributes.__clone() %}
{% for key in new_attributes.storage()|keys %}
    {% set _ = new_attributes.offsetUnset(key) %}
{% endfor %}

...
{% set new_attributes = new_attributes.addClass('menu') %}

2. The Bad: add new method to the core/lib/Drupal/Core/Template/Attribute.php

public function instance($attributes = array()) {
    return new Attribute($attributes);
}

#twig
{% set new_attributes = attributes.instance() %}
{% set new_attributes = attributes.addClass('menu') %}
# or
{% set new_attributes = attributes.instance({'class': 'menu'}) %}

3. The Good: architectural solution in Drupal, giving their own attributes for all sub-menu (not only for root menu).

#using idea:

{% import _self as menus %}
{{ menus.menu_links(items, menu_attributes, 0) }}

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    <ul{{ attributes }}>
    {% for item in items %}
      <li{{ item.attributes }}>..
        {% if item.below %}
          {{ menus.menu_links(item.below, item.menu_attributes, menu_level + 1) }}
        {% endif %}
      </li>
    {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

But the goal of my post #9 was just to draw attention to the fact that

for 
  if
    if
  code
  if

is worse than

if
  if
  for
    code

Thanks!

Jeff Burnz’s picture

Whitespace can easily be removed, e.g. this is from one of my menu templates:

      {% if loop.first %}
        <ul
        {%- if menu_level == 0 -%}
          {{ attributes.addClass(['menu', 'odd', 'menu-level-1', menu_name ? 'menu-name--' ~ menu_name|clean_class ]) }}
        {%- else %}
          class="menu is-child {{ cycle(['odd', 'even'], menu_level) }} {{ 'menu-level-' ~ (menu_level + 1) }}"
        {%- endif -%}
        >
      {% endif %}

.. something along those lines etc.

Sutharsan’s picture

@JeffBurnz, I did consider this. This this would be the code:

      <ul
        {%- if menu_level == 0 -%}
          {{ attributes.addClass('menu') }}
        {%- else %}
          class="menu"
        {%- endif -%}
      >

And the HTML result:

              <ul class="clearfix menu">
            <li class="menu-item menu-item--expanded">
      <a href="/" data-drupal-link-system-path="&lt;front&gt;">Home</a>
                            <ul          class="menu">
            <li class="menu-item menu-item--collapsed">
      <a href="/user" data-drupal-link-system-path="user">sub</a>
          </li>
    </ul>

It is not possible to do {%- else -%}, as this would lead to <ulclass="menu">.

Weighing pros and cons, I still choose my patch. But perhaps you can find a better way to achieve the same result.

Sutharsan’s picture

Regarding @vaplas' alternatives:
'The Good' will make the preprocess significantly more complex (bartik_preprocess_menu() is now a one liner) as it must iterate down the menu tree and set attributes. The preprocess also has to be backwards compatible.

As an alterernative to 'The Bad' and 'The Ugly', I would suggest a new Twig function that creates an Attribute object:

    {% if menu_level == 0 %}
      {{ attributes.addClass('menu') }}
    {% else %}
      {% set attributes = attributes().addClass('menu') %}
    {% endif %}
    <ul{{ attributes }}>
vaplas’s picture

new Twig function - a great idea! It seems that it will be useful not only in these menu.twig templates. Unfortunately the pure name 'attribute' is occupied of Twig, but we can use drupal way pefix 'drupal_attribute'.

Examples:

{% set attributes = drupal_attribute() %}
{% set attributes = drupal_attribute().addClass('menu') %}
{% set attributes = drupal_attribute({'class': 'menu'}) %}

And nit refactoring

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
      <ul{{ attributes }}>               {#!!!#}
        {% for item in items %}
            <li{{ item.attributes }}>
                {{ link(item.title, item.url) }}
                {% if item.below %}
                    {% set attributes = drupal_attribute().addClass(['menu']) %}        {#!!!#}
                    {{ menus.menu_links(item.below, attributes, menu_level + 1) }}
                {% endif %}
            </li>
        {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

also we can use two macro for rendering: wrapper (ul), content (li). Maybe it's better for cleaning twig template (at least, it's better than the loop ;)

vaplas’s picture

Regarding 'The Good'. Yeah, it will make some things done qualitatively. But it also adds flexibly manage by sub-menu (programmatically and via the UI). I sometimes need this opportunity for creative menu. But now it ossified piece for dull menu :)

Jeff Burnz’s picture

Guys, the issue is to use loop and also prevent warnings in IDE like PhpStorm etc, this discussion seems to be about lots of other things beside this simple problem.

Sutharsan’s picture

@Jeff Burnz, Yea, thanks for the focus.

Sutharsan’s picture

Moving the </ul> outside of the "for" loop.

The patch is based on #6; #15 is only for a twig function to create an Attribute object for which we don't seem to have consensus. I let others decide on changing the whitespace solution.

Jeff Burnz’s picture

I quite like the spaceless tags and moving the </ul> out of the for loop as per #19.

vaplas’s picture

We can not moving the /ul outside of the "for" loop, because if empty items the result:
</ul>

The idea of #1 - use only check by {% for item in items %} without {% if items %}. Hence all code must be inside loop. But we are very costly to pay for it. And not only because of the extra performance conditions within the loop. The pattern itself is not successful.

It's not look like:

wrapper_start
    template
wrapeer_end

It's look like:

children_wrapper_start
  parent_wrapper_start
  template
  parent_wrapper_end
  template
  parent_wrapper_start
  template
  parent_wrapper_end
children_wrapper_end

It's antipattern. We inject parent code into children. If we want clear template, maybe better use two macro, like this:

{% macro menu(items, attributes, level) %}
{% if items %}
    {# start template #}
    <ul{{ attributes }}>
        {{ menus.menu_items(items, attributes, level) }}
    </ul>
    {# end template #}
{% endif %}
{% endmacro %}


{% macro menu_items(menu_items, menu_attributes, menu_level) %}
    {% for item in menu_items %}
    {# start template #}
        <li{{ item.attributes }}>..
    {# end template #}
    {% endfor %}
{% endmacro %}

But I am satisfied with the fact that we have now. We just need to solve the problem of double-ul and assigning attributes.

#17: I think this issue primarily focus about improving the template of menu. Loop - only one variant (and not the best). My english is very poor, and I'm tired of pointing this out. Hopefully, John Albin and all others correctly sees my attempt to help. Thanks and good luck!

Jeff Burnz’s picture

@vaplas yes I know what you mean, however is this performance issue so great?

If we just want to remove double UL issue this seems trivial, don't use the loop twig variable,

  {% if items %}
    <ul {% spaceless %}
      {% if menu_level == 0 %}
        {{ attributes.addClass('menu') }}
      {% else %}
        class="menu"
      {% endif %}
    {% endspaceless %}>
    {% for item in items %}
     ...etc

So not as pure as #15 but also not that bad either - adding new twig functions is scope creep (a big scope creep), since we'er not trying to fix every problem in one issue - better to open follow up issues :)

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/menu.html.twig
@@ -30,20 +30,20 @@
+      <ul {% spaceless %}
+        {% if menu_level == 0 %}
+          {{ attributes }}
...
+      {% endspaceless %}>

This would actually introduce an extra white space before if there are no attributes so <ul >

How about:
<ul{{ menu_level == 0 ? attributes }}>

Still confused what this issue is trying to accomplish, maybe @Sutharsan you can explain to me...

Sutharsan’s picture

Status: Needs work » Needs review

I discussed the issue and the options with @joelpittet here at DrupalCon. The issue at hand is that we want to get rid of the waring in IDE's like PhpStorm. And we like to fix only this problem, for the other suggestions we can create a follow up issue. Further we want to minimize the footprint of the patch to maximize the change it gets in.

The ideal patch would do:

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    <ul{{ menu_level == 0 ? attributes.addClass('menu') : ' class="menu"' }}>
    {% for item in items %}
    ...

However due to (what we believe is) a bug in Twig, class="menu" gets escaped. The alternative we came up with is:

{% macro menu_links(items, attributes, menu_level) %}
  {% import _self as menus %}
  {% if items %}
    <ul{% if menu_level == 0 %}{{ attributes.addClass('menu') }}{% else %} class="menu"{% endif %}>
    {% for item in items %}
    ...

This can be reworked once the ternary operator bug gets fixed upstream. To see the twig bug in action, try the following in for example twigfiddle.com:

static: {{ '&' }}<br>
true: {{ true ? '&' : '&' }}<br>
false: {{ false ? '&' : '&' }}<br>
true: {{ true ? '&' : name }}<br>
false: {{ false ? name : '&' }}<br>
Sutharsan’s picture

Created a new patch accordingly.

joelpittet’s picture

Assigned: Unassigned » JohnAlbin

I'd say RTBC to #25 for now but this is @JohnAlbin's original issue and I disagree with the issue summary making things more clear and adding conditions in the loop when they are to apply things twice. Also I don't see the bug other than PHPStorm, so if #25 is the wrong direction we can move that to a new issue.

Assigning to the author for review.

Sumit kumar’s picture

Issue tags: -Twig +Dublin2016
lauriii’s picture

Title: Use twig loop variable instead of wrapping "for" with "if" » Make menu.html.twig easier to understand
Status: Needs review » Active

I don't see much benefit for the change made on #25 and would like to say that it shouldn't be committed as is.

I think what we are trying to do here, is to make the menu.html.twig easier to read? If we want to do that, I think we need some fresh ideas how to do it.

JohnAlbin’s picture

Thank you, Lauri, for putting a better title on this issue. Indeed my original description was a technical solution in search of a problem.

I agree the most recent patch doesn't make things easier to understand.

While _I_ find checking the loop variable easier to understand, the {% if menu_level == 0 %} code is making the whole thing overly complicated. I have some ideas on how to remove that.

joelpittet’s picture

@Sutharsan maybe move the patch to a follow-up issue to deal with the PHPStorm/IDE reporting missing closing tags issue?

Sutharsan’s picture

@joelpittet, no problem but I'd rather wait a little to see where this issue is going to.

vaplas’s picture

What's up?)

After #2616756: Allow instantiating Attribute objects within Twig, we can solve problem with double-ul like this:

<ul{{ attributes.addClass(['menu']) }}>
....
{{ menus.menu_links(item.below, create_attribute(), menu_level + 1) }}

But if we need some fresh ideas, how to make the menu.html.twig easier to read.. Gentlemen, what do you think, if we apply the pattern Mediator (or maybe it's called a Facade, Bridge, .. your variant here). This can well help separate the logic from the layout:

{% import 'core/modules/system/templates/menu_render.twig' as render %}
{{ render.menu(_self, items) }}

{% macro menu(render, items, attributes) %}
    <ul{{ attributes }}>
        {{ render.links(_self, items) }}
    </ul>
{% endmacro %}

{% macro link(render, item, attributes) %}
    <li{{ attributes }}>
        {{ link(item.title, item.url) }}
        {{ render.menu(_self, item.below) }}
    </li>
{% endmacro %}

Expanded variant:

 {% import 'core/modules/system/templates/menu_render.twig' as render %}
 {{ render.menu(_self, items, attributes, 0) }}
 
 {% macro menu(render, items, attributes, menu_level) %}
+    {% set classes = ['menu'] %}
+    {% set attributes = attributes.addClass(classes) %}
     <ul{{ attributes }}>
         {{ render.links(_self, items, menu_level) }}
     </ul>
 {% endmacro %}

 {% macro link(render, item, attributes, menu_level) %}
+  {%
+  set classes = [
+  'menu-item',
+  item.is_expanded ? 'menu-item--expanded',
+  item.is_collapsed ? 'menu-item--collapsed',
+  item.in_active_trail ? 'menu-item--active-trail',
+  ]
+  %}
+  {% set attributes = attributes.addClass(classes) %}
   <li{{ attributes }}>
     {{ link(item.title, item.url) }}
     {{ render.menu(_self, item.below, create_attribute(), menu_level + 1) }}
   </li>
 {% endmacro %}

And magic core/modules/system/templates/menu_render.twig:

{% macro menu(template, items, attributes, menu_level) %}
  {% if items %}
    {% if not attributes %}
      {% set attributes = create_attribute() %}
    {% endif %}
    {{ template.menu(_self, items, attributes, menu_level) }}
  {% endif %}
{% endmacro %}

{% macro links(template, items, menu_level) %}
  {% for item in items %}
    {{ template.link(_self, item, item.attributes, menu_level) }}
  {% endfor %}
{% endmacro %}

Edit: correct mistake

- {% set item.attributes = item.attributes.addClass(classes) %}
+ {% set attributes = attributes.addClass(classes) %}

Hence now macro link have new param attributes. Because it item.attributes = ... not work, sorry!

vaplas’s picture

Also, it may need to supplement the documentation of the available variables:

  * Available variables:
+ * - menu_name: machine name of the menu
+ * - attributes: HTML attributes for the menu,
+ *   instance of \Drupal\Core\Template\Attribute
  * - items: A nested list of menu items. Each menu item contains:
  *   - ...
+ *   - original_link: instance of \Drupal\Core\Menu\MenuLinkDefault

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.