Issue #1898478 by joelpittet, Cottser, m1r1k, lokapujya, Jalandhar, organicwire, chakrapani, duellj, jessebeach, LinL, jstoller, derheap, Risse, mike.roberts, tlattimore, galooph, nadavoid, steveoliver, idflood, likin, killerpoke, EVIIILJ, vlad.dancer, podarok, mh86 | c4rl: menu.inc - Convert theme_ functions to Twig.

Task

Use Twig instead of PHPTemplate

Remaining

  • Patch needs review

Theme function name/template path Conversion status
theme_menu_link Will be handled via #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template
theme_menu_local_action converted
theme_menu_local_task converted
theme_menu_local_tasks converted
theme_menu_tree Will be handled via #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template

To test this code

- set the stark theme to default
- make sure a menu block is placed in a sidebar. ("Tools" or "Main menu" wil do.)
- create one node, and view it's page
- the output of theme_menu_link, theme_menu_local_task, theme_menu_local_tasks, and theme_menu_tree will be visible on this node page output.
- change your theme back to Bartik to see the output of bartik_menu_tree
- visit admin/content to see the output for theme_menu_local_action

Files: 
CommentFileSizeAuthor
#178 interdiff.txt4.14 KBjoelpittet
#178 menu_inc_convert-1898478-178.patch16.18 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,931 pass(es). View
#175 interdiff-1898478-175.txt802 byteslokapujya
#175 1898478-175.patch15.48 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,817 pass(es). View
#170 1898478-170.patch15.66 KBer.pushpinderrana
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,532 pass(es), 16,225 fail(s), and 1,048 exception(s). View
#169 interdiff-1898478-166-169.txt1.65 KBer.pushpinderrana
#166 1898478-166.patch15.6 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,597 pass(es), 16,167 fail(s), and 1,041 exception(s). View
#157 screenshots.zip265.63 KBakalata
#155 1898478-155.patch20.96 KBLinL
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,767 pass(es). View
#153 interdiff.txt444 byteslokapujya
#153 1898478-153.patch21 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,505 pass(es). View
#79 unselected.png27.58 KBmariacha1
#79 selected.png23.74 KBmariacha1

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

steveoliver’s picture

Assigned: Unassigned » steveoliver

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

steveoliver’s picture

Component: other » theme system
Status: Active » Needs review
FileSize
7.79 KB
FAILED: [[SimpleTest]]: [MySQL] 53,833 pass(es), 45 fail(s), and 56 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-twig-menu.inc--1898478-3.patch, failed testing.

mbrett5062’s picture

+++ 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{}
Cottser’s picture

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

mbrett5062’s picture

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

Cottser’s picture

Issue summary: View changes

Adding git attribution to issue summary

steveoliver’s picture

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.

c4rl’s picture

Based on #1905584: Move base theme system templates into /core/templates it seems this issue would be accommodated by #1898454: system.module - Convert PHPTemplate templates to Twig, though that may result in a large patch.

steveoliver’s picture

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

c4rl’s picture

Ignore what I said about system.module in #9, we'll proceed to convert this separately of system.module and put in /core/templates as described in #1905584: Move base theme system templates into /core/templates

Cottser’s picture

Status: Needs work » Needs review
Issue tags: -Twig

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, drupal-twig-menu.inc--1898478-3.patch, failed testing.

Cottser’s picture

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

Cottser’s picture

Assigned: steveoliver » Cottser

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
15.11 KB
10.72 KB
FAILED: [[SimpleTest]]: [MySQL] 53,595 pass(es), 181 fail(s), and 707 exception(s). View

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

Cottser’s picture

FileSize
10.96 KB

Wrong interdiff, here's the smaller one.

Status: Needs review » Needs work

The last submitted patch, 1898478-16.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 54,077 pass(es). View
543 bytes
+++ 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.

Cottser’s picture

FileSize
1.32 KB
10.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

Cottser’s picture

FileSize
1.37 KB
10.66 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

Cottser’s picture

FileSize
1.99 KB
10.65 KB
PASSED: [[SimpleTest]]: [MySQL] 54,117 pass(es). View

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

Cottser’s picture

Issue summary: View changes

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

