Updated: Comment 0

Problem/Motivation

Currently you can't use local tasks with dynamic values, like node/{}/value without extending the base plugin class. This is kind of problematic
for things like the config translation module or just the field UI

Additional like on the link generator issue, the access checking is currently doing way too much in order to check access.

Proposed resolution

Instead of generating the path the plugins should return just the route name and parameters. These parameters could be pulled from either the current page
or from the route definitions (yml probably in most cases).

If we have the route name/parameters checking access is way easier.

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#52 duplicate_variables.patch721 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,792 pass(es).
[ View ]
#47 drupal8.routing-system.2076283-47.patch37.66 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,535 pass(es).
[ View ]
#43 drupal8.routing-system.2076283-42.patch37.67 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,240 pass(es).
[ View ]
#43 interdiff.txt1.21 KBneclimdul
#42 local_task-2076283-42.patch37.67 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,294 pass(es).
[ View ]
#42 interdiff.txt1.21 KBdawehner
#40 drupal8.routing-system.2076283-40.patch37.68 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 59,126 pass(es), 13 fail(s), and 311 exception(s).
[ View ]
#40 interdiff.txt559 bytesneclimdul
#38 drupal8.routing-system.2076283-38-FAIL.patch37.57 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 59,236 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#38 drupal8.routing-system.2076283-38.patch37.68 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,155 pass(es).
[ View ]
#38 interdiff.txt6.97 KBneclimdul
#36 local_task-2076283-36.patch32.82 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es).
[ View ]
#36 interdiff.txt1.22 KBdawehner
#34 local_task-2076283-34.patch32.14 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]
#34 interdiff.txt456 bytesdawehner
#32 local_task-2076283-32.patch32.14 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#32 interdiff.txt1.92 KBdawehner
#31 local_task-2076283-31.patch24.44 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#24 local_task-2076283-24.patch30.31 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,488 pass(es).
[ View ]
#24 2076283-23-24.increment.txt2.69 KBpwolanin
#23 local_task-2076283-23.patch30.4 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,898 pass(es).
[ View ]
#23 interdiff.txt1.56 KBdawehner
#19 local_task-2076283-19.patch28.32 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 20 fail(s), and 20 exception(s).
[ View ]
#19 interdiff.txt5.05 KBdawehner
#18 local_task-2076283-18.patch24.85 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,597 pass(es).
[ View ]
#18 2076283-15-18.increment.txt1.65 KBpwolanin
#15 local_task-2076283-15.patch31.58 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,985 pass(es).
[ View ]
#15 interdiff.txt2.2 KBdawehner
#14 local_task-2076283-14.patch24.38 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,894 pass(es).
[ View ]
#14 2076283-12-14.increment.txt7.05 KBpwolanin
#12 local_task-2076283-12.patch18.12 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,577 pass(es), 11 fail(s), and 4 exception(s).
[ View ]
#12 2076283-10-12.increment.txt15.26 KBpwolanin
#10 interdiff.txt3.26 KBdawehner
#10 local_task-2076283-10.patch15.07 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,998 pass(es).
[ View ]
#9 local_task-2076283-9.patch14.94 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]
#9 interdiff.txt723 bytesdawehner
#7 local_task-2076283-7.patch14.66 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,896 pass(es), 108 fail(s), and 57 exception(s).
[ View ]
#5 local_task-2076283-5.patch14.25 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2076283-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 interdiff.txt7.41 KBdawehner
#3 local_task-2076283-3.patch11.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,133 pass(es), 90 fail(s), and 51 exception(s).
[ View ]
#3 interdiff.txt815 bytesdawehner
#1 local_task-2076283-1.patch10.93 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,836 pass(es), 1,161 fail(s), and 10,109 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new10.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,836 pass(es), 1,161 fail(s), and 10,109 exception(s).
[ View ]

Let's see what the testbot thinks about that.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new815 bytes
new11.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,133 pass(es), 90 fail(s), and 51 exception(s).
[ View ]

.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.41 KB
new14.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2076283-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reworked the patch to now use checkNamedRoute etc.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,896 pass(es), 108 fail(s), and 57 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new723 bytes
new14.94 KB
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]

Yet another rerole error.

StatusFileSize
new15.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,998 pass(es).
[ View ]
new3.26 KB

Some fixes here and there.

StatusFileSize
new15.26 KB
new18.12 KB
FAILED: [[SimpleTest]]: [MySQL] 58,577 pass(es), 11 fail(s), and 4 exception(s).
[ View ]

Here's a new version that takes the route parameters from the Request.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.05 KB
new24.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,894 pass(es).
[ View ]

A couple of those look like sporadic fails (don't et a fail locally for PageNotFoundTest) and the LocalTasksTest is actually looking for the active class on the wrong element - the A instead of the LI.

So, this has been a bogus test for a while. however, the change to the link generator should not have affected that.

This patch adds comment local tasks - though really we want the YAML implementation in so we don't have to re-convert them.

Bug opened here: #2084125: Bug in active detection in LinkGenerator - need to extract array from _raw_variables ParameterBag

StatusFileSize
new2.2 KB
new31.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,985 pass(es).
[ View ]

Merged in many of my ideas from the other issue: #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters and fixed some types etc.

I'm not sure this is correct or needed?

+      elseif ($request->attributes->has($name)) {

The ParamConverterManager puts the raw values of all the route variables into _raw_variables regardless of whether or not they are later upcast.

Well, I personally believe in decoupling of code, so from my perspective it would be cool to not couple the link generator onto the way our the paramconverterManager does stuff,
at least support the normal symfony way of doing it.

StatusFileSize
new1.65 KB
new24.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,597 pass(es).
[ View ]

Improve code comments including an actual route and less ambiguous example

StatusFileSize
new5.05 KB
new28.32 KB
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 20 fail(s), and 20 exception(s).
[ View ]

-      else {
-        // @todo throw exception?
-      }
     }
+    // The UrlGenerator will throw an exception if expected parameters are
+    // missing. This method should be overridden if that is possible.
     return $parameters;
   }

