Replace \Drupal\Core\Routing\AccessAwareRouter::__call() with the real methods

We added __call simply to fetch potential calls we haven't thought about. I think we should totally add function if we forgot about any of them.

Also combine \Drupal\Core\Routing\Router with \Drupal\Core\Routing\UrlMatcher to reduce confusion

That's something which is its own issue totally, IMHO :)

It fullfills me with joy to do that

I guess this step is enough for now. To be honest I would love to tag the router class as internal. It is not in anyone's business really to manipulate the router. There are too many implicit assumptions made all over the place.

yes

Is there really that much value in this unit test?

Do we know that the route attributes start out empty?

Do we care if they are not empty in the beginning?

+++ /dev/null
@@ -1,146 +0,0 @@
-    $parameters = $this->router->matchRequest($request);
-    $request->attributes->add($parameters);
...
-    return $request->attributes->all();

+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -151,7 +186,15 @@ public function matchRequest(Request $request) {
+      $parameters = $this->applyRouteEnhancers($ret, $request);
+      $request->attributes->add($parameters);
...
+      return $request->attributes->all();

Isn't still the same code running at the end?

Oh I see, mhh, do you think we should return all attributes?

Cool, I like this issue :)

The new methods on the Router class should have unit tests. We should port over that from the removed AccessAwareRouterTest.

Here are the previous unit tests converted over ...

Oh I also kept a BC layer around for AccessRouter, you never know ...

I forgot to remove the AccessAwareRouterTest ...

1. +++ b/core/core.services.yml
   @@ -864,7 +869,7 @@ services:
      router.no_access_checks:
        class: \Drupal\Core\Routing\Router
   -    arguments: ['@router.route_provider', '@path.current', '@url_generator']
   +    arguments: ['@router.route_provider', '@path.current', '@url_generator', '@access_manager', '@current_user', FALSE]
        tags:
          - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
          - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   

   Can't we simplify this even further, to inherit as much as possible from router? That'd make the relation clearer.

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -4,143 +4,28 @@
   + * @deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.0.
   

   8.5.x by now.

3. +++ b/core/tests/Drupal/Tests/Core/Routing/RouterTest.php
   @@ -0,0 +1,159 @@
   +  ¶
   

   Stupid whitespace nit here.

Addressed all my feedback. Thoughts?

Can't we simplify this even further, to inherit as much as possible from router? That'd make the relation clearer.

Oh wow, I did not knew this is actually possible.

I think this should be major given what #2883680-69: Force all route filters and route enhancers to be non-lazy argued abotu.

So you liked #24.1? If so, I'd be happy to RTBC. Or perhaps we need @tim.plunkett to RTBC?

I guess its another case where we need a subsystem maintainer review?

Yep, I think so!

This patch looks great, and is a nice cleanup.

