menu.inc - Convert theme_ functions to Twig

Taking this on. Going for consolidation/use of other things like theme('item_list'), in the process.

Consolidated various issues* from the Twig sandbox into this one patch.

Not consolidating or using item_list as mentioned in #2, for two reasons: 1. Straight conversion; 2. These don't always have to be formatted as lists.

TODO (once #1905584: Move base theme system templates into /core/templates is fixed):

1. Delete theme_* functions from menu.inc.
2. Move templates from /core/themes/stark/templates/menu.inc/* to /core/includes/templates/menu.inc/*

Git attribution goes to: likin, killerpoke, EVIIILJ, vlad.dancer, podarok, mh86, steveoliver

Previous sandbox issues, where these came from, in part:
#1843564: Convert theme('menu_local_tasks') to Twig
#1843560: Convert theme('menu_local_task') to Twig
#1824618: Convert theme('menu_local_action') to Twig

+++ b/core/themes/stark/templates/menu.inc/menu-local-tasks.html.twig
@@ -0,0 +1,26 @@
+* @ingroup themeable
+*/
+#}
+{% if primary_tasks %}
+  <h2 class="element-invisible">{{ 'Primary tabs'|t }}</h2>
+  <ul class="tabs primary">{{ primary_tasks }}</ul>
+{% endif %}
+{% if secondary_tasks %}
+  <h2 class="element-invisible">{{ 'Secondary tabs'|t }}</h2>
+  <ul class="tabs secondary">{{ secondary_tasks }}</ul>

Please can we follow BEM principles with our class names. Just adding a class of 'primary' or 'secondary' can and will cause conflicts. Please use 'tabs--primary' and 'tabs--secondary' instead.

Here is a brief explanation of BEM for reference.

BEM is a methodology for naming
and classifying CSS selectors in a way to make them a lot more strict,
transparent and informative.

The naming convention follows this pattern:

    .block{}
    .block__element{}
    .block--modifier{}

* `.block` represents the higher level of an abstraction or component.
* `.block__element` represents a descendent of `.block` that helps form `.block` as a whole.
* `.block--modifier` represents a different state or version of `.block`.

An **analogy** of how BEM classes work might be:

    .person{}
    .person--woman{}
        .person__hand{}
        .person__hand--left{}
        .person__hand--right{}

@mbrett5062 - Thanks for the feedback!

We're working hard to get these Twig templates into core, there will definitely be more cleanup afterwards but for now we're focused on getting these templates into core before feature freeze.

There have been recent discussions and work towards CSS coding standards:

g.d.o announcement and discussion:
http://groups.drupal.org/node/277223

Draft coding standards:
http://drupal.org/node/1886770 and more specifically http://drupal.org/node/1887918 which relates to architecture.

Also #1900768: Consider BEM for CSS Coding Standards in Drupal 8.

Once the standards are worked out, the task of refactoring CSS to meet these standards would probably be best done with a meta issue and sub issues, similar to #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core. I wasn't able to find an existing meta issue tracking this.

Thanks for the feedback @cottser, and I agree with doing these changes in a follow up. Just glad it is noted.

Adding git attribution to issue summary

Just a head's up: #916388: Convert menu links into entities is a few days from landing. We'll probably want to wait till then before looking at this again.

Based on the state of #1905584: Move base theme system templates into /core/templates which introduces support for /core/templates for these includes, this issue is active and I'm on it...

#3: drupal-twig-menu.inc--1898478-3.patch queued for re-testing.

We need to update drupal_common_theme() in theme.inc to add 'template' indexes for all these new templates.

Working on a new patch right now to move this along a bit.

Summary of changes:

1. Moved templates from stark/templates to modules/system/templates
2. Removed theme functions
3. Updated documentation to match #1913208: [policy] Standardize template preprocess function documentation and http://drupal.org/node/1823416

The interdiff is rolled with git diff -C -M to account for the file moves.

All the failing tests pass locally so hopefully testbot agrees :)

Wrong interdiff, here's the smaller one.

