Convert theme_links() to Twig

Left the debug messages where I am having trouble with the active link tests.

Tests that are failing are at
/Users/joel/Sites/d8/html/core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php

one is because of template_process line #2642

  // Initialize html class attribute for the current hook.
  $variables['attributes']['class'][] = drupal_html_class($hook);

The other is because they expect being an active link in the tests.

Have a crack:)

Whoops add template

Not sure if this is supposed to be tagged for review, but I don't see anything noting that it shouldn't be.

Needs this patch for part of this
#1938430: Don't add a default theme hook class in template_preprocess() where the extra class is added.

-
-  // Initialize html class attribute for the current hook.
-  $variables['attributes']['class'][] = drupal_html_class($hook);

And may need this patch as well (which could be tested) for the active class on the links...
#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig

@steveoliver was having a look at those two items as well. Correct me if I am wrong but you came to the same conclusion?

@Cottser, would overriding the template_preprocess() need to be removed at a later date?

tweaked up the preprocess bit to remove the links... but having issues on the active class on Theme functions in Theme tests.

Ok so still don't know what to do about the active links test but this should solve the exception.

@Cottser, thanks for the suggestion to look at the span attributes, it was pulling the wrong bits and I spotted another missing bit. item.text was missing {{}} on the last part.

So with this patch the only bit left to fix is the 'active' class I hope.

Looks like a few tests need to be updated. on it.

Okay, well it wasn't the tests that were the problem. For some reason the {% spaceless %} tag doesn't seem to do anything within the for loop. I added some whitepsace controlers in there to take care of the line breaks and spaces.

The tests are also testing for the ability to add an ID for the heading. We didn't have any code in our preprocess for IDs, only classes, so I added that.

It also looks like for some reason our front page link is getting an active class (both the A tag and the LI tag) when tests are running. Not sure why that is, but it's definitely an issue. in our preprocess we have $is_current_path = ($link['href'] == current_path() || ($link['href'] == '<front>' && drupal_is_front_page())); Not sure why this wold return true during testing, but it does.

I'm not sure the tests are actually running our preprocess function either, even after making the changes noted above, the tests still fail with missing IDs on the heading tags.

still needs work.

Changes from #19 incorporated. Thank you @jenlampton docs and whitepsace. Then reverted my edits for the header[id] bug you found.

@jenlampton sorry my interdiff was messed up. This it the diff between #19-#20.

Ok so drupal_is_front_page() call wasn't releasing the static variable so I added a bounce sheet to the tests so that it would pickup the new "front page". i.e. drupal_static_reset('drupal_is_front_page');

This should pass test as they passed locally. *crosses fingers*

reviewing :)

Manual testing:
The only difference in the markup is the order of the classes on the UL tag, great job guys!

before:

<ul class="links inline"><li class="node-readmore odd first"><a href="/node/1" rel="tag" title="I am an article">Read more<span class="element-invisible"> about I am an article</span></a></li><li class="comment-comments even"><a href="/node/1#comments" title="Jump to the first comment of this posting." property="sioc:num_replies" content="1" datatype="xsd:integer" rel="">1 comment</a></li><li class="comment-add odd last"><a href="/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>

after:

<ul class="inline links"><li class="node-readmore odd first"><a href="/node/1" rel="tag" title="I am an article">Read more<span class="element-invisible"> about I am an article</span></a></li><li class="comment-comments even"><a href="/node/1#comments" title="Jump to the first comment of this posting." property="sioc:num_replies" content="1" datatype="xsd:integer" rel="">1 comment</a></li><li class="comment-add odd last"><a href="/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>

Docs look pretty good, verbose, but more is better than less. :)

patch does not apply, needs a re-roll on #27

Re-rolling #27 now

re-rolled

I will fix this re-roll my mistake.

By mistake, in re-roll #34, I removed $language_url = language(Language::TYPE_URL);

#37 passed and ready for review

Here's some cleanup that needs to be done.