1. +++ b/core/core.services.yml
   @@ -864,12 +869,13 @@ services:
   +    parent: router
   +    arguments:
   +      index_5: FALSE
   

   Wow, that is interesting. The brittleness of index_5 is worrisome, I hope we don't regret this later.

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -4,143 +4,28 @@
   -    $request->attributes->add($parameters);
   -    $this->checkAccess($request);
   -    // We can not return $parameters because the access check can change the
   -    // request attributes.
   -    return $request->attributes->all();
   
   +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -151,7 +186,15 @@ public function matchRequest(Request $request) {
   +      $request->attributes->add($parameters);
   ...
   +        $this->checkAccess($request);
   ...
   +      return $request->attributes->all();
   

   Can we keep this comment? It is odd that we don't return $parameters, so we should document why we alter the request at all.

3. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -151,7 +186,15 @@ public function matchRequest(Request $request) {
   +      $request->attributes->add($parameters);
   +
   +      // Checking access here. ::checkAccess might throw an exception, when
   +      // access is not granted.
   +      if ($this->checkAccess) {
   +        $this->checkAccess($request);
   +      }
   +      return $request->attributes->all();
   

   In theory we should put all of this within the if(), and return $parameters when $this->checkAccess is false.
   Though WTF I'd really like to see access checks stop expecting that they can manipulate the request directly...

4. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -374,6 +417,29 @@ protected function sortFilters() {
   +   * Apply access check service to the route and parameters in the request.
   

   nit: s/Apply/Applies

Very close to RTBC!

Thank you tim!

Wow, that is interesting. The brittleness of index_5 is worrisome, I hope we don't regret this later.

I hope we never swap the order of parameters ... given that this would fuck up other routers, of there are probably zero.

Can we keep this comment? It is odd that we don't return $parameters, so we should document why we alter the request at all.

Nice observation

In theory we should put all of this within the if(), and return $parameters when $this->checkAccess is false.
Though WTF I'd really like to see access checks stop expecting that they can manipulate the request directly...

If things aren't changed it doesn't make a difference anyway, right?

Surely no one would ever change the arguments to the router... #2883680: Force all route filters and route enhancers to be non-lazy
😆

You are right, it doesn't matter, since the calling code (\Symfony\Component\HttpKernel\EventListener\RouterListener) immediately calls

$request->attributes->add()

on this return value.

👏

🎉

1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -4,143 +4,28 @@
    /**
   - * A router class for Drupal with access check and upcasting.
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0.
   + * Use Drupal\Core\Routing\Router with $check_access in the constructor instead.
     */
   

   This should be linked to the CR no? Which I think we need.

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -4,143 +4,28 @@
   +  public function __construct(RouteProviderInterface $route_provider, CurrentPathStack $current_path, UrlGeneratorInterface $url_generator, AccessManagerInterface $access_manager, AccountInterface $account, $check_access = TRUE) {
   +    parent::__construct($route_provider, $current_path, $url_generator, $access_manager, $account, $check_access);
   

   Trigger a deprecation notice in the constructor?

Sure, let's do that.

#35 correctly fixes the remarks in #34, back to RTBC.

Updating the issue summary to reflect the final patch.

I wonder what we should be doing with \Drupal\Core\Routing\AccessAwareRouterInterface? Should that be deprecated too?

Also do we have a hard API break if the router service no longer implements \Drupal\Core\Routing\AccessAwareRouterInterface?

So part of #39 is incorrect. The patch changes \Drupal\Core\Routing\Router to implement AccessAwareRouterInterface. But this brings another question - what does it mean that a router can have the AccessAwareRouterInterface but not check access?

Mhh, this is an interesting question.

We could keep the interface around as some form of documentation.
Instead of the making it possible to enable the access checking via constructor argument, we could continue having the AccessAwareRouter around, but now it extends the base router and sets the property on the object to be TRUE by default.

This way we just change the internal details of the code, but don't touch anything visible from the outside.

What do people think about this approach?

Here is the implementation of #41

If we go down this path, we should follow it to the logical conclusion: move all thought of access from Router back to AccessAwareRouter.

This undoes some of the original patch, but retains the key change to AccessAwareRouter: subclass over decorator.

Then this really boils down to "Drupal loves inheritance, or, decorators in PHP are not ideal".
And to be honest, none of this yet addresses the first line of the IS, about UrlMatcher::finalMatch no longer being called.

To actually accomplish that, we'd need this change too

diff --git a/core/lib/Drupal/Core/Routing/Router.php b/core/lib/Drupal/Core/Routing/Router.php
index e949c5efb8..afb50c9c50 100644
--- a/core/lib/Drupal/Core/Routing/Router.php
+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -6,14 +6,17 @@
 use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface as BaseRouteEnhancerInterface;
 use Symfony\Cmf\Component\Routing\LazyRouteCollection;
 use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface as BaseRouteFilterInterface;
+use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Cmf\Component\Routing\RouteProviderInterface as BaseRouteProviderInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Exception\MethodNotAllowedException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface as BaseUrlGeneratorInterface;
 use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
+use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
 use Symfony\Component\Routing\RouterInterface;
+use Symfony\Component\Routing\Matcher\UrlMatcher as BaseUrlMatcher;
 
 /**
  * Router implementation in Drupal.
@@ -34,14 +37,19 @@
  *
  * This implementation uses ideas of the following routers:
  * - \Symfony\Cmf\Component\Routing\DynamicRouter
- * - \Drupal\Core\Routing\UrlMatcher
  * - \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
  *
  * @see \Symfony\Cmf\Component\Routing\DynamicRouter
- * @see \Drupal\Core\Routing\UrlMatcher
  * @see \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
  */
-class Router extends UrlMatcher implements RequestMatcherInterface, RouterInterface {
+class Router extends BaseUrlMatcher implements RequestMatcherInterface, RouterInterface {
+
+  /**
+   * The current path.
+   *
+   * @var \Drupal\Core\Path\CurrentPathStack
+   */
+  protected $currentPath;
 
   /**
    * The route provider responsible for the first-pass match.
@@ -96,7 +104,7 @@ class Router extends UrlMatcher implements RequestMatcherInterface, RouterInterf
    *   The URL generator.
    */
   public function __construct(BaseRouteProviderInterface $route_provider, CurrentPathStack $current_path, BaseUrlGeneratorInterface $url_generator) {
-    parent::__construct($current_path);
+    $this->currentPath = $current_path;
     $this->routeProvider = $route_provider;
     $this->urlGenerator = $url_generator;
   }
@@ -249,6 +257,19 @@ protected function doMatchCollection($pathinfo, RouteCollection $routes, $case_s
     }
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function getAttributes(Route $route, $name, array $attributes) {
+    if ($route instanceof RouteObjectInterface && is_string($route->getRouteKey())) {
+      $name = $route->getRouteKey();
+    }
+    $attributes[RouteObjectInterface::ROUTE_NAME] = $name;
+    $attributes[RouteObjectInterface::ROUTE_OBJECT] = $route;
+
+    return $this->mergeDefaults($attributes, $route->getDefaults());
+  }
+
   /**
    * Returns a collection of potential matching routes for a request.
    *
diff --git a/core/lib/Drupal/Core/Routing/UrlMatcher.php b/core/lib/Drupal/Core/Routing/UrlMatcher.php
index 9056903368..4ec8706947 100644
--- a/core/lib/Drupal/Core/Routing/UrlMatcher.php
+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
@@ -2,6 +2,8 @@
 
 namespace Drupal\Core\Routing;
 
+@trigger_error('\Drupal\Core\Routing\UrlMatcher is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Core\Routing\Router', E_USER_DEPRECATED);
+
 use Drupal\Core\Path\CurrentPathStack;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\RouteCollection;

which starts to overlap with #2917331: Decouple from Symfony CMF

Mhh, yeah its a good question whether this makes things still easier to understand or whether its basically refactoring on the spot.

So how should we proceed here?

Rerolled the patch #44

Re-roll against the latest 9.2.x.

NW for failed pre checks

Fixed the error in #59. please review.

Now something is wrong again

re-roll for 10.1 and fix tests with $this->route->setOption('compiler_class', RouteCompiler::class);

Additionally to #45 there's issue #3238199: \Drupal\Core\Routing\UrlMatcher is dead code

The issue summary talks about deprecating AccessAwareRouter but I don't see a deprecated call?

Here's reverted removal with deprecation, could use test but methods now should be defined

Maybe it should be no-op instead of attempt to call missing method?

It needs work for constructor changes (BC) and not sure we need to keep _call()

+++ b/core/core.services.yml
@@ -1015,7 +1015,11 @@ services:
+      - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
+      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }

patch replaced as deprecated https://www.drupal.org/node/2894934