Cottser’s picture

Issue summary: View changes

Add conversion summary table

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

Cottser’s picture

Issue summary: View changes

Update remaining

Cottser’s picture

Assigned: Cottser » Unassigned

Unassigning so this can be reviewed.

thedavidmeister’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.65 KB
PASSED: [[SimpleTest]]: [MySQL] 54,707 pass(es). View
5.81 KB

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

duellj’s picture

Status: Needs review » Needs work

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.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
FileSize
12.83 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
1.35 KB

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

joelpittet’s picture

Issue summary: View changes

remove sandbox link

c4rl’s picture

Title: Convert menu.inc to Twig » menu.inc - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

joelpittet’s picture

Assigned: joelpittet » Unassigned
intergalactic overlords’s picture

Status: Needs review » Needs work

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.

hanpersand’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -Twig

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

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Twig

The last submitted patch, 1898478-28-twig-menu.patch, failed testing.

jstoller’s picture

Assigned: Unassigned » jstoller

Working on re-roll.

jstoller’s picture

FileSize
13.53 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

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

jstoller’s picture

Status: Needs work » Needs review

Run through testbot.

jstoller’s picture

Assigned: jstoller » Unassigned
FileSize
1.68 KB
12.83 KB
PASSED: [[SimpleTest]]: [MySQL] 55,909 pass(es). View

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.

jstoller’s picture

FileSize
12.92 KB
None View

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

tlattimore’s picture

Status: Needs review » Needs work

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.

tlattimore’s picture

Status: Needs work » Needs review

... and needs review.

tlattimore’s picture

FileSize
12.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,796 pass(es). View

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.

tlattimore’s picture

Issue summary: View changes

Update conversion table

ericaack’s picture

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.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

mike.roberts’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,322 pass(es). View

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

Cottser’s picture

Issue tags: +Needs profiling

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

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: -Needs 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

duellj’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
13.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,898 pass(es), 10 fail(s), and 14,388 exception(s). View

Rerolled patch from #44 and renamed wrapper_attributes to attributes

Status: Needs review » Needs work

The last submitted patch, 1898478-46-menu-twig.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
949 bytes
13.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,782 pass(es). View

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

I just patched that one, hopefully this will pass.

azinoman’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs profiling

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs another reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,877 pass(es). View
3.04 KB

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

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Thanks @joelpittet! Tagging again for reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,533 pass(es). View

Rerolled

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll. Thanks again @joelpittet :)

Cottser’s picture

Status: Needs review » Needs work
derheap’s picture

Assigned: Unassigned » derheap
derheap’s picture

Assigned: derheap » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,290 pass(es), 162 fail(s), and 29,102 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1898478-59-twig-theme-menu.patch, failed testing.

organicwire’s picture

FileSize
7.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Rerolled patch and removed testbot errors.

Thanks to @lduerig for the collaboration ;)

organicwire’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1898478-61-twig-theme-menu.patch, failed testing.

BarisW’s picture

organicwire’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -Needs profiling, -Twig

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

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Needs profiling, +Twig

The last submitted patch, 1898478-61-twig-theme-menu.patch, failed testing.

organicwire’s picture

Status: Needs work » Needs review
FileSize
11.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,021 pass(es), 25 fail(s), and 126 exception(s). View

Trying again to reroll.

Status: Needs review » Needs work

The last submitted patch, 1898478-67-twig-theme-menu.patch, failed testing.

organicwire’s picture

+++ 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?

BarisW’s picture

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

joelpittet’s picture

@BarisW that sounds way better! #70

Cottser’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es). View

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.

joelpittet’s picture

Issue summary: View changes

added how to test

jessebeach’s picture

+++ 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?

jessebeach’s picture

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>';
}
joelpittet’s picture

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

Cottser’s picture

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

hass’s picture

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.

mariacha1’s picture

Issue summary: View changes
FileSize
23.74 KB
27.58 KB

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.

Unselected menu

Selected menu

joelpittet’s picture

FileSize
11.98 KB
FAILED: [[SimpleTest]]: [MySQL] 53,012 pass(es), 3,894 fail(s), and 982 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 80: 1898478-80-twig-menu.patch, failed testing.

