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

CommentFileSizeAuthor
#52 duplicate_variables.patch721 byteststoeckler
#47 drupal8.routing-system.2076283-47.patch37.66 KBneclimdul
#43 drupal8.routing-system.2076283-42.patch37.67 KBneclimdul
#43 interdiff.txt1.21 KBneclimdul
#42 local_task-2076283-42.patch37.67 KBdawehner
#42 interdiff.txt1.21 KBdawehner
#40 drupal8.routing-system.2076283-40.patch37.68 KBneclimdul
#40 interdiff.txt559 bytesneclimdul
#38 drupal8.routing-system.2076283-38-FAIL.patch37.57 KBneclimdul
#38 drupal8.routing-system.2076283-38.patch37.68 KBneclimdul
#38 interdiff.txt6.97 KBneclimdul
#36 local_task-2076283-36.patch32.82 KBdawehner
#36 interdiff.txt1.22 KBdawehner
#34 local_task-2076283-34.patch32.14 KBdawehner
#34 interdiff.txt456 bytesdawehner
#32 local_task-2076283-32.patch32.14 KBdawehner
#32 interdiff.txt1.92 KBdawehner
#31 local_task-2076283-31.patch24.44 KBpwolanin
#24 local_task-2076283-24.patch30.31 KBpwolanin
#24 2076283-23-24.increment.txt2.69 KBpwolanin
#23 local_task-2076283-23.patch30.4 KBdawehner
#23 interdiff.txt1.56 KBdawehner
#19 local_task-2076283-19.patch28.32 KBdawehner
#19 interdiff.txt5.05 KBdawehner
#18 local_task-2076283-18.patch24.85 KBpwolanin
#18 2076283-15-18.increment.txt1.65 KBpwolanin
#15 local_task-2076283-15.patch31.58 KBdawehner
#15 interdiff.txt2.2 KBdawehner
#14 local_task-2076283-14.patch24.38 KBpwolanin
#14 2076283-12-14.increment.txt7.05 KBpwolanin
#12 local_task-2076283-12.patch18.12 KBpwolanin
#12 2076283-10-12.increment.txt15.26 KBpwolanin
#10 interdiff.txt3.26 KBdawehner
#10 local_task-2076283-10.patch15.07 KBdawehner
#9 local_task-2076283-9.patch14.94 KBdawehner
#9 interdiff.txt723 bytesdawehner
#7 local_task-2076283-7.patch14.66 KBdawehner
#5 local_task-2076283-5.patch14.25 KBdawehner
#5 interdiff.txt7.41 KBdawehner
#3 local_task-2076283-3.patch11.73 KBdawehner
#3 interdiff.txt815 bytesdawehner
#1 local_task-2076283-1.patch10.93 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
10.93 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
815 bytes
11.73 KB

.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.41 KB
14.25 KB

Reworked the patch to now use checkNamedRoute etc.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.66 KB

Rerolled.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
723 bytes
14.94 KB

Yet another rerole error.

dawehner’s picture

FileSize
15.07 KB
3.26 KB

Some fixes here and there.

dawehner’s picture

pwolanin’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
24.38 KB

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

dawehner’s picture

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.

pwolanin’s picture

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.

dawehner’s picture

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.

pwolanin’s picture

Improve code comments including an actual route and less ambiguous example

dawehner’s picture

FileSize
5.05 KB
28.32 KB
-      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.

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
30.4 KB

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

pwolanin’s picture

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.

pwolanin’s picture

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

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

pwolanin’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
pwolanin’s picture

Issue tags: -WSCCI

issue tag weirdness...

pwolanin’s picture

Issue tags: +WSCCI

and more such

pwolanin’s picture

working on the re-roll now...

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
24.44 KB

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

dawehner’s picture

Issue tags: +PHPUnit
FileSize
1.92 KB
32.14 KB
  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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
456 bytes
32.14 KB

This is it.

dawehner’s picture

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

dawehner’s picture

FileSize
1.22 KB
32.82 KB

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

neclimdul’s picture

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

neclimdul’s picture

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.

dawehner’s picture

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

neclimdul’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
37.67 KB

This should be it.

neclimdul’s picture

Status: Needs review » Needs work
FileSize
1.21 KB
37.67 KB

oh right, and pattern change.

neclimdul’s picture

Status: Needs work » Needs review
pwolanin’s picture

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

Great work with the added test coverage and use cases.

tim.plunkett’s picture

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

neclimdul’s picture

just a reroll to fix a conflict. no changes.

catch’s picture

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.

pwolanin’s picture

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.

tstoeckler’s picture

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.

tstoeckler’s picture

Status: Active » Needs review
FileSize
721 bytes

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.

pwolanin’s picture

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.

dawehner’s picture

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.