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
Comment | File | Size | Author |
---|---|---|---|
#91 | 2917331-91.patch | 62.58 KB | catch |
#91 | 2917331-interdiff-78-91.patch | 748 bytes | catch |
#78 | interdiff_73-78.txt | 812 bytes | Deepak Goyal |
#78 | 2917331-78.patch | 62.67 KB | Deepak Goyal |
#76 | interdiff_73-76.txt | 810 bytes | Deepak Goyal |
Comments
Comment #2
alexpottThe other tricky things are like:
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?
Comment #3
catchI'd expect us to actually remove the dependency in 9.0.0 and just leave it deprecated until then.
Comment #4
Wim Leers#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:And similarly:
Comment #7
catchComment #8
dawehnerGiven 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.
Comment #9
catchComment #10
catchComment #12
xjmComment #13
catchComment #14
geek-merlinFixed link.
Comment #16
catchHere'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.
Comment #17
catchAnother one:
Drupal\Core\Routing\UrlGeneratorInterface
extendsSymfony\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.
Comment #18
catchMissing a use statement.
Comment #19
catchjsonapi uses CONTROLLER_NAME after all, so bringing that back.
Comment #21
catchMerging 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.
Comment #22
catchCopying 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.
Comment #23
catchAdding one method from CMF's UrlMatcher to ours and removing the subclass.
Down to only a handful of non-test references now:
Comment #25
catchMerging FilterInterface and EnhancerInterface.
Comment #26
catchComment #27
catchEven more missing use statements.
Comment #29
catchCompletely 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.
Comment #30
catchOverzealous removal.
Comment #31
catchNow deprecating ::getRoutesPaged() and switching the test coverage to a legacy test.
Comment #33
catchUpdating the issue summary.
Comment #35
longwaveWhat is DynamicRouter? I think this is an obsolete Symfony CMF concept?
I don't think this variable name should be changing.
Some remaining references to Symfony CMF.
Comment #36
catchApplying the coding standards fixes.
Comment #37
catchThanks 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.
Comment #38
longwaveThis could just be $router, and $router could be renamed $accessAwareRouter?
This needs to change to match, and also testCall() still mocks the Symfony CMF objects.
Comment #41
catchComment #42
catchGah bad patch, interdiff is OK though.
Comment #45
catchTypo 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.
Comment #46
catchDeprecating ::getRoutesCount() and removing it from the test implementation - completely dead code.
Comment #47
catchRe-rolled against 9.1.x
Comment #48
jibranShould we add a test only patch that removes Symfony CMF component and makes sure the patch is still green?
Comment #49
longwaveLet's try #48.
There is still an @see left in Drupal\Core\Routing\RouteProviderInterface:
Comment #51
longwaveHoisted 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.
Comment #52
heddnBrief 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?
Comment #53
heddnWe 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.
Comment #54
catchReplying 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:
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:
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...
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=
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=
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.
Comment #55
catchUploaded the interdiff twice... here's the patch.
Comment #56
andypostDrafted CR https://www.drupal.org/node/3151009 and fixed deprecation messages to standards
Comment #57
catchChanges in #56 look great.
I opened #3151017: Deprecate UrlGenerator::supports() and UrlGenerator::getRouteDebugMessage() and #3151019: Only allow route names, deprecate support for route objects in UrlGenerator methods for follow-ups.
Comment #58
catchOne more follow-up so it doesn't get lost in the meantime #3151022: Remove Symfony CMF dependency from composer.json.
Comment #59
larowlanThis 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:
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
Do we need some boiler-plate docs here?
nit: should this be Symfony rather than symfony
Missing some docs here
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?
Comment #60
catchWe 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).
Comment #61
andypostMaybe there's way to hijack classloader to throw when "deprecated" class loaded?
Comment #62
andypostMaking mail shorter
Using static analysis for contrib is tricky, follow up?
Comment #63
catchOpened #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.
Comment #64
catchTried to add some docs improvements based on #59.
Comment #65
heddnre #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.
Comment #66
daffie CreditAttribution: daffie commentedWhy the added
"
after route_2?The method also @covers ::all.
Missing testing for the methods: count() and name().
I cannot find testing for this method.
I am not sure if this a BC break.
From where does this method inherits its docblock?
Comment #67
martin107 CreditAttribution: martin107 as a volunteer commentedSorry 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.
Comment #68
martin107 CreditAttribution: martin107 as a volunteer commentedAgain just a partial response to #66
A) 66.4
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
Comment #69
catchNit: 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.
Comment #70
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have fixed Nit mentioned in comment #69.
Comment #71
martin107 CreditAttribution: martin107 as a volunteer commentedThank 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 :)
Comment #72
tim.plunkettStill wrong?
Comment #73
martin107 CreditAttribution: martin107 as a volunteer commented#72: Still wrong? -- Oh good grief Charlie Brown .. thanks fixed.
In response to #66.6 - urlMatcher.php ::getAttributes()
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()
Comment #74
daffie CreditAttribution: daffie commentedJust 2 nitpicks left:
This line can be removed. The variable is not used anywhere.
Can we change this line to:
foreach ($returned_routes as $route_name => $route) {
.Comment #75
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #76
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi @daffie
Updated patch.
Comment #77
daffie CreditAttribution: daffie commentedYou forgot the spaces between "$route_name", "=>" and "$route".
Comment #78
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented@daffie Ooh sorry now i have added please check.
Comment #79
Lal_Comment #80
daffie CreditAttribution: daffie commentedAll 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.
Comment #81
alexpottLooking 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).Comment #82
alexpottNeeds work for #81. I guess swapping out to use the new Drupal constant is what people should do - right?
Comment #83
catchUpdate 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.
Comment #84
alexpottThere'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!
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.
Comment #86
Taran2LThis change is not backward-compatible. It cannot be shipped in D9.x:
Redirect module provides service
redirect.checker
:It depends on
@router.route_provider
. Then the service itself declares:This change produces an error because the type hint is wrong:
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...
Comment #87
Taran2LWell, seems like the Redirect module is using the incorrect type hint for the service. Then no issue here. Sorry for the confusion.
Comment #88
alexpott@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.
Comment #90
catchIf #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.Comment #91
catchHere'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.
Comment #92
alexpottDiscussed #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.
Comment #93
catchSince 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.
Comment #95
daffie CreditAttribution: daffie commentedBack to Fixed.
Comment #96
alexpottCommitted 3789388 and pushed to 9.1.x. Thanks!
Made the same changes.
Comment #97
martin107 CreditAttribution: martin107 as a volunteer commentedThanks for finding better wording ... I was obviously mentally blocked when I added that comment
alexpott++
Comment #99
dpiCreated BC break issue at #3158708: RouteProvider::getAllRoutes no longer returns an iterable, breaking BC