The last submitted patch, 80: 1898478-80-twig-menu.patch, failed testing.

mariacha1’s picture

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

jessebeach’s picture

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.

joelpittet’s picture

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

mariacha1’s picture

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.

joelpittet’s picture

Nice catch, thanks for reviewing @mariacha1

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
FileSize
11.98 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es). View

Added the $ in comments as indicated in #86

joelpittet’s picture

FileSize
638 bytes

forgot interdiff...

BarisW’s picture

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

joelpittet’s picture

#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?

joelpittet’s picture

FileSize
1.52 KB
13.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,338 pass(es). View

Here removes the two three functions from bartik.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

idflood’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.42 KB
PASSED: [[SimpleTest]]: [MySQL] 63,129 pass(es). View

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.

maggo’s picture

Assigned: Unassigned » maggo
Issue tags: +SprintWeekend2014, +D8MA
maggo’s picture

Status: Needs review » Reviewed & tested by the community

Lookin' good

maggo’s picture

Assigned: maggo » Unassigned
Cottser’s picture

Status: Reviewed & tested by the community » Needs work

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

idflood’s picture

Status: Needs work » Needs review
Issue tags: -Needs profiling
FileSize
13.4 KB
PASSED: [[SimpleTest]]: [MySQL] 63,147 pass(es). View

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%
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

chakrapani’s picture

Assigned: Unassigned » chakrapani

Working on re-roll.

chakrapani’s picture

Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898478-twig-102-menu.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 102: 1898478-twig-102-menu.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
485 bytes
13.43 KB
PASSED: [[SimpleTest]]: [MySQL] 63,412 pass(es). View

Rerolled from #99.

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

chakrapani’s picture

Status: Needs work » Needs review
FileSize
13.43 KB
PASSED: [[SimpleTest]]: [MySQL] 64,437 pass(es). View

Re-rolled from #104

BarisW’s picture

Status: Needs review » Reviewed & tested by the community

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!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

Cottser’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,727 pass(es). View

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.

BarisW’s picture

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

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a re-roll.

Risse’s picture

FileSize
13.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,383 pass(es). View

Re-rolled.

Risse’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
galooph’s picture

Assigned: Unassigned » galooph
galooph’s picture

FileSize
13.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1898478-116.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled from #113.

galooph’s picture

Assigned: galooph » Unassigned
jygastaud’s picture

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

Status: Needs review » Needs work

The last submitted patch, 116: 1898478-116.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
8.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 42,480 pass(es), 17,714 fail(s), and 1,040 exception(s). View

Updating the patch with reroll, Please review.

Jalandhar’s picture

FileSize
13.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,616 pass(es). View

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

Jalandhar’s picture

FileSize
13.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,627 pass(es). View

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

Jeff Burnz’s picture

  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?

Jeff Burnz’s picture

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

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.

sun’s picture

Cottser’s picture

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.

Jeff Burnz’s picture

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

nadavoid’s picture

Status: Needs work » Needs review
FileSize
12.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,689 pass(es). View

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.

hass’s picture

+++ 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?

tim.plunkett’s picture

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

m1r1k’s picture

FileSize
721 bytes
13.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,326 pass(es), 533 fail(s), and 130 exception(s). View

Lets avoid generating html in preprocess functions

Status: Needs review » Needs work
joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
joelpittet’s picture

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

m1r1k’s picture

Assigned: Unassigned » m1r1k

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

m1r1k’s picture

Status: Needs work » Needs review
FileSize
21.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,236 pass(es), 256 fail(s), and 17 exception(s). View
9.58 KB

Status: Needs review » Needs work
m1r1k’s picture

Status: Needs work » Needs review
FileSize
638 bytes
21.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,589 pass(es). View

Silly typos

Status: Needs review » Needs work
m1r1k’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
pwolanin’s picture

nadavoid’s picture

Status: Needs work » Needs review

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.

Cottser’s picture

Assigned: m1r1k » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

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.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
21.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 149: 1898478-149.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,481 pass(es). View

Trying again.

Cottser’s picture

Status: Needs review » Needs work

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()…".

lokapujya’s picture

