Problem/Motivation

We have Symfony CMF as a dependency, but we only really use four things from it:

RouteEnhancerInterface
RouteFilterInterface

Also the ROUTE_NAME and ROUTE_OBJECT constants from RouteObjectInterface

And the LazyRouteCollection class - which has a handful of very short methods.

Proposed resolution

Copy LazyRouteCollection to Drupal\Core\Routing and Drupalize it - this is the only logic from Symfony CMF we actually use at this point except maybe one or two small methods, and it's a very simple class.

Create our own version of RouteObjectInterface which just contains the ROUTE_NAME, ROUTE_OBJECT and CONTROLLER constants, then convert code to use those.

Where our interfaces extend from Symfony CMF interfaces, add the inherited methods to our own interfaces.

Remove all usages of PagedRouteCollection, deprecated the getRoutesPaged() method.

After this we'll have no direct dependencies on Symfony CMF routing, and could remove the dependency in 10.0.0

Two follow-ups are opened, for further deprecations and API tightening that this patch enables:

#3151017: Deprecate UrlGenerator::supports() and UrlGenerator::getRouteDebugMessage() and #3151019: Only allow route names, deprecate support for route objects in UrlGenerator methods

Remaining tasks

User interface changes

API changes

Drupal\Core\Routing\RouteObjectInterface has been added to provide two constants previously provided by Symfony\Cmf\Component\Routing\RouteObjectInterface

Drupal\Core\Routing\UrlGeneratorInterface no longer extends Symfony\Cmf\Component\Routing\VersatileGeneratorInterface.

Data model changes

Release notes snippet

Drupal 10 will no longer depend on symfony/cmf. Code referencing the constants on RouteObjectInterface (including RouteObjectInterface::ROUTE_NAME will need to reference the Drupal core versions, see https://www.drupal.org/node/3151009

CommentFileSizeAuthor
#91 2917331-91.patch62.58 KBcatch
#91 2917331-interdiff-78-91.patch748 bytescatch
#78 interdiff_73-78.txt812 bytesDeepak Goyal
#78 2917331-78.patch62.67 KBDeepak Goyal
#76 interdiff_73-76.txt810 bytesDeepak Goyal
#76 2917331-76.patch62.67 KBDeepak Goyal
#73 interdiff-2917331-70-73.txt721 bytesmartin107
#73 2917331-73.patch62.72 KBmartin107
#70 interdiff_68-70.txt721 bytesravi.shankar
#70 2917331-70.patch62.72 KBravi.shankar
#68 interdiff-67-88.txt4.08 KBmartin107
#68 2917331-68.patch62.72 KBmartin107
#67 interdiff-2917331-64-67.txt3.15 KBmartin107
#67 2917331-67.patch61.25 KBmartin107
#64 2917331-interdiff.txt4.07 KBcatch
#64 2917331-64.patch59.86 KBcatch
#64 2917331-interdiff.txt4.07 KBcatch
#56 2917331-56.patch59.72 KBandypost
#56 interdiff.txt5.57 KBandypost
#55 2917331-54.patch58.68 KBcatch
#54 interdiff-2917331-54.txt2.55 KBcatch
#54 interdiff-2917331-54.txt2.55 KBcatch
#49 interdiff.2917331.47-49.txt4.22 KBlongwave
#49 2917331-49-test-only.patch60.63 KBlongwave
#47 2917331-47.patch56.41 KBcatch
#46 2917331-46.patch56.41 KBcatch
#46 2917331-interdiff.txt2.5 KBcatch
#45 2917331-45.patch55.34 KBcatch
#45 2917331-interdiff.txt705 bytescatch
#42 2917331-41.patch60.31 KBcatch
#37 2917331-37.patch57.77 KBcatch
#37 2917331-interdiff.txt4.17 KBcatch
#36 2917331-35.patch56.87 KBcatch
#31 2917331-31.patch57.28 KBcatch
#30 2917331-29.patch49.56 KBcatch
#29 2917331-29.patch52.29 KBcatch
#27 2917331-27.patch46.73 KBcatch
#27 2917331-interdiff.txt930 bytescatch
#26 2917331-26.patch46.59 KBcatch
#26 2917331-interdiff.txt501 bytescatch
#25 2917331-24.patch46.55 KBcatch
#25 2917331-interdiff.txt2.16 KBcatch
#23 2917331-23.patch44.38 KBcatch
#22 2917331-interdiff.txt1.62 KBcatch
#22 2917331-22.patch43.18 KBcatch
#21 2917331-20.patch40.75 KBcatch
#21 2917331-20-interdiff.txt4.26 KBcatch
#21 2917331-20.patch41.52 KBcatch
#19 2917331-18-19.txt471 bytescatch
#19 2917331-19.patch32.19 KBcatch
#18 2917331-19.patch32.1 KBcatch
#17 2917331-17.patch32.1 KBcatch
#17 2917331-interdiff.txt871 bytescatch
#17 2917331-17.patch32.1 KBcatch
#17 2917331-interdiff.txt871 bytescatch
#17 2917331-interdiff.txt871 bytescatch
#16 2917331-16.patch31.25 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

alexpott’s picture

The other tricky things are like:

  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 }
    deprecated: The "%service_id%" service is deprecated. You should use the 'router.no_access_checks' service instead.