+1
  • Fixed a type in your comment
  • Created a separate integration test in LocalTaskTest for placeholders
  • While doing it, I stumbled upon a bug on routes without a menu item in the chain at all.

Status:Needs review» Needs work
Issue tags:-MenuSystemRevamp, -WSCCI

The last submitted patch, local_task-2076283-19.patch, failed testing.

Status:Needs work» Needs review

#19: local_task-2076283-19.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+MenuSystemRevamp, +WSCCI

The last submitted patch, local_task-2076283-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.56 KB
new30.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,898 pass(es).
[ View ]

Not really surprising to have failures if you have local file missing to the patch.

StatusFileSize
new2.69 KB
new30.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,488 pass(es).
[ View ]

Fixes local fails for me by removing the weights on the test plugins (by default the root one gets -10)

Also, remove unused use statements.

Status:Needs review» Needs work
Issue tags:-MenuSystemRevamp, -WSCCI

The last submitted patch, local_task-2076283-24.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+MenuSystemRevamp, +WSCCI

#24: local_task-2076283-24.patch queued for re-testing.

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

Issue tags:-WSCCI

issue tag weirdness...

Issue tags:+WSCCI

and more such

working on the re-roll now...

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new24.44 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Since files are renamed, etc I don't think an interdiff is meaningful here. BAsically the same as #24 with conversion to YAML files.

Issue tags:+phpunit
StatusFileSize
new1.92 KB
new32.14 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
  1. +++ b/core/includes/menu.inc
    @@ -1958,6 +1965,20 @@ function menu_local_tasks($level = 0) {
    +    $route_name = Drupal::request()->attributes->get('_route');

    This should be using RouteObjectInterface::ROUTE_NAME

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -88,6 +89,37 @@ public function getRouteName() {
    +  public function getRouteParameters(Request $request) {

    Let's put some @inheritdoc on there.

I just realized that #2076283-18: Allow local task plugins on paths with a dynamic value (like a node) dropped the unit tests again. Let's fix that.

Status:Needs review» Needs work

The last submitted patch, local_task-2076283-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new456 bytes
new32.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]

This is it.

#34: local_task-2076283-34.patch queued for re-testing.

StatusFileSize
new1.22 KB
new32.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es).
[ View ]

Added a usecase mentioned by neclimdul: Define the route parameters in the plugin definition.

loving this. Think there might be a bug in trailing parameters though. Working on a test case.

StatusFileSize
new6.97 KB
new37.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,155 pass(es).
[ View ]
new37.57 KB
FAILED: [[SimpleTest]]: [MySQL] 59,236 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Rerolled after routing key rename and with tests for the bug I'd found and a fix from dawehner. Will need another reroll shortly for the pattern->path change but want get this up and out of the way.

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Derivative/LocalTaskTest.php
@@ -0,0 +1,25 @@
+<?php
+/**

Nitpick alarm: There should be an empty line + @file

StatusFileSize
new559 bytes
new37.68 KB
FAILED: [[SimpleTest]]: [MySQL] 59,126 pass(es), 13 fail(s), and 311 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal8.routing-system.2076283-40.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new37.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,294 pass(es).
[ View ]

This should be it.

Status:Needs review» Needs work
StatusFileSize
new1.21 KB
new37.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,240 pass(es).
[ View ]

oh right, and pattern change.

Status:Needs work» Needs review

Priority:Normal» Major
Status:Needs review» Reviewed & tested by the community

Great work with the added test coverage and use cases.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
@@ -91,26 +92,43 @@ public function getRouteName() {
-  public function getTitle() {
-    // Subclasses may pull in the request or specific attributes as parameters.
-    return $this->t($this->pluginDefinition['title']);
...
+  public function getTitle() {
+    // Subclasses may pull in the request or specific attributes as parameters.
+    return $this->t($this->pluginDefinition['title']);

This is the only part of the patch I thought was questionable, but it isn't added here, just moved. +1 for RTBC

StatusFileSize
new37.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,535 pass(es).
[ View ]

just a reroll to fix a conflict. no changes.

Title:Allow local task plugins on paths with a dynamic value (like a node)Change notice: Allow local task plugins on paths with a dynamic value (like a node)
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Yes this looks good. Committed/pushed to 8.x.

Will need a change notice.

Title:Change notice: Allow local task plugins on paths with a dynamic value (like a node)Allow local task plugins on paths with a dynamic value (like a node)
Status:Active» Fixed
Issue tags:-Needs change record

Here's a short change notice: https://drupal.org/node/2094027

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Active

Re-opening this for the following problem:

+++ b/core/includes/menu.inc
@@ -1979,6 +1986,20 @@ function menu_local_tasks($level = 0) {
+      foreach ($local_tasks as $level => $items) {

This is overriding the $level variable that is being passed into the function. I'm not 100% through with debugging, but I think this might be the cause of some missing tabs in config_translation that I'm having a problem with.

Status:Active» Needs review
StatusFileSize
new721 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,792 pass(es).
[ View ]

Here we go. The tab funkiness I experienced was in fact not related to this at all. I still think this should be fixed, though. I'm fine with moving this into a follow-up, but I wanted to get some attention from some people who understand what's going on here, first.

Let's move this to a follow-up, since this issue was marked closed/fixed.

Looks indeed like ti would be better not to whack the variable.

Status:Needs review» Fixed

Opened a follow up as it is simple to do that.

#2106967: menu_local_tasks $level gets overridden

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