+++ b/core/includes/theme.incundefined
@@ -1687,13 +1689,21 @@ function theme_link($variables) {
+  // @todo Remove after http://drupal.org/node/1938430 is resolved.

Does this @todo still apply? Looks like #1938430: Don't add a default theme hook class in template_preprocess() is fixed.

+++ b/core/includes/theme.incundefined
@@ -1744,45 +1749,37 @@ function theme_links($variables) {
+          $item += array(

+++ b/core/includes/theme.incundefined
@@ -1744,45 +1749,37 @@ function theme_links($variables) {
+          $item += array(

I have never seen += used for concatenation PHP, nor do I believe it will work in. To add additinal keys to an array shouldn't we be doing something like $items[] = array();?

+++ b/core/modules/system/templates/links.html.twigundefined
@@ -0,0 +1,64 @@
+ *   the 'element-invisible' CSS class. Do not use 'display:none', which

This needs to updated per #1363112: Simplify names of "element-x" helper classes to mention 'visually-hidden' and not 'element-invisible'.

Fixing patch

Made corrections stated in #39.

Did not change: '$item += array(' because this is proper per Cottser.

#41 Passed still needs profiling by someone.

+++ b/core/includes/theme.inc
@@ -1677,13 +1679,20 @@ function theme_status_messages($variables) {
+
+  // Remove 'links' from the attributes array. This is added through
+  // template_preprocess() but we do not need it.
+  if (isset($variables['attributes']['class'][0]) && $variables['attributes']['class'][0] == 'links') {
+    unset($variables['attributes']['class'][0]);
+    if (empty($variables['attributes']['class'])) {
+      unset($variables['attributes']['class']);
+    }
+  }
+  $variables['attributes'] = new Attribute($variables['attributes']);

This whole chunk can be removed.

+++ b/core/modules/system/templates/links.html.twig
@@ -0,0 +1,64 @@
+ *   If the heading is an array, it can have the following elements:

Shouldn't say the type in a twig doc, array.
Just remove this line.

+++ b/core/modules/system/templates/links.html.twig
@@ -0,0 +1,64 @@
+ *   - attributes: (optional) A keyed array of attributes for the heading.

This should not say 'array' this should read a list.

Made changes from #43

#44 passed still needs review and profiling.

I added three articles to my site, and I found that they all had read more links. These links seem to be rendered through theme_links(). This patch seems to be working fine.

Profiling

Scenario
- Devel generate
- Generate 50 menu links in the Main Navigation menu. Allow up to 50 links on the menu with a depth of 1

=== 8.x..8.x compared (51ec5ca916d7d..51ec5cfb002ea):

ct  : 133,342|133,342|0|0.0%
wt  : 532,495|534,057|1,562|0.3%
cpu : 518,900|518,998|98|0.0%
mu  : 18,330,840|18,330,840|0|0.0%
pmu : 18,391,368|18,391,368|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec5ca916d7d&...

=== 8.x..1939064-theme_links compared (51ec5ca916d7d..51ec5d631cb28):

ct  : 133,342|135,809|2,467|1.9%
wt  : 532,495|540,491|7,996|1.5%
cpu : 518,900|527,997|9,097|1.8%
mu  : 18,330,840|18,395,136|64,296|0.4%
pmu : 18,391,368|18,457,408|66,040|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec5ca916d7d&...

Removing the needs profiling tag

#44: 1939064-44.patch queued for re-testing.

Re-rolled and removed extra whitespace modifiers and intented the contents of the spaceless control. Also removed the @see template_preprocess() in the docblock.

Hmm forgot that the modifiers are need inside the loop. Spaceless doesn't penetrate the for loop.

#56: 1939064-56-twig-links.patch queued for re-testing.

I re-ran the profiling similar but I turned on all permissions for anonymous and did 250 links on all menus using devel. So this should also grab a bit of menus that are generated in admin as well.
Without the spaceless things looks better on the # of function calls.

=== 8.x..8.x compared (52353d82849e7..52353dd1139c1):

ct  : 70,607|70,607|0|0.0%
wt  : 671,741|671,711|-30|-0.0%
cpu : 637,211|636,387|-824|-0.1%
mu  : 40,771,304|40,771,304|0|0.0%
pmu : 40,894,968|40,894,968|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52353d82849e7&...

=== 8.x..1939064-twig-links compared (52353d82849e7..52353e715a325):

ct  : 70,607|71,206|599|0.8%
wt  : 671,741|678,259|6,518|1.0%
cpu : 637,211|643,214|6,003|0.9%
mu  : 40,771,304|40,880,232|108,928|0.3%
pmu : 40,894,968|41,003,104|108,136|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52353d82849e7&...

+++ b/core/includes/theme.inc
@@ -1664,21 +1663,17 @@ function theme_links($variables) {
+      $heading['text'] = check_plain($heading['text']);

We can use String::checkPlain now.

So, the function theme_links() no longer exists in this patch yet we have a bunch of documentation that still references it. I found 14 matches for 'theme_links()' with this patch applied.

Also, in reference to #60, as well as check_plain() being used, it's also referenced in the documentation of links.html.twig

Thanks @jibran and @thedavidmeister! Good catches!

#62: 1939064-62-twig-links.patch queued for re-testing.

#62: 1939064-62-twig-links.patch queued for re-testing.

In the preprocess the heading.level is not assigned with any value, and on the other hand in the old theme_links() it only sett statically to 'h2'.

So there seems be no reason to keep any of that in the preprocess nor in the twig file.

The documentation is also edited to reflect these changes.

A small error got in to the previous patch file. (The twig file was missing)

Here's the correct file.

Use the correct (?) method of adding new files "git diff"...

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

@farrington could you please provide an interdiff for the changes between #62 - #69 and #69to #71?

That way we can easily what you changed between the patches.

Thank you for working on this!

Here are the interdiffs.

I had to do a reroll of the patch as it would no longer apply.

I really like that the template now has the h2 in it instead of a variable for it's level. Thanks @farrington:) and thank you @drupalninja99 for the re-roll. I'm going to see if I can't get rid of those errors, now which are very likely related to the removal of the level variable. This may constitute as an API change... but let's hope not because it shouldn't be so.

On second thought, I'd really like to keep this issue as a straight conversion issue, get it into core first. I've created a follow up issue once this is complete and in core.
#2116359: Leave the heading level in the template and out of the variables for links.html.twig

Meantime this is the re-rolled and reverted those changes.

Remove sandbox link

79: 1939064-79-twig-links.patch queued for re-testing.

Rerolled the latest patch

86: 1939064-85-convert-theme-links.patch queued for re-testing.

+++ b/core/includes/menu.inc
@@ -2186,6 +2186,114 @@ function _menu_get_legacy_tasks($router_item, &$data, &$root_path) {
+ * Retrieves contextual links for a path based on registered local tasks.
+ *
+ * This leverages the menu system to retrieve the first layer of registered
+ * local tasks for a given system path. All local tasks of the tab type
+ * MENU_CONTEXT_INLINE are taken into account.
+ *
...
+function menu_contextual_links($module, $parent_path, $args) {
...

looks like menu_contextual_links snuck back into this patch. I doubt that has to do with the errors but compare yours to the one you are rerolling and you may spot the answer.

Sorry, @mark.labrecque this issue is a bit trickier than I originally thought for the re-roll. A bunch of router related stuff for links got in there and some of the code improvements were made similarly when those got in. I'll try to do a re-roll here from scratch.

I am sure this will fail, but it seems to be because the active classes aren't working... again. That static reset for the homepage didn't seem to fix it so I removed that for now. I also removed spaceless for now and just added more modifiers but would rather have no whitespace modifiers it's quite a task to rewrite all the tests to be xpath or something that doesn't care about whitespace.

It has to do with the fallback in _current_path in current_path... this is referencing #1269742: Make path lookup code into a pluggable class.

Let's see what the errors look like though from testbot...

Here's the failed output for reference:
Result:
'<h2>Links heading</h2><ul id="somelinks"><li class="a-link odd first"><a href="/a/link">A &lt;link&gt;</a></li><li class="plain-text even">Plain &quot;text&quot;</li><li class="front-page odd"><a href="/">Front page</a></li><li class="router-test even last"><a href="/router_test/test1">Test route</a></li></ul>'

Expected:

'<h2>Links heading</h2><ul id="somelinks"><li class="a-link odd first"><a href="/a/link">A &lt;link&gt;</a></li><li class="plain-text even">Plain &quot;text&quot;</li><li class="front-page odd active"><a href="/" class="active">Front page</a></li><li class="router-test even last"><a href="/router_test/test1">Test route</a></li></ul>'

The good news: there are only 4 fails and they are the ones I knew would fail.
The bad news: I've no idea how to make the drupal_get_front_page() know it's active inside the test.

Help please!

@larowlan++ helped me out here. Turns out the l() was not being active because it was getting the query params from the Request service. So in the test I disabled them.

Reroll.

I've got some better performance this time, likely due to the twig engine updates to 1.15.

=== 8.x..8.x compared (52e6fc5cc809e..52e6fca507bc1):

ct  : 73,852|73,852|0|0.0%
wt  : 502,535|502,970|435|0.1%
cpu : 493,614|493,689|75|0.0%
mu  : 32,302,512|32,302,512|0|0.0%
pmu : 32,425,320|32,425,320|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e6fc5cc809e&...

=== 8.x..1939064-twig-links compared (52e6fc5cc809e..52e6fcee66138):

ct  : 73,852|74,438|586|0.8%
wt  : 502,535|505,093|2,558|0.5%
cpu : 493,614|496,230|2,616|0.5%
mu  : 32,302,512|32,393,072|90,560|0.3%
pmu : 32,425,320|32,514,400|89,080|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e6fc5cc809e&...

Also did another markup review and there are no differences there too.
Passes testbot, this one is ready to go.

Still RTBC:) Thanks @Cottser

1939064-102.patch no longer applies.

That was a tricky re-roll for such a simple change. Just dropped the hunk:

diff --git a/core/modules/system/tests/modules/ajax_test/ajax_test.module b/core/modules/system/tests/modules/ajax_test/ajax_test.module
index 2ea9eb9..e78d163 100644
--- a/core/modules/system/tests/modules/ajax_test/ajax_test.module
+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.module
@@ -120,7 +120,7 @@ function ajax_test_dialog() {
     ),
   );

-  // Dialog behavior applied to links rendered by theme_links().
+  // Dialog behavior applied to links rendered by links.html.twig.
   $build['links'] = array(
     '#theme' => 'links',
     '#links' => array(

Back to RTBC.

Oh yeah here's the patch.

I think this probably needs some change notice action, or maybe an update to an existing change notice for theme_*() removals? Can someone list change records that might need an update here? Otherwise, if we should draft a new one instead, we can do that now!
https://groups.drupal.org/node/402688

I don't think we did that because we just remove the theme function and swap with a template file. Maybe should just haven't done it yet for any of them in #1757550: [Meta] Convert core theme functions to Twig templates

After applying this patch there are still 4 mentions of theme_links() in the code. As this function is being removed that's surprising.

$ grep -r "theme_links" ./
.//includes/theme.inc: *   - set_active_class: (optional) Whether theme_links() should compare the
.//includes/theme.inc: * theme_links() unfortunately duplicates the "active" class handling of l() and
.//modules/system/system.module:  // @see theme_links()
.//modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestController.php:    // Dialog behavior applied to links rendered by theme_links().

Thanks @alexpott those must have snuck in there from the looks of them when some of the url routing changes and theme_links type=>link consolidation was going on.

112: 1939064-twig-links-112.patch queued for re-testing.

Random testbot fails.

Testbot you can do it!

112: 1939064-twig-links-112.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!