from core.services.yml

We've deprecated it but if we remove Symfony Cmf prior to Drupal 9 we're going to need our version of NestedMatcher - no?

catch’s picture

I'd expect us to actually remove the dependency in 9.0.0 and just leave it deprecated until then.

Wim Leers’s picture

#2883680: Force all route filters and route enhancers to be non-lazy is in. We now have \Drupal\Core\Routing\EnhancerInterface which looks like this:

<?php

namespace Drupal\Core\Routing;

use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;

/**
 * A route enhance service to determine route enhance rules.
 */
interface EnhancerInterface extends RouteEnhancerInterface {

}

And similarly:

<?php

namespace Drupal\Core\Routing;

use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface;

/**
 * A route filter service to filter down the collection of route instances.
 */
interface FilterInterface extends RouteFilterInterface {

}

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

dawehner’s picture

Given we have a solution how to get rid of Route enhancer/filter I am wondering whether we could get rid of route_name/route_object by creating a copy of those files, including the same namespace as before, into Drupal itself. This way we could for most sites throw deprecation errors already.

catch’s picture

catch’s picture

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +Drupal 9
geek-merlin’s picture

Issue summary: View changes

Fixed link.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

catch’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Active » Needs review
FileSize
31.25 KB

Here's an initial patch.

All I've done for now is move the two constants from RouteObjectInterface to an identically named interface in core.

To be honest this doesn't really seem like the right place for those constants, but I didn't see another interface which would be obvious to add them to either yet.

I dropped the other constants from the interface because we never use them in core. Nothing type hints on it, nothing extends it.

Not possible to do a deprecation as such because they're constants.

catch’s picture

Another one:

Drupal\Core\Routing\UrlGeneratorInterface extends Symfony\Cmf\Component\Routing\VersatileGeneratorInterface

VersatileGeneratorInterface provides two methods:

::supports() - we never call this.

::getRouteDebugMessage() - this is only mis-used to handle getting a route name from either a string route name or a route object..

For this I think we should:

1. Just stop implementing the interface
2. In a follow-up: deprecate the ::supports() and :getRouteDebugMessage() methods on UrlGenerator, they can be moved inline or to new protected methods.
3. In a further follow-up: always require a string route name in UrlGenerator and friends, deprecate the route object handling.
4. File a further follow-up to type hint on string where we currently accept route or route name - this would need to be Drupal 11-only.

catch’s picture

Missing a use statement.

catch’s picture

jsonapi uses CONTROLLER_NAME after all, so bringing that back.

The last submitted patch, 18: 2917331-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

Merging three methods into RouteProviderInterface from Cmf's interface of the same name.

Also making it no longer implenent PagedRouteProviderInterface - those methods aren't used in runtime by core.

catch’s picture

Copying LazyRouteCollection to core from CMF. Not much logic in it and as far as I can tell some of the methods are unused, but trying to get back to green for the moment.

catch’s picture

Adding one method from CMF's UrlMatcher to ours and removing the subclass.

Down to only a handful of non-test references now:

