Reunite the router: One router to rule them all

Wow.

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -39,14 +38,14 @@ class AccessAwareRouter implements AccessAwareRouterInterface {
-   * @param \Symfony\Cmf\Component\Routing\ChainRouter $chain_router
+   * @param \Drupal\Core\Routing\DynamicRouter $chain_router
...
-  public function __construct(ChainRouter $chain_router, AccessManagerInterface $access_manager, AccountInterface $account) {
+  public function __construct(DynamicRouter $chain_router, AccessManagerInterface $access_manager, AccountInterface $account) {

Can we switch to an interface here?

1. +++ b/core/core.services.yml
   @@ -795,16 +795,6 @@ services:
   -      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   
   @@ -838,16 +828,14 @@ services:
          - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
   ...
   +      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   

   As long as route filters and enhancers run, I don't think we'll have any regressions. I manually tests page_manager/panels and ran the test suites with this patch applied, everything worked as expected.

2. +++ b/core/core.services.yml
   @@ -838,16 +828,14 @@ services:
   +    class: \Drupal\Core\Routing\DynamicRouter
   

   I used to have a problem using a leading \ for this, great that it's not broken anymore!

3. +++ b/core/lib/Drupal/Core/Routing/DynamicRouter.php
   @@ -0,0 +1,229 @@
   +class DynamicRouter extends UrlMatcher implements RequestMatcherInterface {
   

   I think naming this DynamicRouter but not extending \Symfony\Cmf\Component\Routing\DynamicRouter is confusing.

   Why not just call it Router?

As a subsystem maintainer I'm fine with this, but I would feel better if Crell signed off on this.

Merge the routers into one basically. This removes a lot of indirection in the process and keeps the code flow much easier.

YES PLEASE!

Anybody who has debugged the current routing system is still deeply scarred by that experience.

Also: the IS says This hopefully flattens down the routers from 5 to 3., but then there are only two listed in the "after" step.

+1 on the general approach.

1. +++ b/core/core.services.yml
   @@ -795,16 +795,6 @@ services:
   -  router.matcher.final_matcher:
   -    class: Drupal\Core\Routing\UrlMatcher
   -    arguments: ['@path.current']
   -  router.matcher:
   -    class: Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
   -    arguments: ['@router.route_provider']
   -    calls:
   -      - [setFinalMatcher, ['@router.matcher.final_matcher']]
   -    tags:
   -      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   

   we cannot remove services during the D8 release cycle, so the removal here will have to wait until D9. Or do we consider those services internal? Is a change notice enough to let contrib maintainers know that those services are gone now?

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -114,14 +120,14 @@ protected function checkAccess(Request $request) {
      public function getRouteCollection() {
   -    return $this->chainRouter->getRouteCollection();
   +    throw new \Exception('not supported');
      }
   

   this exception should have a better message.

   And the exception type should probably be \BadMethodCallException instead of the useless generic type?

   Something like throw new \BadMethodCallException(sprintf("'%s' is not supported by Drupal 8.", __FUNCTION__));

   Also elsewhere.

3. +++ b/core/lib/Drupal/Core/Routing/DynamicRouter.php
   @@ -0,0 +1,229 @@
   +/**
   + * Dynamic router handles the central piece of logic of routing in Drupal.
   + *
   

   we should probably document where the code in this class comes from. Symfony CMF somewhere?

4. +++ b/core/lib/Drupal/Core/Routing/DynamicRouter.php
   @@ -0,0 +1,229 @@
   +class DynamicRouter extends UrlMatcher implements RequestMatcherInterface {
   

   big +1 to rename this class from "DynamicRouter" to just "Router". The "dynamic" is meaningless.

5. +++ b/core/lib/Drupal/Core/Routing/DynamicRouter.php
   @@ -0,0 +1,229 @@
   +
   +  public function addRouteFilter(BaseRouteFilterInterface $routeFilter, $priority) {
   

   doc blocks are incomplete and missing in this patch, so make sure to check those when we have signoff from Crell.

6. +++ b/core/lib/Drupal/Core/Routing/DynamicRouter.php
   @@ -0,0 +1,229 @@
   +   * @return array
   +   *   The request attributes after applying the enhancers.
   +   */
   +  protected function applyRouteEnhancers($defaults, Request $request) {
   

   data type, what is in the array? More arrays? Or flat strings?

I'm on board with the general direction; I'll review the code after the next patch as it looks like there's a number of changes to be made for the reviews above.

I'd suggest DrupalRouter for the class name, as it's the Drupal implementation of the RouterInterface.

As far as the API breakage, this approach would eliminate a number of obscure ways to fiddle with the routing system, like multiple stages of chained routers. However, in practice I don't think that would work anyway given how much is built on the router and I'd argue we never actually said that was part of the API. Rather, the API of the routing system (as I proposed at DrupalCon Austin in my core conversation on @internal APIs) is:

1) The *.routing.yml file and its PHP equivalents.
2) RouteFilters
3) RouteEnhancers

So as long as we don't break the expected behavior of those steps and the interfaces on the latter two, I think we're fine API-wise.

I think we'd consider the services removed internal, but if we wanted to we could deprecate them in 8.3.x and remove them in 8.4.x (nothing stops us doing that if we think they're @internal anyway, it's just being extra nice if someone was somehow relying on them).

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -56,23 +57,28 @@ public function __construct(ChainRouter $chain_router, AccessManagerInterface $a
+    // Ensure to call every other function to the inner router.
+    if (method_exists($this->router, $name)) {
+      return call_user_func_array([$this->router, $name], $arguments);
+    }

I'm a little unsure why we need to have __call here instead of directly delegated the expected methods?

1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -114,14 +121,14 @@ protected function checkAccess(Request $request) {
      public function getRouteCollection() {
   -    return $this->chainRouter->getRouteCollection();
   +    throw new \BadMethodCallException(sprintf("'%s' is not supported by Drupal 8.", __FUNCTION__));
      }
   

   What's the reasoning here? We're taking away a method, but not fixing the interface? That seems problematic. What was this method doing before, and what would be the equivalent operation now?

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -114,14 +121,14 @@ protected function checkAccess(Request $request) {
      public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH) {
   -    return $this->chainRouter->generate($name, $parameters, $referenceType);
   +    throw new \BadMethodCallException(sprintf("'%s' is not supported by Drupal 8.", __FUNCTION__));
      }
   

   At its base, the RouterInterface is an aggregate of a matcher and a generator. We shouldn't be breaking that contract. Inject the generator and let it delegate here.

Otherwise this looks pretty good overall.

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -121,14 +119,18 @@ protected function checkAccess(Request $request) {
+    if ($this->router instanceof RouterInterface) {

This check is too broad. Strictly speaking we only need check for GeneratorInterface here. (RouterInterface extends it.)

We should still get a framework manager to verify that they're OK with the narrow definition of "API break" we're assuming here, so leaving the tag in place.

1. +++ b/core/core.services.yml
   @@ -798,6 +798,7 @@ services:
   +    deprecated: The “%service_id%” service is deprecated. You should use the 'router.no_access_checks' service instead.
   

   This is fascinating. I didn't know this is how it worked. It's great to have this example.

   Should we have

   # @todo Remove in 8.4.0
   

   ?

2. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + * Central router implementation in Drupal.
   

   "Central" seems a strange adjective here?

3. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + * A router determines, for an incoming process, the active controller, which is
   

   s/process/request/

4. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + * a callable that creates the output of the page.
   

   s/the output of the page/a response/

5. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + * It consists of several steps, of which each are explained in more details
   + * below:
   + *   - Get a collection of routes which potentially match the current request:
   + *     This is done by the route provider ::getInitialRouteCollection()
   + *   - Filter the collection down further more. For example this filters out
   + *     routes applying to other formats: ::applyRouteFilters()
   + *   - Find the best matching route out of the remaining one, by applying a
   + *     regex: ::matchCollection()
   + *   - Enhance the list of route attributes, for example loading entity objects
   + *     : ::applyRouteEnhancers().
   + *
   

   If this is a sequence, shouldn't we number them instead?

   s/one/ones/

6. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + * This implementation consists ideas of the following routers:
   

   "consists ideas" needs better wording.

Not RTBCing without a framework manager review, but I'm happy here. Thanks all!

I like this issue a lot. Ambivalent about the service deprecations and when they could be removed, probably needs a discussion in the @internal issue like many other things.

Adding one class to drop two confusing services is a good trade-off.

@catch: we are dropping 3 confusing services :)

Looks good, some minor nitpicks:

1. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   + *   4. Enhance the list of route attributes, for example loading entity objects
   + *     : ::applyRouteEnhancers().
   

   stray colon here? "objects" from the first line should be on the second line.

2. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   +  /**
   +   * @var \Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface[]
   +   */
   +  protected $enhancers = [];
   

   Description missing.

3. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   +  /**
   +   * An array of RouteFilterInterface objects.
   +   *
   +   * @var \Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface[]
   +   */
   +  protected $filters = [];
   

   Yes, i can see that this is an array. Is it also a cache same as $sortedEnhancers?

4. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,296 @@
   +   * @param int $priority
   +   *   (optional) The priority of the enhancer. Higher number enhancers will be
   +   *   used first.
   +   *
   +   * @return $this
   +   */
   +  public function addRouteEnhancer(BaseRouteEnhancerInterface $route_enhancer, $priority) {
   

   priority is not optional here, it is a required parameter. Should we make it optional by using a default value in the method signature?

catch is a framework maintainer that signed off this approach, removing tag. For the @internal services discussion I think it is a good first step to just deprecate those services before removing them. We can always remove them later if we want to.

We should probably have a change record for this.

I tweaked the change record slightly. I think our work here is done.

RTBC+1, I updated the change record with the list of deprecated services also.

Committed 21a86f3 and pushed to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
index 76a9477..5c3d931 100644
--- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -7,7 +7,6 @@
 use Drupal\Core\Session\AccountInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
-use Symfony\Component\Routing\Generator\Dumper\GeneratorDumperInterface;
 use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
 use Symfony\Component\Routing\RequestContext as SymfonyRequestContext;
 use Symfony\Component\Routing\RequestContextAwareInterface;
diff --git a/core/lib/Drupal/Core/Routing/Router.php b/core/lib/Drupal/Core/Routing/Router.php
index 58f6fb3..5fcf5ee 100644
--- a/core/lib/Drupal/Core/Routing/Router.php
+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -23,16 +23,19 @@
  *
  * It consists of several steps, of which each are explained in more details
  * below:
- *   1. Get a collection of routes which potentially match the current request:
- *     This is done by the route provider, see ::getInitialRouteCollection().
- *   2. Filter the collection down further more. For example this filters out
- *     routes applying to other formats: See ::applyRouteFilters()
- *   3. Find the best matching route out of the remaining ones, by applying a
- *     regex: See ::matchCollection().
- *   4. Enhance the list of route attributes, for example loading entity objects
- *     See ::applyRouteEnhancers().
+ * 1. Get a collection of routes which potentially match the current request.
+ *    This is done by the route provider. See ::getInitialRouteCollection().
+ * 2. Filter the collection down further more. For example this filters out
+ *    routes applying to other formats: See ::applyRouteFilters()
+ * 3. Find the best matching route out of the remaining ones, by applying a
+ *    regex. See ::matchCollection().
+ * 4. Enhance the list of route attributes, for example loading entity objects.
+ *    See ::applyRouteEnhancers().
  *
  * 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
@@ -170,7 +173,7 @@ protected function getInitialRouteCollection(Request $request) {
   }
 
   /**
-   * Apply the route enhancers to the defaults, according to priorities
+   * Apply the route enhancers to the defaults, according to priorities.
    *
    * @param array $defaults
    *   The defaults coming from the final matched route.

Some docs fixes on commit - reviewed in real life with @dawehner.

This breaks my install when opcache is turned off.

Fatal error: Cannot use Symfony\Component\Routing\Generator\UrlGeneratorInterface as UrlGeneratorInterface because the name is already in use in /Users/tim.plunkett/www/d8/core/lib/Drupal/Core/Routing/Router.php on line 13

\Drupal\Core\Routing\UrlGeneratorInterface exists in the same namespace as Router, so the use statement of Symfony\Component\Routing\Generator\UrlGeneratorInterface is conflicting.

This can be fixed by aliasing the import like this:

diff --git a/core/lib/Drupal/Core/Routing/Router.php b/core/lib/Drupal/Core/Routing/Router.php
index 5fcf5ee..80a31f7 100644
--- a/core/lib/Drupal/Core/Routing/Router.php
+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -10,7 +10,7 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Exception\MethodNotAllowedException;
 use Symfony\Component\Routing\Exception\ResourceNotFoundException;
-use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface as BaseUrlGeneratorInterface;
 use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
 use Symfony\Component\Routing\RouteCollection;
 use Symfony\Component\Routing\RouterInterface;
@@ -95,7 +95,7 @@ class Router extends UrlMatcher implements RequestMatcherInterface, RouterInterf
    * @param \Symfony\Component\Routing\Generator\UrlGeneratorInterface $url_generator
    *   The URL generator.
    */
-  public function __construct(BaseRouteProviderInterface $route_provider, CurrentPathStack $current_path, UrlGeneratorInterface $url_generator) {
+  public function __construct(BaseRouteProviderInterface $route_provider, CurrentPathStack $current_path, BaseUrlGeneratorInterface $url_generator) {
     parent::__construct($current_path);
     $this->routeProvider = $route_provider;
     $this->urlGenerator = $url_generator;

But this should be reverted first. Also maybe we should have test runs once in a while without opcache on?

git revert 21a86f3a465bba771556251729e809e61369edc3

This was a known bug in PHP, https://bugs.php.net/bug.php?id=66773
It was just fixed in PHP 19 days ago, so it might be a while before we see the fix on testbot :)

By fix, I mean all of our tests would begin failing due to invalid imports.

#2446847: [Policy] Create test runner groups for different issue stages
#2446719: Need test runners without bytecode caching

Reverted - I think these issues are rare enough just to do the revert as asked... it is not that painful.

New patch. Interdiff is against #38. This just rolls in the changes from #42 and #44.

Committed 0e71ca6 and pushed to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Routing/Router.php b/core/lib/Drupal/Core/Routing/Router.php
index f1e280b..80a31f7 100644
--- a/core/lib/Drupal/Core/Routing/Router.php
+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -39,6 +39,7 @@
  *
  * @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 {
 

Didn'r mean to remove that in my original commit if I did... doesn't look I did.