+++ b/core/includes/menu.incundefined
@@ -1555,106 +1555,92 @@ function _menu_tree_data(&$links, $parents, $depth) {
-    'localized_options' => array(),
+    'localized_options' => new Attribute(),

Uh, oops. That doesn't belong there.

This simplifies template_preprocess_menu_link() (removes drupal_render() and ternary) and updates one of the preprocess docblocks to match #1913208: [policy] Standardize template preprocess function documentation.

Missed "Default template". Interdiff is from #19.

One more, sorry for the barrage. Just another docs tweak, interdiff is from #19 again.

Making commit mentions from the sandbox more prominent ala http://drupal.org/node/1535868

Add conversion summary table

Tagging.

Update remaining

Unassigning so this can be reviewed.

Review for #22:

+  $variables['primary_tasks'] = (!empty($variables['primary'])) ? drupal_render($variables['primary']) : FALSE;
+  $variables['secondary_tasks'] = (!empty($variables['secondary'])) ? drupal_render($variables['secondary']) : FALSE;

Do we need to render these in the preprocess? they should just work as-is in the template, right? In fact, do we even need the preprocess at all here? looks like we could just use {{ primary }} and {{ secondary }} in the template.

We have a new template for menu_local_tasks but the theme function wasn't removed in the patch.

/**
 * Markup generated by theme_menu_local_tasks().
 */
.tabs > li {
  margin-left: 0.3em;
  margin-right: 0;
}

This still exists in system.theme-rtl.css

/**
 * Markup generated by theme_menu_local_tasks().
 */
div.tabs {
  margin: 1em 0;
}

This still exists in system.theme.css

Cleaned up some docs and removed the preprocessing and theme for menu_local_tasks from #25

bartik_menu_tree() needs to be converted here too. If the wrapper in menu_tree.html.twig is changed to use an Attribute(), then bartik can just implement a preprocess, since all it's doing is adding a class.

Did #27, thanks @duellj I went with the twig file as discussed on IRC to help with #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme

remove sandbox link

Menu's, submenu's, breadcrumbs and action links are ok, they have the same html output in both Stark and Bartik.

Tabs need a little more work.

The output for the tabs isn't the same (in both Stark and Bartik). On the active tab, <span class="element-invisible">(active tab)</span> should be a child of the link, but in the twig version, it is a sibling.

#28: 1898478-28-twig-menu.patch queued for re-testing.

Working on re-roll.

Here's a straight re-roll. Looking at the issue raised in #31.

Run through testbot.

I've fixed the accessibility issue raised in #31. After some discussions here at DrupalCon, we've decided to add the span tag markup directly in template_preprocess_menu_local_task(). This is not ideal. I've added a @TODO comment noting that this should be changed once the l() function is updated.

Sorry. Uploaded the wrong patch file. The previous interdiff was correct though.

Marking to needs work so we can then re-queue #38 for testing. It failed when stuff went down while the sprint was going on last Friday.

... and needs review.

I am attaching the patch from #38 here so it can be tested by the bot. I couldn't get it to be re-queued with my attempts above.

Update conversion table

I think everything passed the manual testing steps outlined at the top of this issue, although I'd like confirmation that the output for step 4 (change your theme back to Bartik to see the output of bartik_menu_tree) is correct. The comment for what showed up in the source was:

<!-- CALL: theme('menu_tree__admin') -->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/menu-tree.html.twig' -->

Is there any need to use Daisy Diff on the output, or is it enough that the items listed in the testing steps were all present?

If not, and assuming the bartik_menu_tree was handled correctly, then this passes manual testing.

CSS directories moved around, #41 needs re-roll.

Also, all wrapper_attributes should be just attributes and can be lazy loaded for performance ala #1982024: Lazy-load Attribute objects later in the rendering process only if needed

Dipping my toes in the pool with a reroll. I hope I did this right.

Reroll looks great, thanks @mike.roberts! Tagging for eventual profiling.

Thanks @mike.roberts! Give a go at the wrapper_attributes bit from #43

It's a the following:

1. Rename all wrapper_attributes in this patch to attributes
2. Remove Attribute() instantiation around values passed to attributes

That should make things a bit quicker

Rerolled patch from #44 and renamed wrapper_attributes to attributes

@duellj close, missed one;) Thank you for the patch.