lib/Drupal/Core/Routing/RouteProvider.php:use Symfony\Cmf\Component\Routing\PagedRouteCollection;
lib/Drupal/Core/Routing/FilterInterface.php:use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface;
lib/Drupal/Core/Routing/RouteProviderInterface.php: * @see \Symfony\Cmf\Component\Routing
lib/Drupal/Core/Routing/Router.php: * - \Symfony\Cmf\Component\Routing\DynamicRouter
lib/Drupal/Core/Routing/Router.php: * - \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
lib/Drupal/Core/Routing/Router.php: * @see \Symfony\Cmf\Component\Routing\DynamicRouter
lib/Drupal/Core/Routing/Router.php: * @see \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
lib/Drupal/Core/Routing/EnhancerInterface.php:use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;

The last submitted patch, 22: 2917331-22.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

Merging FilterInterface and EnhancerInterface.

catch’s picture

catch’s picture

Even more missing use statements.

Status: Needs review » Needs work

The last submitted patch, 27: 2917331-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

Status: Needs work » Needs review
FileSize
52.29 KB

Completely removing the dependency on PagedRouteCollection. The functionality it provides (chunked loading in RouteProvider::getAllRoutes()) is only ever used in test coverage so we're not gaining anything from it.

Similarly ChainRouter is only used for convenience for a mock in tests, which can equally use our own Router.

Patch now has no references to Symfony/Cmf except autoloading, which we need to keep until we can actually remove the dependency.

@todos: this issue should properly deprecate ::getRoutesPaged() but does not yet.

catch’s picture

Overzealous removal.

catch’s picture

Now deprecating ::getRoutesPaged() and switching the test coverage to a legacy test.

The last submitted patch, 29: 2917331-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

Issue summary: View changes

Updating the issue summary.

Status: Needs review » Needs work

The last submitted patch, 31: 2917331-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