Status: Needs work » Needs review
FileSize
21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,505 pass(es). View
444 bytes

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

LinL’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,767 pass(es). View
akalata’s picture

Issue summary: View changes

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

akalata’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
265.63 KB

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.

pwolanin’s picture

Cottser’s picture

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

Cottser’s picture

Namely menu-tree.html.twig.

pwolanin’s picture

Status: Postponed » Needs work

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

dawehner’s picture

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.

Cottser’s picture

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

joelpittet’s picture

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

Cottser’s picture

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

Cottser’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,597 pass(es), 16,167 fail(s), and 1,041 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 166: 1898478-166.patch, failed testing.

joelpittet’s picture

Issue tags: +Novice

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

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
11.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 47,292 pass(es), 18,797 fail(s), and 933 exception(s). View

Incorporated #168 fixes suggested by @joelpittet, please review updated patch.

er.pushpinderrana’s picture

FileSize
15.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 49,532 pass(es), 16,225 fail(s), and 1,048 exception(s). View

New added files are missing in previous patch, this one is updated. Previous interdiff file is fine.

The last submitted patch, 169: 1898478-169.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 170: 1898478-170.patch, failed testing.

lokapujya’s picture

why failing? is this now dependent on another patch?

also,

+++ b/core/includes/menu.inc
@@ -436,19 +443,24 @@ function theme_menu_local_action($variables) {
+      '#type' => 'link',
+      '#title' => $link['title'],
+      '#options' => $link['localized_options'],

2 spaces

Cottser’s picture

This shouldn't depend on anything else but there may have been interdependencies on the menu_link conversion in previous patches. I didn't look too hard when I rerolled, just did it quickly.

+++ b/core/includes/menu.inc
@@ -436,19 +443,24 @@ function theme_menu_local_action($variables) {
+  $variables['link'] = array(
+      '#type' => 'link',
+      '#title' => $link['title'],
+      '#options' => $link['localized_options'],
+  );

With a bit more context this time :) otherwise it's hard to tell what you two are going on about. The contents of this array need to be indented by 2 fewer spaces.

(edited to fix typo)

lokapujya’s picture

Status: Needs work » Needs review
FileSize
15.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,817 pass(es). View
802 bytes
dawehner’s picture

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

Are we sure we don't want to use a template override for adding those classes? Note: we can call parent templates easily and just actually do the add class calls!

Cottser’s picture

Yeah those can be added much cleaner now with addClass. Those lines are very old :)

joelpittet’s picture

FileSize
16.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,931 pass(es). View
4.14 KB

Re #176 and #177 I'd love to remove those to the template but they are on the link element. And there is no NestedArray::mergeDeepArray in Twig (thankfully).

Which means this approach I took isn't viable:

{{ dump(link) }}
{%
  set link = link|merge({
    '#options': {
      'attributes': {
        'class': link['#options'].attributes.class|default([])|merge(['button--primary', 'button--small'])
      }
    }
  })
%}
{{ dump(link) }}
<li{{ attributes }}>{{ link }}</li>

That almost works just missing the 'set_active_class' => true

And second question, that preprocess is attaching modernizr... I'd love to remove that preprocess, can we safely move that somewhere else? I'm thinking seven.info.yml but need a hand on that?

I made some minor tweaks here:

  1. Removed classes that were already added for button in seven
  2. Shuffled around some @todos
  3. Added attributes to the local actions template so it will be easier to consolidate maybe
  4. Removed spaceless (let's see what testbot thinks).
  5. Removed the typecasting for array on the preprocess because we aren't doing that with any other preprocess functions thus far.

Anyways, I'd rather not be too concerned about that, this patch looks RTBC. Let me know if you have any concerns with my changes. I'd like to get these 3 conversions in so we can close #1757550: [Meta] Convert core theme functions to Twig templates!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

It is my honor to RTBC this! Nice work! The remainder and using more suggestions can be follow-up. But this keeps us in releasable state :).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

180 comments and novice!

Committed cff05b2 and pushed to 8.0.x. Thanks!

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

  • alexpott committed cff05b2 on
    Issue #1898478 by joelpittet, Cottser, lokapujya, m1r1k, jstoller, er....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.