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

Files: 
CommentFileSizeAuthor
#85 2102777-85-docs-followup.patch1.21 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]
#75 interdiff-2102777-75.txt2.35 KBdamiankloip
#75 2102777-75.patch14.07 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,741 pass(es).
[ View ]
#74 2102777-74.patch17.32 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,642 pass(es).
[ View ]
#68 2102777-68.patch13.55 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2102777-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#68 interdiff-2102777-68.txt3.05 KBdamiankloip
#66 2102777-66.patch16.6 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#66 interdiff-2102777-66.txt2.09 KBdamiankloip
#63 2102777-63.patch16.99 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#63 interdiff-2102777-63.txt882 bytesdamiankloip
#60 2102777-60.patch17.62 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,928 pass(es).
[ View ]
#60 interdiff-2102777-60.txt1.98 KBdamiankloip
#56 2102777-56.patch16.67 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es).
[ View ]
#40 2102777-40.patch18.55 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,803 pass(es).
[ View ]
#40 xhprof-40.tar_.gz16.54 KBdamiankloip
#34 2102777-34.patch21.61 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,036 pass(es).
[ View ]
#32 2102777-32.patch22.96 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,831 pass(es), 1,199 fail(s), and 590 exception(s).
[ View ]
#29 2102777-28.patch30.45 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]
#29 interdiff-2102777-28.txt1.84 KBdamiankloip
#23 2102777-23.patch31.28 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,767 pass(es).
[ View ]
#23 interdiff-2102777-23.txt3.75 KBdamiankloip
#16 links-2102777-16.patch30.46 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,700 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#16 interdiff.txt11.04 KBdawehner
#11 interdiff-2102777-11.txt3.72 KBdamiankloip
#11 2102777-11-tim.txt11.74 KBdamiankloip
#11 2102777-11.patch25.95 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es).
[ View ]
#9 2102777-9.patch11.44 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,978 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 interdiff-2102777-9.txt3.17 KBdamiankloip
#7 2102777-7.patch8.26 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#7 interdiff-2102777-7.txt966 bytesdamiankloip
#4 2102777-4.patch9.21 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,302 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#2 2102777-2.patch7.64 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,345 pass(es).
[ View ]
#2 interdiff-2102777-2.txt2.23 KBdamiankloip
#1 d8.operation-route-conv.patch5.41 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,579 pass(es), 60 fail(s), and 23 exception(s).
[ View ]

Comments

StatusFileSize
new5.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,579 pass(es), 60 fail(s), and 23 exception(s).
[ View ]

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.

Status:Active» Needs review
StatusFileSize
new2.23 KB
new7.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,345 pass(es).
[ View ]

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

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

StatusFileSize
new9.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,302 pass(es), 39 fail(s), and 1 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new966 bytes
new8.26 KB
FAILED: [[SimpleTest]]: [MySQL] 58,788 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
new11.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,978 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Let's see how this gets on.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new25.95 KB
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es).
[ View ]
new11.74 KB
new3.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....

Title:Convert operation links to use route_name instead of hrefAllow theme_links to use routes as well as href

+++ 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 :(

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

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

StatusFileSize
new11.04 KB
new30.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,700 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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.

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

We should *not* remove them.

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

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

Status:Needs work» Needs review
StatusFileSize
new3.75 KB
new31.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,767 pass(es).
[ View ]

This should fix that.

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!

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

This should get some eyeballs by the Twig folks.

Don't you consider thedavidmeister as twig folks?

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

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!

StatusFileSize
new1.84 KB
new30.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]

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

Status:Needs work» Reviewed & tested by the community

This addressed the points mentioned in #29.

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?

Status:Needs work» Needs review
StatusFileSize
new22.96 KB
FAILED: [[SimpleTest]]: [MySQL] 57,831 pass(es), 1,199 fail(s), and 590 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new21.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,036 pass(es).
[ View ]

Whoops.

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

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

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

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.

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

StatusFileSize
new16.54 KB
new18.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,803 pass(es).
[ View ]

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.

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? :/

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

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

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.

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%

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

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

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.

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

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

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.

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.

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.

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

Then we can bump the other issues.

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.

Issue tags:+Needs profiling
StatusFileSize
new16.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es).
[ View ]

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.

Issue tags:-Needs profiling

Meh

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

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?

StatusFileSize
new1.98 KB
new17.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,928 pass(es).
[ View ]

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%

Status:Needs review» Reviewed & tested by the community

These numbers are promising.

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?

StatusFileSize
new882 bytes
new16.99 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

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

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

StatusFileSize
new2.09 KB
new16.6 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Whoooops. Interface changes needed.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
new13.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2102777-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

seems like

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

could go outside the loop also, just below

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

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.

StatusFileSize
new17.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,642 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new14.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,741 pass(es).
[ View ]
new2.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.

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.

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.

Status:Needs review» Reviewed & tested by the community

This was RTBC before, and the rerolls look good.

+1

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.

Issue tags:+WSCCI

Title:Allow theme_links to use routes as well as hrefChange 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,

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.

Title:Change notice: Allow theme_links to use routes as well as hrefChange notice + Docs follow-up: Allow theme_links to use routes as well as href
Status:Active» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]

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.

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

Issue tags:-Needs change record

Removing tag.

Title:Change notice + Docs follow-up: Allow theme_links to use routes as well as hrefDocs 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.

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

Issue tags:-blocker

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

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.

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.

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

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

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!

Title:Docs follow-up: Allow theme_links to use routes as well as hrefAllow theme_links to use routes as well as href

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

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.