I just patched that one, hopefully this will pass.

I created both block menus and regular menus and everything looks good. Changing to reviewed and tested by community.

Re-tagging for profiling :) thanks for reviewing @azinoman. To be clear - when you tested did you compare the markup before and after the patch? If so we can remove the 'Needs manual testing' tag, otherwise this needs a round of manual testing.

Needs another reroll.

Re-rolled and remove the @see template_preprocess() note in the twig files.

Thanks @joelpittet! Tagging again for reroll.

Rerolled

Tagging for reroll. Thanks again @joelpittet :)

Rerolled patch and removed testbot errors.

Thanks to @lduerig for the collaboration ;)

Related: #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template

#61: 1898478-61-twig-theme-menu.patch queued for re-testing.

Trying again to reroll.

+++ b/core/includes/menu.inc
@@ -1665,100 +1665,85 @@ function _menu_tree_data(&$links, $parents, $depth) {
+  $variables['link'] = l($link['content'], $link['href'], $link['localized_options']);

It seems that $link['href'] makes problems here. In a few cases there's no 'href' key which breaks the link ("Undefined index: href") and ruins the test run. Any ideas anyone?

Great work so far! I've discussed the menu logics with jenlampton, mortendk and jwilson3 at the core sprint yesterday and we propose a major overhaul of the current menu setup.

The way menu's are outputted now is odd: we use theme_menu_tree for the menu's in the sidebar (with hierarchy) and we misuse theme_menu_links for the same menu's when we output them as Primary Menu or Secondary Menu. Because of this, the menu cannot have drop-downs and the checkbox in the backend (Show expanded menu) does not work at all for these two menu's.

So the main proposal here is to add a template_proprocess_menu and use that one for all menu's. Remove ALL theme_menu functions and introduce a menu.html.twig file. More info is here: #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template.

This would introduces some other smaller tasks (like styling dropdowns in Bartik, but that will be taken care of later).

@BarisW that sounds way better! #70

We've already put a lot of work in here, so until #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template is underway I suggest we continue to move forward with this issue. Troubleshooting the failing tests would be the next step, I looked at this briefly with @organicwire and @lduerig in Prague. Seemed like a tricky merge to me but if that merge can be resolved in the right way we should be able to make this green again.

Re-rolled again based on #55 which was the last one to be green by testbot.

Ran the failing tests locally and they passed that failed on #67.

Would like to build out the menu_local_task link in the template because the span tag for active tab for visually impaired is hard coded in the preprocess.

added how to test

+++ b/core/themes/bartik/templates/menu-tree.html.twig
@@ -0,0 +1,19 @@
+<ul class="menu clearfix">
+  {{ tree }}
+</ul>

Many contrib modules need to change the classes on the ul wrapper of menu-tree.html.twig. Does it make sense to provide these as variables so that they're more overrideable?

Also note that the current theme function does not have a clearfix class:

/**
 * Returns HTML for a wrapper for a menu sub-tree.
 *
 * @param $variables
 *   An associative array containing:
 *   - tree: An HTML string containing the tree's items.
 *
 * @see template_preprocess_menu_tree()
 * @ingroup themeable
 */
function theme_menu_tree($variables) {
  return '<ul class="menu">' . $variables['tree'] . '</ul>';
}

73: 1898478-73-twig-menu.patch queued for re-testing.

@jessebeach - Thanks! I think that in most cases it will probably make more sense to do <ul{{ attributes }}>…</ul> rather than hardcoding classes into the core templates.

But what jessebeach really needs for toolbar is the ability to change aka remove menu and clearfix classes for the toolbar menu_tree's only. Above code is in general correct as bartic overrides the classes from core.

After applying patch #73 to the latest dev version of d8 my toolbar menu won't expand anymore. I tried it in both Bartik and Stark.

@mariacha1 thanks for the manual test screenshots. I just gave it a go and didn't get that result (Latest FF and Chrome for mac) but I did have some odd git thing saying one file couldn't get changed so I'm reposting another re-roll just in case that helps. Just to cover all bases, could you apply this and delete /sites/default/files/php folder after applying it, just in case somethings cached in twig. It's extreme but just to make sure.

It's definitely still happening for me. Looking at the code produced, it looks like the ul inside div class=toolbar-menu-administration is empty. But I'm seeing other issues, like inability to write config files, on my end, so there might be something wrong with my system setup as well. If it turns out to be a false alarm, I'll report back. (I'm on Linux.)

