Problem/Motivation

Since the introduction of RouteMatch, we're trying to remove direct usage of $request->attributes.
Local tasks and actions use it for getOptions(), but could just use RouteMatch instead

Proposed resolution

Switch from request to route match

Remaining tasks

User interface changes

API changes

LocalActionInterface::getOptions(), LocalActionInterface::getRouteParameters(), LocalTaskInterface::getOptions(), and LocalTaskInterface::getRouteParameters() will receive RouteMatch instead of Request.

CommentFileSizeAuthor
#18 2294157-18.patch32.01 KBdawehner
#18 interdiff.txt1.51 KBdawehner
#16 2294157-16.patch31.78 KBdawehner
#10 local-route-match-2294157-10.patch28.2 KBtim.plunkett
#3 local-route-match-2294157-3.patch19.73 KBtim.plunkett
#1 local-route-match-2294157-1.patch19.36 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
Issue tags: +API change
StatusFileSize
new local-route-match-2294157-1.patch19.36 KB

Status: Needs review » Needs work

The last submitted patch, 1: local-route-match-2294157-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new local-route-match-2294157-3.patch19.73 KB

Forgot to adjust core.services.yml for action manager.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
@@ -19,18 +18,14 @@ class BlockContentAddLocalAction extends LocalActionDefault {
-    if ($request->attributes->has('theme')) {
-      $options['query']['theme'] = $request->attributes->get('theme');
+    if ($theme = $route_match->getParameter('theme')) {
+      $options['query']['theme'] = $theme;
...
-    if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'block_content.list') {
+    if ($route_match->getRouteName() == 'block_content.list') {
       $options['query']['destination'] = 'admin/structure/block/block-content';
     }

These are the *actual usages* of Request that are now RouteMatch.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major
Issue tags: +beta target
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +RouteMatch
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/lib/Drupal/Core/Menu/LocalActionInterface.php
@@ -45,15 +46,15 @@ public function getWeight();
+   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
+   *   The current route match.

    Did we actually ever considered to move the routeMatch interface into Drupal/Component? This could be really useful for other code as well and allows people to write generic libraries without pulling in all of drupal. 

  2. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
@@ -182,6 +193,7 @@ public function getActionsForRoute($route_appears) {
     $links = array();
     $request = $this->requestStack->getCurrentRequest();
+    /** @var $plugin \Drupal\Core\Menu\LocalActionInterface */

    It is a bit odd that we need both the request and the route match object. It could be possible to construct another request object based upon a routeMatch, which indeed could be really convenient, because the route match just contains high-level results.

effulgentsia’s picture

effulgentsia commented
Title: Switch LocalActionInterface::getOptions() and LocalTaskInterface::getOptions() to use RouteMatch » Switch getOptions() and getRouteParameters() within LocalActionInterface and LocalTaskInterface to use RouteMatch
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Related issues: +#2296205: Create a \Drupal\Component\Routing\ namespace and move Drupal-independent routing classes into it

Re #7.1: good idea: #2296205: Create a \Drupal\Component\Routing\ namespace and move Drupal-independent routing classes into it.

Re #7.2: yeah, that seems wrong. We should make getRouteParameters() and getOptions() have the same signature, so needs work on that. Also, in #2292217: ThemeNegotiator::determineActiveTheme() should not require a RouteMatch to be passed in, the RTBC patch there switched to removing $route_match from a passed-in parameter to a CI dependency. Should we consider the same approach here?

dawehner’s picture

dawehner
Primary language German
commented

@effulgentsia
Well, I made a comment on that issue that I think it not in doens't make sense.
Also here, if you provide custom plugins you most often will do things dynamically, so it seems wrong
that you have to pass it along via CI.

Personally my mind modell is the same as the account on the various access(AccountInterface $accountb) methods.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new local-route-match-2294157-10.patch28.2 KB

Didn't switch to CI, because that's still under debate in the other issue.
Fixed getRouteParameters().

dawehner’s picture

dawehner
Primary language German
commented

I like it just like it is. So how do we resolve the two situations?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
@@ -99,7 +100,7 @@ public function getRouteParameters(Request $request) {
-    $raw_variables = $request->attributes->get('_raw_variables');
+    $raw_variables = $route_match->getRawParameters();

+++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
@@ -19,18 +18,14 @@ class BlockContentAddLocalAction extends LocalActionDefault {
-    if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'block_content.list') {
+    if ($route_match->getRouteName() == 'block_content.list') {

I would consider both of these improvements.

anavarre queued 10: local-route-match-2294157-10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: local-route-match-2294157-10.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Assigned: tim.plunkett » Unassigned
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2294157-16.patch31.78 KB

Rerolled the approach with the passed route match object.

Status: Needs review » Needs work

The last submitted patch, 16: 2294157-16.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.51 KB
new 2294157-18.patch32.01 KB

That was probably easy.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical

This is now blocking #2281619: Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead so bumping.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

It's ready.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looks great. We can nearly remove the request stack from \Drupal\Core\Menu\LocalActionManager but still one usage there.

Committed/pushed to 8.0.x, thanks!

  • catch committed c422305 on 8.0.x 
    Issue #2294157 by tim.plunkett, dawehner: Switch getOptions() and...

Status: Fixed » Closed (fixed)

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