Updated: Comment #N

Problem/Motivation

We need to get with the times and have our operation links use routes directly instead of path (href). This is definitely needed to be able to integrate CSRF tokens into the routing system.

Proposed resolution

Convert operation link items to use 'route_name' and 'route_parameters' instead of 'href' key. theme_links is used for dropbuttons so this is where it needs to be fixed.

Remaining tasks

Convert remaining operation links (entity list controller in another issue as this is a different complexity?), Modify theme_links to accept route_name and route_parameters etc.. See drupal_pre_render_link as this is currently doing what we need.

User interface changes

None

API changes

Links and operations can use route names directly.

#1798296: Integrate CSRF link token directly into routing system

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Here is a start, this is what we need to do when converting the links, this will not work currently due to changes needed in the summary, plus this is only a handful ofthe conversions.

damiankloip’s picture

Status: Active » Needs review
FileSize
2.23 KB
7.64 KB

So we could modify theme_links something like this? This is a quick/rough attempt...

dawehner’s picture

It is great to see that there aren't any problems with converting things to route_name/route_parameters anymore.

+++ b/core/includes/theme.inc
@@ -1689,6 +1689,18 @@ function theme_links($variables) {
       if (isset($link['href'])) {

When we added links to theme_local_task and co we realized that 'href' is often set for some reasons, so maybe it is safer to check for route_name first. One question: can't we put all parameters to the link element and then let drupal_pre_render_link decide what to do? (this already has the needed logic built in).

damiankloip’s picture

FileSize
9.21 KB

That's a good point, moved the check for route_name first.

Regarding passing all parameters: We could set defaults, then just pass everything along so pre_process_link will take care of it, including the active class on the link (which is doesn't do atm). This would make it more consistent, as the active class for a route created link is on the link, and for a href created link is on the wrapper around the link.

Status: Needs review » Needs work

The last submitted patch, 2102777-4.patch, failed testing.

thedavidmeister’s picture

+    // Check if the current path is active.
+    $language_url = language(Language::TYPE_URL);
+    $is_current_path = ($element['#href'] == current_path() || ($element['#href'] == '<front>' && drupal_is_front_page()));
+    $is_current_language = (empty($link['language']) || $link['language']->id == $language_url->id);
+    if ($is_current_path && $is_current_language) {
+      $element['#options']['attributes']['class'][] = 'active';
+    }
+
     $element['#markup'] = l($element['#title'], $element['#href'], $element['#options']);

This makes no sense to me. l() does almost identical (more efficient) active class checking internally already, and there's work being done to make it more efficient at #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

AFAICS, theme_links() has no reason to try and handle active classes on behalf of the individual links it is rendering, so that code should just be removed.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
966 bytes
8.26 KB

That's what I like to hear, we can just remove it!

Let's see how this get's on with the tests now. Should actually fix most of them I hope.

Status: Needs review » Needs work

The last submitted patch, 2102777-7.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
11.44 KB

Let's see how this gets on.

Status: Needs review » Needs work

The last submitted patch, 2102777-9.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
25.95 KB
11.74 KB
3.72 KB

Ok, I spoke to Tim on IRC, and we needed to revert some of the stuff added in #2091691: Convert test non-form page callbacks to routes and controllers as using {entity} as the slug in the path causes issues (slap on the wrist content_translation ;)). This makes sure that the dynamic links created by content_translation us {$entity_type} as the slug, we then get '{comment}', {node} etc.. and peace gets restored.

This should* fix the tests now....

damiankloip’s picture

Title: Convert operation links to use route_name instead of href » Allow theme_links to use routes as well as href
tim.plunkett’s picture

+++ b/core/includes/theme.inc
@@ -1689,31 +1693,24 @@ function theme_links($variables) {
-        $is_current_path = ($link['href'] == current_path() || ($link['href'] == '<front>' && drupal_is_front_page()));
-        $is_current_language = (empty($link['language']) || $link['language']->id == $language_url->id);
-        if ($is_current_path && $is_current_language) {
-          $class[] = 'active';
-        }

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageSwitchingTest.php
@@ -72,12 +72,6 @@ function testLanguageBlock() {
-      if (in_array('active', $classes)) {
-        $links['active'][] = $langcode;
-      }
-      else {
-        $links['inactive'][] = $langcode;
-      }

@@ -86,7 +80,6 @@ function testLanguageBlock() {
-    $this->assertIdentical($links, array('active' => array('en'), 'inactive' => array('fr')), 'Only the current language list item is marked as active on the language switcher block.');

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php
@@ -164,7 +164,7 @@ function testLinks() {
-    $expected_links .= '<li class="front-page odd last active"><a href="' . url('<front>') . '" class="active">' . check_plain('Front page') . '</a></li>';
+    $expected_links .= '<li class="front-page odd last"><a href="' . url('<front>') . '" class="active">' . check_plain('Front page') . '</a></li>';

@@ -196,7 +196,7 @@ function testLinks() {
-    $expected_links .= '<li class="front-page odd last active"><a href="' . url('<front>') . '" class="active">' . check_plain('Front page') . '</a></li>';
+    $expected_links .= '<li class="front-page odd last"><a href="' . url('<front>') . '" class="active">' . check_plain('Front page') . '</a></li>';

I don't think these changes can be made here. This has a rather long in-depth discussion of its own in #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

If its absolutely required to make this work, we're blocked on that :(

damiankloip’s picture

Is that really the case? This just removes the active class from the actual wrapper around the link and not the actual link. That remains untouched. I have been told we can quite happily remove the active on the li element as we're kind of duplicating stuff we don't need...

tim.plunkett’s picture

Ah yeah I missed that it was on the LI not the A.
But.

grep -nr "li\.active" *
core/modules/overlay/css/overlay-child.css:130:#overlay-tabs li.active a,
core/modules/overlay/css/overlay-child.css:131:#overlay-tabs li.active a.active,
core/modules/overlay/css/overlay-child.css:132:#overlay-tabs li.active a:active,
core/modules/overlay/css/overlay-child.css:133:#overlay-tabs li.active a:visited {
core/modules/overlay/css/overlay-child.css:142:#overlay-tabs li.active a:focus,
core/modules/overlay/css/overlay-child.css:143:#overlay-tabs li.active a:hover {
core/modules/views_ui/config/tour.tour.views-ui.yml:15:      data-class: views-display-top li.active
core/modules/views_ui/css/views_ui.admin.theme.css:483:.views-displays ul.secondary li.active a.active.error,
core/modules/views_ui/css/views_ui.admin.theme.css:498:.views-displays ul.secondary li.active a,
core/modules/views_ui/css/views_ui.admin.theme.css:499:.views-displays ul.secondary li.active a.active {
core/themes/bartik/css/colors.css:13:#main-menu-links li.active-trail a {
core/themes/bartik/css/colors.css:19:.tabs ul.primary li.active a {
core/themes/bartik/css/style.css:1126:.tabs ul.primary li.active a {
core/themes/bartik/css/style.css:1146:.tabs ul.primary li.active a {
core/themes/seven/install-page.css:122:.install-task-list li.active {
core/themes/seven/install-page.css:127:.install-task-list li.active:after {
core/themes/seven/install-page.css:141:[dir="rtl"] .install-task-list li.active:after {
core/themes/seven/style.css:283:ul.primary li.active a {
core/themes/seven/style.css:300:[dir="rtl"] ul.primary li.active a {
core/themes/seven/style.css:303:ul.primary li.active a,
core/themes/seven/style.css:304:ul.primary li.active a.active,
core/themes/seven/style.css:305:ul.primary li.active a:active,
core/themes/seven/style.css:306:ul.primary li.active a:visited {
core/themes/seven/style.css:313:ul.primary li.active a:hover {
core/themes/seven/style.css:344:ul.secondary li.active a,
core/themes/seven/style.css:345:ul.secondary li.active a.active {
core/themes/seven/style.css:350:ul.secondary li.active a,
core/themes/seven/style.css:351:ul.secondary li.active a.active {
core/themes/seven/style.css:396:  .touch ul.primary li.active a {
core/themes/seven/style.css:1154:.task-list li.active {
core/themes/seven/style.css:1159:[dir="rtl"] .task-list li.active {
core/themes/seven/style.css:1453:.views-displays ul.secondary li.active a,
core/themes/seven/style.css:1454:.views-displays ul.secondary li.active a.active {
core/themes/seven/style.css:1462:.views-displays ul.secondary li.active a,
core/themes/seven/style.css:1463:.views-displays ul.secondary li.active a.active {

This will be a problem

dawehner’s picture

FileSize
11.04 KB
30.46 KB

Added a new function on the link generator to support extracting the information about the activeness of the route_name/route parameters.

Additional I reverted some of the test changes.

Status: Needs review » Needs work

The last submitted patch, links-2102777-16.patch, failed testing.

thedavidmeister’s picture

aaaaah, it's on the li. I knew I must have been missing something obvious. Well @tim.plunkett, I don't remember the linked issue touching on theme_links at all, it's all focussing on l() over there.

Additionally, the active class logic on theme_links is a little different to the logic on l().

Added a new function on the link generator to support extracting the information about the activeness of the route_name/route parameters.

amazing.

damiankloip’s picture

Hang on, don't we still want to go with removing the active on the li elements?

tim.plunkett’s picture

We should *not* remove them.

thedavidmeister’s picture

Nobody wants to remove active classes from where they are currently.

damiankloip’s picture

OK, I literally don't care either way :-) that's just what we discussed before.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
31.28 KB

This should fix that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh I am sorry with all the bugs at my changes in the middle of the night!

webchick’s picture

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

This should get some eyeballs by the Twig folks.

dawehner’s picture

Don't you consider thedavidmeister as twig folks?

damiankloip’s picture

We did talk to some of the twig guys (thedavidmeister and Fabianx) about this issue before. I will get them to confirm.

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -119,10 +119,7 @@ public function generate($text, $route_name, array $parameters = array(), array
    -    $variables['url_is_active'] = $route_name == $this->active['route_name']
    -      // The language of an active link is equal to the current language.
    -      && (empty($variables['options']['language']) || $variables['options']['language']->id == $this->active['language'])
    -      && $full_parameters == $this->active['parameters'];
    +    $variables['url_is_active'] = $this->isRouteActive($route_name, $full_parameters, $variables);
    

    Please remove that change and inline in the helper function again.

    It is 5-15% slower (IIRC) to not inline and links are in the critical path. Dawehner pointed out that we could inline the attribute handling also and I agree, but that is a nice follow-up issue.

  2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -152,4 +149,16 @@ public function generate($text, $route_name, array $parameters = array(), array
    +  public function isRouteActive($route_name, array $route_parameters, array $variables) {
    

    The helper function is nice for theme_links :).

    But generate() should inline the code as it did before ...

Overall I like the cleanup, but please leave the inline function in.

This could need some profiling / is a good opportunity to benchmark it if any of the converted lists are user facing (instead of just admin facing like book admin list ...).

Thanks!

damiankloip’s picture

FileSize
1.84 KB
30.45 KB

Thanks Fabian. Here is an updated patch with that change reverted.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

This addressed the points mentioned in #29.

thedavidmeister’s picture

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

What Fabianx says in #28 is true, that previously we've considered pulling "is active" functionality into separate functions and it always comes back from profiling as a measurable slowdown :( Can we see what the impact of isRouteActive() is here?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
22.96 KB

Rerolled.

Also I think it is more performant to just call isRouteActive() in this case as the linkGenerator has a static cache of the active route info (see linkGenerator::setRequest). If we didn't use this we would have to \Drupal::request() or cache the active info about the request ourselves in theme_links.

Thoughts?

Status: Needs review » Needs work

The last submitted patch, 2102777-32.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
21.61 KB

Whoops.

dawehner’s picture

So what is the reason to drop big parts of the patch? Was that stuff which should not have been in here?

damiankloip’s picture

Well, basically the same changes were added by commit db6985b.

dawehner’s picture

I fear that people will complain about a missing profiling as this issue had a needs profiling tag.
As I know damian I would be fine without one :p

damiankloip’s picture

I profiled yesterday quickly, and it didn't look great.. Unfortunately I'm now away for the weekend though so can't post anything.

We need this either way as we should be using route names.

thedavidmeister’s picture

We just need some public profiling results so an informed decision can be made by all of us together :)

damiankloip’s picture

FileSize
16.54 KB
18.55 KB

Hmm, this is what I found before:

Run #526bd141c9fe0 Run #526bd15a33d10 Diff Diff%
Number of Function Calls 10,852 194,833 183,981 1695.4%
Incl. Wall Time (microsec) 62,489 1,239,675 1,177,186 1883.8%
Incl. CPU (microsecs) 61,801 1,156,017 1,094,216 1770.5%
Incl. MemUse (bytes) 1,235,488 4,921,912 3,686,424 298.4%
Incl. PeakMemUse (bytes) 1,225,976 5,064,920 3,838,944 313.1%

It causes quite some overhead. See attached archive too.

thedavidmeister’s picture

Those numbers are so bad they're hard to believe :( I think we should leave the "Needs profiling" tag on this issue until we can corroborate that data.

Is there something we're doing here to cause the overhead? what can we do to speed this up beyond inlining the "is active" logic?

This overhead is coming from the route system itself isn't it? :/

damiankloip’s picture

Yea I think so :/ so were basically tied, because we need to use route_name for stuff like this...

Apart from the is active stuff, the rest is just passed to theme_link().

thedavidmeister’s picture

There's no such thing as theme_link(), if you use #type 'link' it goes almost directly to l() or the linkGenerator().

I actually thought we were still pushing the perf tickets in before trying to convert everything to use the generator/routes (only converting links that run access checks for now) - https://drupal.org/node/2047619#comment-7801557

Evidently that's no longer the case? I only say this because none of the tickets mentioned in the linkGenerator issue have been resolved:

- #1965074: Add cache wrapper to the UrlGenerator
- #2058845: Pre-load non-admin routes
- #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links

damiankloip’s picture

There's no such thing as theme_link(), if you use #type 'link' it goes almost directly to l() or the linkGenerator().

Sorry, that's what I meant, fingers doing the thinking there...

I don't know what the grand plan is for performance issues related to routing I'm afraid, but what do we do wait until the 11th hour when we are happier with the performance, then add all this new stuff to the API's?! That doesn't really work either IMO. We certainly can't ship with the current form, which is a muddle of route_name and href across the board.

I think the biggest problem here atm, is that we are loading the linkGenerator for every call to get the active class. I'm not sure moving inline will help too much as there is alot of stuff the link generator needs to get this info.

damiankloip’s picture

So just to give you an idea of what it looks like without the calls to isRouteActive() (I know this adds no active class to the li element so not totally fair, but still shows how much overhead this part adds) and the original current href implementation:

Run #526bd141c9fe0 Run #526cf82f1fd24 Diff Diff%
Number of Function Calls 10,852 11,216 364 3.4%
Incl. Wall Time (microsec) 62,489 63,349 860 1.4%
Incl. CPU (microsecs) 61,801 62,600 799 1.3%
Incl. MemUse (bytes) 1,235,488 1,386,944 151,456 12.3%
Incl. PeakMemUse (bytes) 1,225,976 1,377,736 151,760 12.4%
thedavidmeister’s picture

@damiankloip, I can turn what you're saying around just as easily, are we supposed to worry about performance at the 11th hour?

Anyway, if the active class handling in PHP is adding a 1880% overhead, I think these results should be referenced at #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links - there's still a window of opportunity to get JavaScript based active class handling happening over there which could help avoid the overhead here.

thedavidmeister’s picture

Oh, also, what is the scenario being benchmarked in #40 and #45? How many links are we talking and in what context?

damiankloip’s picture

Well, you could argue the same but api level things affect developer's code directly whereas performance improvements do not. I.e.these changes can be made and people will likely not have to change their module code, whereas if we wait another few months before telling people all of their links have to use route names instead that probably won't fly so well.

The benchmarks I posted we rendering 2 links using theme_links() directly, 100 times. Repeated with href then route_name.

thedavidmeister’s picture

#48 - I know what you're saying. I'm sure you understand what I'm saying too. I guess time pressures are just frustrating everyone.

I don't think you can say that in general performance improvements do not effect APIs, because they can and do influence decisions about how things are implemented. In this case though, I can see that it makes sense to forge ahead with theme_links and try to clean up the "is active" overhead elsewhere.

damiankloip’s picture

#49, Indeed, it is definitely a catch 22 situation... :/

Thanks, I think the active stuff is a general problem that we need to solve, and this will benefit from the outcomes of the other issues.

Do you think this patch should still include that isRouteActive stuff? or should we try to bring that back in a follow up even? We could then only focus on that, as this issue makes a few other changes too.

catch’s picture

Those numbers are shocking but unsurprising - could you post some more detail from the profiling about what's taking most of the time, or is it spread out within route generation?

If this goes in we'll likely need to promote #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links and #2058845: Pre-load non-admin routes to critical.

damiankloip’s picture

Agreed, on the promotions above.

Basically most of the time is the calls to isRouteActive() to add the active class on an li element in theme_links(). Without that (see comment and numbers in #45) we is more similar to the current lookup of href property.

thedavidmeister’s picture

I think we need to leave in the isRouteActive stuff or we'll have regressions in functionality.

I'd be ok with pushing this patch through and marking those two linked issues critical.

damiankloip’s picture

@thedavidmeister, So the patch in #40 is ready to go in your opinion?

Then we can bump the other issues.

thedavidmeister’s picture

Issue tags: -needs profiling

I don't totally understand what this is doing:

 content_translation.local_tasks:
   derivative: 'Drupal\content_translation\Plugin\Derivative\ContentTranslationLocalTasks'
-  class: 'Drupal\content_translation\Plugin\ContentTranslationLocalTasks'
   weight: 100
diff --git a/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationForm.php b/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationForm.php
index 4ec42ba..a81083a 100644
--- a/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationForm.php
+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/ContentTranslationForm.phpundefined
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * @file
  * Contains \Drupal\content_translation\Form\ContentTranslationForm.
diff --git a/core/modules/content_translation/lib/Drupal/content_translation/Plugin/ContentTranslationLocalTasks.php b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/ContentTranslationLocalTasks.php
deleted file mode 100644
index 172b6be..0000000
--- a/core/modules/content_translation/lib/Drupal/content_translation/Plugin/ContentTranslationLocalTasks.php
+++ /dev/nullundefined
@@ -1,18 +0,0 @@
-<?php
-
-/**
- * @file
- * Contains \Drupal\content_translation\Plugin\ContentTranslationLocalTasks.
- */
-
-namespace Drupal\content_translation\Plugin;
-
-use Drupal\Core\Menu\LocalTaskDefault;
-use Symfony\Component\HttpFoundation\Request;
-
-/**
- * Provides route parameter manipulation for content translation local tasks.
- */
-class ContentTranslationLocalTasks extends LocalTaskDefault {
-
-}

I see there were problems in #11, but the code that was causing troubles at the time seems to have changed since then?

Could I just get confirmation that this is still needed?

Other than that, it seems fine to me atm.

damiankloip’s picture

Issue tags: +needs profiling
FileSize
16.67 KB

Yeah, I think the big local task conversion, and another patch for content translation makes that un needed now. Removed those files from the diff.

damiankloip’s picture

Issue tags: -needs profiling

Meh

thedavidmeister’s picture

Can we add a comment or something on the inline active logic saying something along the lines of "This logic is the same as isRouteActive, but inlined for performance"?

thedavidmeister’s picture

Should we try replacing "isRouteActive" with "getActive" and putting that outside foreach ($links as $key => $link) { in theme_links() then inline the active class logic?

damiankloip’s picture

FileSize
1.98 KB
17.62 KB

For sure! I think this is the way to go, xhprof comparison looks much much better :)

Run #5271056f18e21 Run #52710585b2541 Diff Diff%
Number of Function Calls 11,609 11,916 307 2.6%
Incl. Wall Time (microsec) 64,514 67,575 3,061 4.7%
Incl. CPU (microsecs) 63,823 66,821 2,998 4.7%
Incl. MemUse (bytes) 1,318,288 1,390,624 72,336 5.5%
Incl. PeakMemUse (bytes) 1,308,856 1,383,112 74,256 5.7%
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

These numbers are promising.

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs review

does getActive() not need to set the request if active is empty?

should we even introduce isRouteActive() at all if it's so slow?

damiankloip’s picture

FileSize
882 bytes
16.99 KB

does getActive() not need to set the request if active is empty?

huh? No. Where would it get the request from? This is set by the DIC and already called. See symfony docs on setter injection and anywhere else we use this.

should we even introduce isRouteActive() at all if it's so slow?

It's not that it's so slow, it's just how it is being used more than anything. I think it's handy to have it there, but don't really care. Let's remove it for now then.

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

The last submitted patch, 2102777-63.patch, failed testing.

damiankloip’s picture

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

#63: 2102777-63.patch queued for re-testing.

damiankloip’s picture

Whoooops. Interface changes needed.

Status: Needs review » Needs work

The last submitted patch, 2102777-66.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
13.55 KB

Sorry, not having a good morning! Need to remove the isRouteActive assertions from linkGeneratorTest too...

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Anonymous’s picture

seems like

+ $language_url = language(Language::TYPE_URL);

could go outside the loop also, just below

+ $active = \Drupal::linkGenerator()->getActive();

Xano’s picture

68: 2102777-68.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: 2102777-68.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: 2102777-68.patch, failed testing.

damiankloip’s picture

FileSize
17.32 KB

Rerolled. Moved the language back to the top of the theme_links function, thanks beejeebus.

Ugh, looks like #2084463: Convert contextual links to a plugin system similar to local tasks/actions changed theme_links to support route_name. This patch does it properly IMO, and tidies up the function and uses a link type to render instead of calling l() directly (as well as benchmarks it).

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.07 KB
2.35 KB

Sorry, rebased from the wrong branch. This is based on the last passing patch, unlike the previous one...

Interdiff attached to show changes to resolve merge conflict and moving language per comment in #74.

thedavidmeister’s picture

I agree that what we're doing here looks better than the other issue.

Looking at it now though, I do wonder why we call drupal_render() inside a foreach, and then concatenate it onto strings instead of just building a single render array and returning drupal_render($build); right at the end...

But, I think looking at that should be a followup.

damiankloip’s picture

Yes, agreed. I did think that a while back. Another issue is the place though, as how it wraps the li elements currently means it has to work like it does at the moment.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC before, and the rerolls look good.

dawehner’s picture

+1

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +D8MI, +language-config, +blocker, +DX (Developer Experience)

This would make our code in #1952394: Add configuration translation user interface module in core way simpler by requiring less extra method on our interfaces to produce these paths that are now needed... It also cleans up yet another piece of API to work with routes rather than paths, the duplicity of which is really painful for #1952394: Add configuration translation user interface module in core at least, but I suspect for many others. So elevating to major and tagging up.

xjm’s picture

Issue tags: +WSCCI
alexpott’s picture

Title: Allow theme_links to use routes as well as href » Change notice: Allow theme_links to use routes as well as href
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed c4b089f and pushed to 8.x. Thanks!

+++ b/core/includes/theme.inc
@@ -1693,44 +1701,46 @@ function theme_links($variables) {
+      // @todo Reconcile Views usage of 'ajax' as a boolean with the rest of

Is there are follow up already? If not lets get one created.

Fixed the following in commit - replaced a deprecated function in a moved line of code and removed href from a link array where the route_name has been defined.

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 44be532..6ce5c63 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -1678,7 +1678,7 @@ function theme_links($variables) {
     $num_links = count($links);
     $i = 0;
     $active = \Drupal::linkGenerator()->getActive();
-    $language_url = language(Language::TYPE_URL);
+    $language_url = \Drupal::languageManager()->getLanguage(Language::TYPE_URL);

     foreach ($links as $key => $link) {
       $i++;
diff --git a/core/modules/comment/comment.admin.inc b/core/modules/comment/comment.admin.inc
index 0653e9f..2013de8 100644
--- a/core/modules/comment/comment.admin.inc
+++ b/core/modules/comment/comment.admin.inc
@@ -162,7 +162,6 @@ function comment_admin_overview($form, &$form_state, $arg) {
     if (module_invoke('content_translation', 'translate_access', $comment)) {
       $links['translate'] = array(
         'title' => t('translate'),
-        'href' => 'comment/' . $comment->id() . '/translations',
         'route_name' => 'content_translation.translation_overview_comment',
         'route_parameters' => array('comment' => $comment->id()),
         'query' => $destination,
damiankloip’s picture

Yep, good catch, and thank you. That's the correct change :)

afaik, there is no issue for the views ajax thing. I'll create one.

damiankloip’s picture

tstoeckler’s picture

Title: Change notice: Allow theme_links to use routes as well as href » Change notice + Docs follow-up: Allow theme_links to use routes as well as href
Status: Active » Needs review
FileSize
1.21 KB

Wanting to write a change notice for this I noticed that we didn't update the documentation of theme_links() here. Attached patch does so.

tstoeckler’s picture

Created change notice: https://drupal.org/node/2137053
I couldn't get it to link here, it may be that that specific autocomplete got broken by the D7 upgrade. It's pretty short/basic, so feel free to improve on that, but I didn't know what else to say.

Also updated 1 existing change notice: https://drupal.org/node/1989646/revisions/view/2681396/6708387

tstoeckler’s picture

Issue tags: -Needs change record

Removing tag.

Gábor Hojtsy’s picture

Title: Change notice + Docs follow-up: Allow theme_links to use routes as well as href » Docs follow-up: Allow theme_links to use routes as well as href
Component: base system » documentation
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

The docs followup patch looks good :) The change notice too.

Gábor Hojtsy’s picture

BTW I attached the change notice to this issue now. I think the problem was the + is in the title not escaped in the lookup or something along those lines... Now with no +, its trivial to find :)

effulgentsia’s picture

Issue tags: -blocker

The docs followup is nice, but nothing's blocked on it.

xjm’s picture

In the future, please create a separate issue for small followup patches, as @alexpott suggested above. Changing around the status of a single issue like this and having multiple commits against it confuses contributors, pollutes lists of tagged issues, and messes up metrics.

tstoeckler’s picture

This broke the docs gate so should never have been comitted. In that case it is more usual to re-open the issue instead of creating a follow-up.

xjm’s picture

No, @tstoeckler, when a core maintainer asks for it as a followup issue, create it as a followup issue.

Gábor Hojtsy’s picture

Who asked for it? AFAIS @tstoeckler found it independently above :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

I just committed that patch to 8.x, so don't bother with a follow-up. Thanks for being thorough!

jhodgdon’s picture

Title: Docs follow-up: Allow theme_links to use routes as well as href » Allow theme_links to use routes as well as href
xjm’s picture

Disregard previous comment about the change notice being missing; I just don't know how to use Drupal.org anymore. :)

xjm’s picture

Component: documentation » base system
Priority: Normal » Major

But, restoring proper component and priority.

Status: Fixed » Closed (fixed)

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