The patch in 80 is working fine for me. No trouble with the Toolbar Menu.

mariacha1, try a clean install in a new directory. You may have residual config files causing you trouble.

@mariacha1 sounds like you have your /sites/default/files/ directory not writable for the web server. For dev you can be lazy and just chmod 777 or you can add change ownership/group to the apache user.

There is also a couple twig settings in settings.php for debugging and not caching the templates. All compiled templates end up in /sites/default/files/php along with service containers and other cache files that can be deleted at will.

Thanks for all the advice everyone. I fiddled around with some things on my end and I imagine it has to do with my having the default twig settings instead of any of the custom ones.

In case anyone else has the issue, it resolved itself when I changed the default theme to a new one. I don't think it's worth worrying about.

I tested several pages with patch 80 and they all looked good to me. My only comment is that it introduces one formatting error by adding this line 1705 of core/includes/menu.inc

+ * Default template: menu-local-task.html.twig.
+ *
+ * @param array variables

That @param array variables should be @param array $variables.

Nice catch, thanks for reviewing @mariacha1

Added the $ in comments as indicated in #86

forgot interdiff...

The issue summary states that bartik_menu_tree will be handled in another issue, but the issue that is pointed to is closed and the bartik_menu_tree doesn't seem to be tackled there? Also - and probably related - why is there so much difference between the menu-tree.html.twig in the menu module compared to the same file in the bartik/templates folder?

I assume we would have the same variables available there?

Thanks for all the hard work so far. Let's get this in soon, so we can start cleaning up (and please let's stop using preprocess_links for menu's!).

#2004826: bartik.theme - Convert theme_ functions to Twig this is where it was closed for this one... needs an issue summary update.

@BarisW, which differences are you refering to?

Here removes the two three functions from bartik.

Tagging for reroll.

Here is a reroll.

In template_preprocess_menu_link() there was one more line in the 8.x branch:

$element['#localized_options']['set_active_class'] = TRUE;

Eventually I kept it in the reroll. This was the only notable change.

Lookin' good

This patch still needs profiling, thanks for reviewing @maggo.

The patch wasn't applying so here is another reroll. I've also profiled this patch with 'admin/structure/menu/manage/admin' as the frontpage.

=== 8.x..8.x compared (52e4c97c57ee5..52e4ca3e2b642):

ct  : 285,843|285,843|0|0.0%
wt  : 1,373,308|1,363,312|-9,996|-0.7%
cpu : 1,331,080|1,325,707|-5,373|-0.4%
mu  : 18,223,128|18,223,128|0|0.0%
pmu : 20,656,208|20,656,208|0|0.0%


=== 8.x..menu-twig compared (52e4c97c57ee5..52e4caf158913):

ct  : 285,843|286,546|703|0.2%
wt  : 1,373,308|1,374,417|1,109|0.1%
cpu : 1,331,080|1,332,247|1,167|0.1%
mu  : 18,223,128|18,302,048|78,920|0.4%
pmu : 20,656,208|20,797,152|140,944|0.7%

Tagging for reroll.

Working on re-roll.

Here is the re-rolled patch against the latest head.

Rerolled from #99.

Tagging for reroll.

Re-rolled from #104

Patch applies nicely with a little bit of offset.
It was RTBC before at #98, but needed profiling. That's done, and re-rolled. Code looks good to me!

theme-menu-inc-1898478-107.patch no longer applies.

error: patch failed: core/includes/menu.inc:11
error: core/includes/menu.inc: patch does not apply

Thanks @alexpott! This still applied with `patch -p1` so here's a quick reroll.

Also updating the proposed commit message because it was very out of date.

Ah sorry, I didn't know that. I used -p1 as well.

Needs a re-roll.

