Problem/Motivation

We have many similar functions in core that generate lists of links. Even worse, our menu theme functions are also guilty of splitting up the UL tag from the LI tag, something that goes against our principles for D8,

Below is a lit of current functions that need to be reworked / removed / replaced:

Proposed resolution

We'd like everything that produces lists of menu links to call theme_menu (or some variation) instead of writing their own function(s).

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

Remaining tasks

  • Remove theme_menu_tree / theme_menu_link and replace with a call to theme_menu
  • Remove theme_menu_local_tasks/theme_menu_local_task replace with a call to theme_menu__local_tasks
  • Remove theme_menu_local_tasks/theme_menu_local_action replace with a call to theme_menu__local_actions
  • Provide theme hook suggestions for each of the previous that allow a menu-specific override of each
    • theme_menu__[MENU_NAME]
    • theme_menu__[MENU_NAME]__local_tasks
    • theme_menu__[MENU_NAME]__local_actions

User interface changes

None.

API changes

Removal of the following functions:

#1813426: [meta] Consolidate all item list templates and add theme_hook_suggestions
#1834022: Combine theme_menu_tree and theme_menu_link, and rename theme_menu

Files: 
CommentFileSizeAuthor
#25 menu-1777332-25.patch17.06 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,754 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 menu-1777332-23.patch7.24 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,718 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Comments

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.

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.

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.

Issue summary:View changes

update

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.

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

Issue summary:View changes

add menu

more tagging

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

Issue summary:View changes

more sense

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

Updating title.

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

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?

@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.

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.

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.

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. :-)

*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) }}

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

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

I will write up some example code later here.

I will write up some example code later here.

I will write up some example code later here.

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

Issue summary:View changes

update issue summary

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

Status:Active» Needs review
StatusFileSize
new7.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,718 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new17.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,754 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

#2239833: Contextual Links not visible in '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.