LocalTaskBase and LocalTaskInterface has to work with routes containing parameters [#2045267]

Updated: Comment #16

Problem/Motivation

At the moment we don't pass parameters from the current request to the url generator, so the API does not work really simply for many cases.

Proposed resolution

In order to implement it, we need probably need the following fixes first:
#2031487: When replacing the upcasted values in the request attributes array, retain the original raw value too
#2046737: Add a method to the AccessManager that only needs a route name and parameters
#2047619: Add a link generator service for route-based links

And is/was blocked on:
#2031353: URLgenerator broken for Drupal installed in a subdirectory - doesn't have a way to get a Drupal path
#2046661: Provide a method which renders a path to a certain route with the current request object

Solution:

Once we have a way to readily render URL from route + parameters, we need to change getPath() to a different method that returns the route and parameters and use the new rendering function.

We may want to write a replacement for theme_menu_local_task, or modify it to take the route + params.

Remaining tasks

User interface changes

API changes

Blocking

Might be blocking getting config_translation into core. See: #2044737: Provide local tasks as plugins

#1984702-59: Convert menu.module's page callbacks to Controllers
#1741498-206: Add a responsive preview bar to Drupal core
#2057155: Add a generateFromRoute() method on the url generator to accept options like url()

Comment	File	Size	Author
#14	local_task-2045267-14.patch	11.41 KB	dawehner
#14
#14	interdiff.txt	1.37 KB	dawehner
#8	local_task-2045267-8.patch	11.36 KB	dawehner
#8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pwolanin CreditAttribution: pwolanin commented 19 July 2013 at 15:35

I think this may also be blocked on #2031353: URLgenerator broken for Drupal installed in a subdirectory - doesn't have a way to get a Drupal path at the moment.

Comment #2

dawehner

German

CreditAttribution: dawehner commented 20 July 2013 at 21:47

That is blocked based upon #2046661: Provide a method which renders a path to a certain route with the current request object now. (the method we will be able to use)

Comment #3

pwolanin CreditAttribution: pwolanin commented 22 July 2013 at 01:11

Title:

LocalTaskBase have to work with routes containing parameters

» LocalTaskBase has to work with routes containing parameters

I'm not sure we actually need that other fix - I'd rather move away as fast as possible from using system paths to route name + params for rendering links.

Comment #4

dawehner

German

CreditAttribution: dawehner commented 22 July 2013 at 18:21

I kind of agree. Just linking the other issue we want to use: #2047619: Add a link generator service for route-based links

Comment #5

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 24 July 2013 at 02:06

My interdiff in #1984702-59: Convert menu.module's page callbacks to Controllers tried to solve this with the least amount of changes.

Comment #6

jessebeach CreditAttribution: jessebeach commented 24 July 2013 at 02:07

Just ran smack into this while updating local tasks to plugins for #1741498-206: Add a responsive preview bar to Drupal core.

Comment #6.0

jessebeach CreditAttribution: jessebeach commented 24 July 2013 at 02:07

Issue summary:

View changes

update summary to add https://drupal.org/node/2046737

Comment #6.1

pwolanin CreditAttribution: pwolanin commented 24 July 2013 at 02:07

Issue summary:

View changes

Updated issue summary.

Comment #7

pwolanin CreditAttribution: pwolanin commented 24 July 2013 at 02:35

Title:

LocalTaskBase has to work with routes containing parameters

» LocalTaskBase and LocalTaskInterface has to work with routes containing parameters

We'll probably need to adjust the interface a bit

Comment #7.0

pwolanin CreditAttribution: pwolanin commented 24 July 2013 at 02:35

Issue summary:

View changes

Updated issue summary.

Comment #8

dawehner

German

CreditAttribution: dawehner commented 1 August 2013 at 17:51

Status:

Active

» Needs review

File	Size
local_task-2045267-8.patch	11.36 KB

As suggested by peter, this adds a new method on the local task, which uses the idea of #2046661: Provide a method which renders a path to a certain route with the current request object
to dynamically pull information from the request object.

I am actually not sure what to do, when the parameter is missing. It seems to be that the local task is simply not shown.

Comment #9

dawehner

German

CreditAttribution: dawehner commented 12 August 2013 at 22:29

Issue tags:

-Local Tasks, -MenuSystemRevamp

#8: local_task-2045267-8.patch queued for re-testing.

Comment #10

12 August 2013 at 23:48

Status:

Needs review

» Needs work

The last submitted patch, local_task-2045267-8.patch, failed testing.

Comment #11

dawehner

German

CreditAttribution: dawehner commented 13 August 2013 at 19:15

Status:

Needs work

» Needs review

#8: local_task-2045267-8.patch queued for re-testing.

Comment #12

dawehner

German

CreditAttribution: dawehner commented 15 August 2013 at 06:32

Issue tags:

+Local Tasks, +MenuSystemRevamp

#8: local_task-2045267-8.patch queued for re-testing.

Comment #13

damiankloip CreditAttribution: damiankloip commented 15 August 2013 at 15:44

+++ b/core/lib/Drupal/Core/Menu/LocalTaskBase.php
@@ -89,13 +103,42 @@ public function getTitle() {
+   * Get the parameters for a route by a given request.

Gets

+++ b/core/lib/Drupal/Core/Menu/LocalTaskBase.php
@@ -89,13 +103,42 @@ public function getTitle() {
+    $compiled = $this->routeProvider->getRouteByName($name)->compile();
+    $variables = $compiled->getPathVariables();

One line? or to much chaining? :)

+++ b/core/lib/Drupal/Core/Menu/LocalTaskBase.php
@@ -89,13 +103,42 @@ public function getTitle() {
+      if ($request->attributes->get('_raw_variables') && $value = $request->attributes->get('_raw_variables')->get($variable)) {

I like to wrap ($value == ...). I find it easier to read, but that's just preference.

Can't we also just get the _raw_variables outside of the foreach?

+++ b/core/lib/Drupal/Core/Menu/LocalTaskBase.php
@@ -89,13 +103,42 @@ public function getTitle() {
+        // @todo throw exception?

Good question, it seems like an exception might be a bit too strong though?

File	Size
interdiff.txt	1.37 KB
local_task-2045267-14.patch	11.41 KB

Comment #15

pwolanin CreditAttribution: pwolanin commented 18 August 2013 at 01:35

I'm still wondering if there is any point until this gets in?

#2057155: Add a generateFromRoute() method on the url generator to accept options like url()

Comment #16

dawehner

German

CreditAttribution: dawehner commented 18 August 2013 at 11:30

For sure! The complex part of this patch is not to call a single method on the url generator but to pull the needed properties and prepare the call.
This patch does not use $options, so why would we even have to switch to generateFromRoute?

But yes, we need to get the other one in sooner than later.