Re-rolled.

Re-rolled from #113.

116: 1898478-116.patch queued for re-testing.

Tagging for reroll.

Updating the patch with reroll, Please review.

Some files were skipped while taking the patch. Updating the patch again with all reroll changes .

Just changing the file name of the patch. (earlier there is a space between convert and theme words)

1. +++ b/core/modules/system/templates/menu-link.html.twig
   @@ -0,0 +1,21 @@
   + * Note: This template renders the content for each individual menu item in
   + * menu-local-tasks.html.twig.
   

   I assume this is meant to say menu-tree.html.twig

2. +++ b/core/modules/system/templates/menu-local-task.html.twig
   @@ -0,0 +1,22 @@
   + * Note: This template renders the content for each individual task item in
   + * menu-local-tasks.html.twig.
   

   "each" and "individual" are pretty much the same thing, just choose one.

3. +++ b/core/modules/system/templates/menu-local-tasks.html.twig
   @@ -0,0 +1,25 @@
   + * Note: Each item for both sets of tasks can be individually themed in
   + * menu-local-task.html.twig.
   

   Again, and this time it sounds quite ambiguous as what I actually can or cannot do, e.g. from this I would expect a loop in menu-local-tasks.html.twig where I could "theme individual items".

4. +++ b/core/themes/bartik/templates/menu-tree--shortcut-default.html.twig
   @@ -0,0 +1,19 @@
   + * Bartik's theme implementation for the wrapper of a shortcut menu tree.
   

   Is this even required?

I realise this is a strait conversion issue however it's still baffling why we have all these templates (as it was when we had all functions etc), and why we get a gnarly great string in menu-tree.html.twg and not a renderable array of items (I know why, but it's a bit mad how this all works), to my mind there should only be one template (menu.html.twig with a loop for items) and local tasks etc are suggestions or something like that. 2cents.

Thanks @Jalandhar and @Jeff Burnz. Consolidation comes after conversion unless things are really slow - it's significantly easier to consolidate things once they are already template-ified and even if consolidation/improvement of menu items doesn't happen by 8.0 it at least means we've improved things!

I think we do need the Bartik overrides if markup is changing. I did notice that several templates are now missing newlines at the end of the file so that needs to be fixed and if it makes automated tests fail we might need to adjust those.

Weird.

This issue appears to conflict with the plan in #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template ...?

Not sure if it's really a conflict. This issue converts, that issue consolidates. This patch is also very near RTBC and there is no patch on the other issue.

Thanks Cottser and sun, indeed that issue mentioned by sun is what I would expect to happen next, fantastic.

This patch addresses the comments in #125, including removing core/themes/bartik/templates/menu-tree--shortcut-default.html.twig because I didn't see why it was needed. There are no other matching file names in drupal core, and its markup is identical to menu-tree.html.twig. Newlines at ends of files have been added too.