longwave’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
    @@ -2,14 +2,83 @@
    +   * that the DynamicRouter will only call this method once per
    +   * DynamicRouter::getRouteCollection() call.
    

    What is DynamicRouter? I think this is an obsolete Symfony CMF concept?

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -23,9 +23,9 @@ class AccessAwareRouterTest extends UnitTestCase {
    -  protected $chainRouter;
    +  protected $coreRouter;
    

    I don't think this variable name should be changing.

  3. core/tests/Drupal/KernelTests/RouteProvider.php
    18:   * @return \Drupal\Core\Routing\PreloadableRouteProviderInterface|\Symfony\Cmf\Component\Routing\PagedRouteProviderInterface|\Symfony\Component\EventDispatcher\EventSubscriberInterface
    
    core/lib/Drupal/Core/Routing/Router.php
    33: * - \Symfony\Cmf\Component\Routing\DynamicRouter
    35: * - \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
    37: * @see \Symfony\Cmf\Component\Routing\DynamicRouter
    39: * @see \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher
    
    core/lib/Drupal/Core/Routing/RouteProviderInterface.php
    13: * @see \Symfony\Cmf\Component\Routing
    
    core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    128:    $this->chainRouter = $this->getMockBuilder('Symfony\Cmf\Component\Routing\ChainRouter')
    

    Some remaining references to Symfony CMF.

catch’s picture

Status: Needs work » Needs review
FileSize
56.87 KB

Applying the coding standards fixes.

catch’s picture

Thanks for reviewing!

#35.1: yes we should remove the reference.

#35.2 hmm it's not using ChainedRouter any more so it probably should change, we don't have that as an explicit concept anymore. However I was grasping at straws with coreRouter so agreed it is not necessarily better either. Left as-is for now though.

#35.3 removed those.

longwave’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -23,9 +23,9 @@ class AccessAwareRouterTest extends UnitTestCase {
    -  protected $chainRouter;
    +  protected $coreRouter;
    

    This could just be $router, and $router could be renamed $accessAwareRouter?

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -56,7 +56,7 @@ protected function setUp() {
    +    $this->chainRouter = $this->getMockBuilder('Drupal\Core\Routing\Router')
    

    This needs to change to match, and also testCall() still mocks the Symfony CMF objects.

The last submitted patch, 36: 2917331-35.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 37: 2917331-37.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
57.77 KB
catch’s picture

FileSize
60.31 KB

Gah bad patch, interdiff is OK though.

The last submitted patch, , failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 42: 2917331-41.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
705 bytes
55.34 KB

Typo in the expected deprecation method. (edit: interdiff missed the deletion of the extra test class).

Also given the amount of setup involved, reckon it is better to make the existing method on the existing test account for the deprecation rather than copying everything around for what is really a two line change.

catch’s picture

Deprecating ::getRoutesCount() and removing it from the test implementation - completely dead code.

catch’s picture

jibran’s picture

Should we add a test only patch that removes Symfony CMF component and makes sure the patch is still green?

longwave’s picture

Let's try #48.

There is still an @see left in Drupal\Core\Routing\RouteProviderInterface:

/**
 * Extends the router provider interface
 *
 * @see \Symfony\Cmf\Component\Routing
 */
interface RouteProviderInterface {

Status: Needs review » Needs work

The last submitted patch, 49: 2917331-49-test-only.patch, failed testing. View results

longwave’s picture

Status: Needs work » Needs review

Hoisted by my own petard here, as the only failing test is one I introduced to ensure that we clean up after ourselves when removing packages, which I didn't do properly.

Happy otherwise that the removal is good so back to NR for #47.

heddn’s picture

Brief review. The actual patch is pretty repetitive. Not much to see there. However, the IS recommends opening up several follow-ups. Should those happen now?

heddn’s picture

We also don't seem to have any deprecation testing of the symfony cmf removal. And I'm not sure we can actually remove it from composer.lock/json until later. Or we'll run into issue where folks are still using those things. Any way to setup class aliases that trigger BC warnings? Just trying to figure out to inform folks of this removal, before it bites them in D10.

catch’s picture

Issue summary: View changes
FileSize
2.55 KB
2.55 KB

Replying to #52/#53:

We definitely can't remove the actual Symfony CMF dependency until Drupal 10.

There is test coverage for the ::getRoutesPaged() deprecation, it's not new test coverage because the existing test coverage is the only usage in core and we're not replacing it with anything:

+++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
@@ -705,6 +705,9 @@ public function testGetRoutesByPatternWithLongPatterns() {
   /**
    * Tests getRoutesPaged().
+   *
+   * @group legacy
+   * @expectedDeprecation Drupal\Core\Routing\RouteProvider::getRoutesPaged() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0
    */
   public function testGetRoutesPaged() {

I noticed there wasn't deprecation test coverage for ::getRoutesCount() so have just added that. This resolves one of the follow-ups in the issue summary.

For the interface changes:

  1. +++ b/core/lib/Drupal/Core/Routing/EnhancerInterface.php
    @@ -2,11 +2,25 @@
     
     /**
      * A route enhance service to determine route enhance rules.
      */
    -interface EnhancerInterface extends RouteEnhancerInterface {
    +interface EnhancerInterface {
    +
    

    We already override this interface, so existing code should already be type hinting/extending this interface. A grep of contrib shows that modules are all extending the core interface: http://grep.xnddx.ru/search?text=Enhancer%5CRouteEnhancerInterface&filen...

  2. +++ b/core/lib/Drupal/Core/Routing/FilterInterface.php
    @@ -2,11 +2,31 @@
      */
    -interface FilterInterface extends RouteFilterInterface {
    +interface FilterInterface {
    +
    

    This is implemented less in contrib, but again contrib modules are already extending the Drupal core interface when they do:

    http://grep.xnddx.ru/search?text=RouteFilterInterface&filename=

  3. 
    --- /dev/null
    
    --- /dev/null
    +++ b/core/lib/Drupal/Core/Routing/LazyRouteCollection.php
    
    +++ b/core/lib/Drupal/Core/Routing/LazyRouteCollection.php
    +++ b/core/lib/Drupal/Core/Routing/LazyRouteCollection.php
    @@ -0,0 +1,65 @@
    
    @@ -0,0 +1,65 @@
    +<?php
    

    This is the only 1-1 replacement, but the Symfony class is not referenced even once from contrib: http://grep.xnddx.ru/search?text=LazyRouteCollection&filename=

  4. +namespace Drupal\Core\Routing;
    +
    +/**
    + * Provides constants used for retrieving matched routes.
    + */
    +interface RouteObjectInterface {
    +
    

    This swaps from the old Symfony interface to the new one, but it only provides constants, so we couldn't trigger_error() in a method, only when the class is autoloaded which is less helpful.

It should theoretically be possible to class_alias() the old Symfony classes and add trigger_error(), but not sure it would be that useful in practice due to the above. Drupal Rector rules for the interface namespace change would definitely be useful though.

While reviewing the test coverage, I also noticed we don't have coverage for the new LazyRouteCollection so have adapted that over from Symfony CMF's.

I'll take a look at opening the remaining follow-ups.

catch’s picture

Uploaded the interdiff twice... here's the patch.

andypost’s picture

FileSize
5.57 KB
59.72 KB

Drafted CR https://www.drupal.org/node/3151009 and fixed deprecation messages to standards

catch’s picture

catch’s picture

One more follow-up so it doesn't get lost in the meantime #3151022: Remove Symfony CMF dependency from composer.json.

larowlan’s picture

This is looking good to me, in terms of current patch there's really only some docs issues and one bigger question.

First the docs issues:

  1. +++ b/core/lib/Drupal/Core/Routing/EnhancerInterface.php
    @@ -2,11 +2,25 @@
    +   * Update the defaults based on its own data and the request.
    ...
    +   *   The getRouteDefaults array.
    

    I think the docs could use some improvements here, perhaps pointing to the fact that 'defaults' maps to '_defaults:' in routing.yml files. As a minimum 'getRouteDefaults array' should be replaced with a real description

  2. +++ b/core/lib/Drupal/Core/Routing/LazyRouteCollection.php
    @@ -0,0 +1,65 @@
    +  public function __construct(RouteProviderInterface $provider) {
    

    Do we need some boiler-plate docs here?

  3. +++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
    @@ -2,14 +2,77 @@
    +   * must extend the core symfony route. The classes may also implement
    

    nit: should this be Symfony rather than symfony

  4. +++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
    @@ -2,14 +2,77 @@
    +   * @return \Symfony\Component\Routing\Route
    ...
    +   * @throws \Symfony\Component\Routing\Exception\RouteNotFoundException
    

    Missing some docs here

  5. +++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
    @@ -0,0 +1,33 @@
    +    /**
    ...
    +    }
    

    nit: think there's some phpcs issues here around empty lines

Then the bigger question.

We're obviously not removing this from composer.lock/composer.json because we have to provide BC.

But how do we signal to modules and custom code that might be using parts of CMF directly that 'this is going to go away in D10' - is that something we can only do with release notes?

The most likely usage is of the RouteObjectInterface constants - how do we trigger deprecation errors for usages of those?

catch’s picture

We can't trigger_error() for class constants - this is the case whether they originally belonged to vendor code or core, the only way to inform people outside of change records is static analysis (drupal check/rector).

andypost’s picture

Maybe there's way to hijack classloader to throw when "deprecated" class loaded?

andypost’s picture

Making mail shorter

Using static analysis for contrib is tricky, follow up?

catch’s picture

Opened #3151105: Trigger deprecation notifications for replaced Vendor classes to discuss deprecating the use of classes from dependencies. That would be useful for this case, deprecating constants in general (i.e. when they're on classes that are not deprecated overall) I think we need to leave to static analysis.

catch’s picture

Tried to add some docs improvements based on #59.

heddn’s picture

re #60:
can we do a class_exists check in a conditional and add our own and implementation of symfony's interfaces with trigger warnings and then remove the composer dependency? That way we could trigger the deprecation and keep the implementation for those that still need it.

daffie’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
    @@ -0,0 +1,34 @@
    +      'route_2"' => new Route('/route-2'),
    

    Why the added " after route_2?

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
    @@ -0,0 +1,34 @@
    +  public function testGetIterator() {
    

    The method also @covers ::all.

  3. +++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
    @@ -0,0 +1,34 @@
    +class LazyRouteCollectionTest extends UnitTestCase {
    

    Missing testing for the methods: count() and name().

  4. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -401,7 +399,16 @@ protected function routeProviderRouteCompare(array $a, array $b) {
       public function getAllRoutes() {
    -    return new PagedRouteCollection($this);
    +    $select = $this->connection->select($this->tableName, 'router')
    +      ->fields('router', ['name', 'route']);
    +    $routes = $select->execute()->fetchAllKeyed();
    +
    +    $result = [];
    +    foreach ($routes as $name => $route) {
    +      $result[$name] = unserialize($route);
    +    }
    +
    +    return $result;
    

    I cannot find testing for this method.

  5. +++ b/core/lib/Drupal/Core/Routing/Router.php
    @@ -72,14 +61,14 @@ class Router extends UrlMatcher implements RequestMatcherInterface, RouterInterf
    -  public function __construct(BaseRouteProviderInterface $route_provider, CurrentPathStack $current_path, BaseUrlGeneratorInterface $url_generator) {
    +  public function __construct(RouteProviderInterface $route_provider, CurrentPathStack $current_path, BaseUrlGeneratorInterface $url_generator) {
    

    I am not sure if this a BC break.

  6. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -41,4 +42,17 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getAttributes(Route $route, $name, array $attributes) {
    

    From where does this method inherits its docblock?

martin107’s picture

FileSize
61.25 KB
3.15 KB

Sorry for the partial fix... I am working on this as I am able

#66 1, 2 and 3 fixed..

I added all of the required LazyRouteCollectionTest tests.

more from me soon hopefully.

martin107’s picture

FileSize
62.72 KB
4.08 KB

Again just a partial response to #66

A) 66.4

I cannot find testing for this method.

I also could not find one .. we have new code in this method .. so I wrote a test.

B) phpcs fixes to errors I introduced in #67

catch’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
@@ -291,6 +291,33 @@ public function testDuplicateRoutePaths($path, $number, $expected_route_name = N
 
+  /**
+   * Confirms RouteProvider::gatAllroutes() extracts information correctly from the database.
+   */

Nit: s/gatAllRoutes/getAllRoutes/

@66.5: The case that could break would be if someone had subclassed, and overridden the constructor (but leaving the first argument unchanged) - in that case the bc break is that core's RouteProviderInterface no longer extends from Symfony CMF's RouteProviderInterface. To cover that case we'd need to leave the inheritance in core's RouteProviderInterface and remove it in Drupal 10 (possibly in the patch that also removes the dependency). However since we've already provided core's on RouteProviderInterface for some time, code type hinting on RouteProviderInterface should strictly be using that as the type hint anyway.

ravi.shankar’s picture

Here I have fixed Nit mentioned in comment #69.

martin107’s picture

Thank you ravi.shankar that was almost as embarrassing, as that time when I was eleven year old boy, and I discovered I could not spell carpat :)

tim.plunkett’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
@@ -291,6 +291,33 @@ public function testDuplicateRoutePaths($path, $number, $expected_route_name = N
+   * Confirms RouteProvider::getAllroutes() extracts information correctly from the database.

Still wrong?

martin107’s picture

Status: Needs work » Needs review
FileSize
62.72 KB
721 bytes

#72: Still wrong? -- Oh good grief Charlie Brown .. thanks fixed.

In response to #66.6 - urlMatcher.php ::getAttributes()

From where does this method inherits its docblock?

When I trace it... I see it has crossed from drupal comments into Symfony style docs

Here is the original overridden method

Symfony\Component\Routing\Matcher\UrlMatcher::getAttributes()

    /**
     * Returns an array of values to use as request attributes.
     *
     * As this method requires the Route object, it is not available
     * in matchers that do not have access to the matched Route instance
     * (like the PHP and Apache matcher dumpers).
     *
     * @param string $name       The name of the route
     * @param array  $attributes An array of attributes from the matcher
     *
     * @return array An array of parameters
     */
    protected function getAttributes(Route $route, $name, array $attributes)
    {
        $defaults = $route->getDefaults();
        if (isset($defaults['_canonical_route'])) {
            $name = $defaults['_canonical_route'];
            unset($defaults['_canonical_route']);
        }
        $attributes['_route'] = $name;

        return $this->mergeDefaults($attributes, $defaults);
    }
daffie’s picture

Status: Needs review » Needs work

Just 2 nitpicks left:

  1. +++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
    @@ -291,6 +291,33 @@ public function testDuplicateRoutePaths($path, $number, $expected_route_name = N
    +    $route_count = count($returned_routes);
    

    This line can be removed. The variable is not used anywhere.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
    @@ -291,6 +291,33 @@ public function testDuplicateRoutePaths($path, $number, $expected_route_name = N
    +    foreach($returned_routes as $route_name=>$route) {
    

    Can we change this line to: foreach ($returned_routes as $route_name => $route) {.

Deepak Goyal’s picture

Assigned: Unassigned » Deepak Goyal
Deepak Goyal’s picture

Assigned: Deepak Goyal » Unassigned
Status: Needs work » Needs review
FileSize
62.67 KB
810 bytes

Hi @daffie
Updated patch.

daffie’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
@@ -291,6 +291,32 @@ public function testDuplicateRoutePaths($path, $number, $expected_route_name = N
+    foreach ($returned_routes as $route_name=>$route) {

You forgot the spaces between "$route_name", "=>" and "$route".

Deepak Goyal’s picture

@daffie Ooh sorry now i have added please check.

Lal_’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

All my remarks have been addressed.
All code changes look good to me.
Testing has been added where needed.
A CR has been added.
For me it is RTBC.

alexpott’s picture

Looking at http://grep.xnddx.ru/search?text=RouteObjectInterface&filename= I think the CR and the release note needs to give special mention for what to do when you are using the. \Symfony\Cmf\Component\Routing\RouteObjectInterface::ROUTE_NAME constant given there's plenty of usages of that in contrib (and therefore probably custom too).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs work for #81. I guess swapping out to use the new Drupal constant is what people should do - right?

catch’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Update the release note and CR for #81. The constant will be covered by changing the use statement for RouteObjectInterface so the specific mention is really for googleability.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

There's quite a big risk that we'll use the constants from Symfony\Cmf\Component\Routing\RouteObjectInterface sometime without realising it but we have #3151105: Trigger deprecation notifications for replaced Vendor classes to improve that. If we do introduce any new usages then it'd not a big deal to convert them because at least we have the new API in.

Committed 98eed2b and pushed to 9.1.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
index eb450ea03c..88aa947043 100644
--- a/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
+++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
@@ -68,7 +68,7 @@ public function testCount() {
   }
 
   /**
-   * Search for a both a findable and a unfindable route.
+   * Search for a both an existing and a non-existing route.
    *
    * @covers ::get
    */
@@ -84,11 +84,11 @@ public function testGetName() {
     // Miss.
     $this->routeProvider
       ->method('getRouteByName')
-      ->with('does_not_exists')
+      ->with('does_not_exist')
       ->will($this->throwException(new RouteNotFoundException()));
 
     $lazyRouteCollectionFail = new LazyRouteCollection($this->routeProvider);
-    $this->assertNull($lazyRouteCollectionFail->get('does_not_exists'));
+    $this->assertNull($lazyRouteCollectionFail->get('does_not_exist'));
   }
 
 }

Unfindable is not in our dictionary - we could add it but I choose the simpler solution of replacing the text with something that matches the test a bit more.

  • alexpott committed 98eed2b on 9.1.x
    Issue #2917331 by catch, martin107, Deepak Goyal, longwave, andypost,...
Taran2L’s picture

Priority: Major » Critical
Status: Fixed » Needs work

This change is not backward-compatible. It cannot be shipped in D9.x:

Redirect module provides service redirect.checker:

  redirect.checker:
    class: Drupal\redirect\RedirectChecker
    arguments: ['@config.factory', '@state', '@access_manager', '@current_user', '@router.route_provider']

It depends on @router.route_provider. Then the service itself declares:


namespace Drupal\redirect;

use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Cmf\Component\Routing\RouteProviderInterface;

/**
 * Redirect checker class.
 */
class RedirectChecker {

  /**
   * @var \Drupal\Core\Routing\RouteProviderInterface
   */
  protected $routeProvider;

  public function __construct(ConfigFactoryInterface $config, StateInterface $state, AccessManager $access_manager, AccountInterface $account, RouteProviderInterface $route_provider) {}

This change produces an error because the type hint is wrong:

Argument 5 passed to Drupal\redirect\RedirectChecker::__construct() must be an instance of Symfony\Cmf\Component\Routing\RouteProviderInterface, instance of Drupal\Core\Routing\RouteProvider given, called in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 257
Drupal\redirect\RedirectChecker->__construct()() (Line: 43)

See https://dispatcher.drupalci.org/job/drupal8_contrib_patches/40547/ and https://dispatcher.drupalci.org/job/drupal8_contrib_patches/40541/

UPDATE

Well, there is only 3 modules using it, but still http://grep.xnddx.ru/search?text=Symfony%5CCmf%5CComponent%5CRouting%5CR...

Taran2L’s picture

Status: Needs work » Fixed

Well, seems like the Redirect module is using the incorrect type hint for the service. Then no issue here. Sorry for the confusion.

alexpott’s picture

Status: Fixed » Needs work

@berdir has pointed out in slack this is a hard break for redirect. I'm going to revert to see if we can come up with a solution that works for redirect without a hard break but still allows us to decouple.

  • alexpott committed cd7455f on 9.1.x
    Revert "Issue #2917331 by catch, martin107, Deepak Goyal, longwave,...
catch’s picture

If #3153908: Redirect module uses incorrect type hint for the router.route_provider service argument of the redirect.checker service is the Redirect issue, then I don't think it' s a hard break - Redirect currently type hints on RouteProviderInterface directly from Symfony CMF, but core has provided RouteProviderInterface since probably 8.0.x, so updating the type-hint to the core version should be enough.

catch’s picture

Status: Needs work » Needs review
FileSize
748 bytes
62.58 KB

Here's a version that shouldn't force changes to Redirect module (until Drupal 10).

For Drupal 10 we'd essentially have to reverse the interdiff here prior to removing the Symfony/Cmf dependency.

alexpott’s picture

Discussed #91 with @catch and @Berdir - I think it is the most BC friendly solution. Also I think we can add a compiler pass to check if any services have a route.provider dependency and are using a Symfony\Cmf\Component\Routing\RouteProviderInterface typehint. As this will be useful for deprecating / removing any third party dependency going to open a follow-up to do this - see #3154108: Add compiler pass to check for use of third party interfaces that are going to be removed.

I've tested #91 locally with the redirect module and it works great. +1 for rtbc - I'll leave doing that to someone else so I can commit.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Since the updated patch is just a straight revert of a couple of lines, moving back to RTBC.

edit: also opened the Drupal 10 follow-up #3154116: Remove Symfony/CMF dependency.

  • alexpott committed 3789388 on 9.1.x
    Issue #2917331 by catch, martin107, Deepak Goyal, longwave, andypost,...
daffie’s picture

Status: Reviewed & tested by the community » Fixed

Back to Fixed.

alexpott’s picture

Committed 3789388 and pushed to 9.1.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
index eb450ea03c..88aa947043 100644
--- a/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
+++ b/core/tests/Drupal/Tests/Core/Routing/LazyRouteCollectionTest.php
@@ -68,7 +68,7 @@ public function testCount() {
   }
 
   /**
-   * Search for a both a findable and a unfindable route.
+   * Search for a both an existing and a non-existing route.
    *
    * @covers ::get
    */
@@ -84,11 +84,11 @@ public function testGetName() {
     // Miss.
     $this->routeProvider
       ->method('getRouteByName')
-      ->with('does_not_exists')
+      ->with('does_not_exist')
       ->will($this->throwException(new RouteNotFoundException()));
 
     $lazyRouteCollectionFail = new LazyRouteCollection($this->routeProvider);
-    $this->assertNull($lazyRouteCollectionFail->get('does_not_exists'));
+    $this->assertNull($lazyRouteCollectionFail->get('does_not_exist'));
   }
 
 }

Made the same changes.

martin107’s picture

findable and a unfindable route.

Thanks for finding better wording ... I was obviously mentally blocked when I added that comment

alexpott++

Status: Fixed » Closed (fixed)

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

dpi’s picture