+++ b/core/includes/menu.inc
@@ -349,19 +337,15 @@ function theme_menu_local_action($variables) {
+    $variables['link'] = Drupal::l($link['title'], $link['route_name'], $link['route_parameters'], $link['localized_options']);
...
+    $variables['link'] = l($link['title'], $link['href'], $link['localized_options']);

Why not Drupal::l() on the second link?

We have different data, so we use different functions.
One takes title, href, options. The other takes title, route name, route params, options.

Lets avoid generating html in preprocess functions

134: drupal8-menu_convert_theme_functions_to_twig-1898478-134.patch queued for re-testing.

@m1r1k That looks like a good patch. I'm not quite sure why it fails but may have something to do with the theme=>link vs l() API differences. Of which I wish I knew by heart...

Would you mind debugging locally some of those test fails?

Something happened with local-actions content. And previous patch is also broken.
I will look at it right away.

Silly typos

142: drupal8-menu_convert_theme_functions_to_twig-1898478-142.patch queued for re-testing.

theme_menu_link is being changed to use a Url object in #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

Can that be supported in twig?

142: drupal8-menu_convert_theme_functions_to_twig-1898478-142.patch queued for re-testing.

EDIT: Queued again because running the test locally showed no errors.

Tagging for reroll.

Unassigning since it's been a few weeks, @m1r1k if you still want to tackle this just reassign. Thanks!

@pwolanin at a glance it looks like generating the URL would happen in preprocess rather than in Twig.

merging core/themes/seven/seven.theme was a little complex.

Trying again.

Thanks @lokapujya! I diffed the two patches, the only thing that jumped out at me as far as merge conflicts was:

+++ b/core/themes/seven/seven.theme
@@ -37,77 +37,36 @@ function seven_preprocess_page(&$variables) {
 /**
  * Overrides theme_menu_local_task().
  *
- * Returns HTML for a local task.
+ * Implements hook_preprocess_HOOK() for menu-local-task templates.
  */
-function seven_menu_local_task($variables) {

This should not say "Overrides theme_menu_local_task()" because we're removing that function, just have this docblock says "Implements hook_preprocess_HOOK()…".

Fixed issue mentioned in the last commented and manually tested that the each of the twig files mentioned in the issue summary are being used.

Thank you for fixing that @lokapujya! Unfortunately this needs another reroll now, any takers?

Updating "To test this code" instructions since admin/content/node returns a 404.

Manual review of #155:

• Visually identical. (Attached as instructed at https://www.drupal.org/node/2048869).
• Resulting HTML has a slightly different formatting (adds new lines) coming from menu-link.html.twig. Not sure how much people care about that?
• Action link classes are in a different order. (I don't know if that's important.)
• Action link uses "data-drupal-link-system-path" instead of "href", I'm assuming this is intentional but don't know enough about the RDF changes.

Switching back to Needs Work so somebody more knowledgeable can decide if the differences are negligible enough to be marked RTBC.

Please postpone any change affecting menu links until this is committed: #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

Thanks @pwolanin! I just wanted to mention that some new and useful looking templates have been introduced via #2301317: MenuLinkNG part4: Conversion, good stuff!

Namely menu-tree.html.twig.

might need to use the functionality from #2073811: Add a url generator twig extension

I really wonder whether we could / should rather push #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template given that it actually improves the experience for the themer.

@dawehner yes in general I would agree but that issue doesn't cover all the theme output from menu.inc I don't think. Thinking of local tasks, local actions, etc.

From the Twig conversion perspective we can at least remove anything touching menu_tree from the patch in this issue and I would be okay with leaving menu_link changes to #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template. The other issue needs an issue summary update I think because it's not really doing what it says in the title/summary (unless it plans to?).

@dawehner I agree I love what has started there.

@Cottser if that other one can't cover the local tasks/actions too then we can keep doing that bit here if needed?

Yes that's what I was thinking. Divide and conquer!

Here's a rough reroll that also removes the conversions of menu_tree and menu_link to defer to the awesomeness of #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template.

Also, a lot of people have worked on this issue. Wow.

@Cottser thanks for the re-roll.

Here are a few little items that a Novice contributor can pickup, there is likely more like this, so whomever picks this up, keep a diligent eye out for coding standards or missed pieces from before the conversion. There are likely a few pieces that snuck in between the previous patch and the latest re-roll to catch up to head.

1. +++ b/core/includes/menu.inc
   @@ -403,29 +403,36 @@ function theme_menu_local_task($variables) {
   +      '#type' => 'link',
   +      '#title' => $link_text,
   +      '#options' => $link['localized_options'],
   

   2 spaces.

2. +++ b/core/includes/theme.inc
   @@ -2398,12 +2398,15 @@ function drupal_common_theme() {
   +        'template' => 'menu-local-action',
   

   extra spaces before.

3. +++ b/core/themes/seven/seven.theme
   @@ -164,22 +121,11 @@ function seven_preprocess_tablesort_indicator(&$variables) {
   -  $link['localized_options']['attributes']['class'][] = 'button';
   -  $link['localized_options']['attributes']['class'][] = 'button--primary';
   -  $link['localized_options']['attributes']['class'][] = 'button--small';
   ...
   +  $variables['link']['#options']['attributes']['class'][] = 'button--primary';
   +  $variables['link']['#options']['attributes']['class'][] = 'button--small';
   

   Missing button class.