Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The architecture of ChainedRouter => NestedRouter => DynamicRouter gives us great flexibility by sharing code with CMF.
On the other hand, in HEAD, nothing is using the ChainedRouter, we just use it as one additional layer.
This makes things a bit more complex than it has to be, it adds a little bit more runtime than it needs to be.
Proposed resolution
Remove the usage and alias the existing service. Its a GREAT sign for the architecture that this is everything we need at this given point in time.
Remaining tasks
User interface changes
API changes
Data model changes
Beta phase evaluation
Issue category | Bug/Task/Feature because ... |
---|---|
Issue priority | Normal, because its one detail of the routing subsystem |
Disruption | The only disruption is for whoever uses the chained router directly. They can alter the DI definitions and be done. Existing containers should contain to work as they are, as we don't remove any code. |
Comment | File | Size | Author |
---|---|---|---|
#29 | 2576809-frontpage-router.patch | 2.31 KB | dawehner |
#23 | interdiff.txt | 15.55 KB | dawehner |
#23 | 2576809-23.patch | 18.6 KB | dawehner |
#11 | interdiff.txt | 623 bytes | dawehner |
#11 | 2576809-11.patch | 4.63 KB | dawehner |
Comments
Comment #2
dawehnerComment #3
alexpottIf this works +1 - less onion skins. And it is easy for a project to use it if they need it.
Comment #4
dawehnerComment #7
dawehnerComment #8
kgoel CreditAttribution: kgoel at Forum One commentedComment #11
dawehnerMhhh, this is not nice, see https://github.com/symfony-cmf/Routing/issues/151
Comment #14
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedWe should really kill as much of this code as possible - anyone who really needs to nest or chain can add those back
Comment #15
kgoel CreditAttribution: kgoel as a volunteer commentedIf I understand correctly ChainRouter just prioritize list of routers and loops through chained router. Also that ChainRouter provides DynamicRouter and uses NestedMatcher.
Do we need to use 3 step or multi-step matching process or are we using prioritize list of router in core? I have quickly scanned core for the Routing CMF and I didn't find usage of ChainRouter in core except in AccessAwareRouter.php class. If we decide to remove ChainRouter then does that mean that we need to remove DynamicRouter and NestedMatcher from core. As for right now LazyRouteFilter and UrlMatcher classes are using NestedMatcher.
I am wondering if it's too late to remove this from core. I had a conversation on IRC and suggestion was to move CMF into an optional module enabled by default/hook_update and later drop this in d9.
I think removing this from core would be a good idea if we aren't using it and don't know if contrib would have any need to use ChainRouter. I would like to work on this issue after getting approval from routing maintainer.
Comment #16
dawehnerI hope we can keep this issue in scope. People don't understand what CMF is doing, they don't understand what the NestedMatcher is really doing, but well, the ChainRouter here is doing effectively nothing by default at the moment so I think we should drop it.
Comment #17
kgoel CreditAttribution: kgoel as a volunteer commentedYes, that is the issue that very few people understand routing in general and layers associated with it which is sad. My question is - Do we need to get rid of CMF and use default routing from Symfony or just drop ChainRouter from core? Also, this seems like too late to go in 8.0.x but could be done in 8.1.x
Comment #18
dawehnerYeah, this is the thing, the NestedMatcher is the class which deals with the RouteProvider. We cannot get rid of that, well, beside reimplementing the functionality.
This issue is first about getting rid of one class, which is not really needed.
Comment #19
Crell CreditAttribution: Crell as a volunteer commentedChainRouter is just a chain-of-responsibility wrapper. Removing it from the pipeline SHOULD be possible, but that doesn't buy us much, IMO.
My concern with removing it from the default config is that if someone wants to re-enable it in order to add a faster pre-router (which is what ChainRouter is for), it's non-obvious how to do so, especially if multiple modules try to do so. We've been discussing that already, eg #2471234-11: Create a ChainedRouteProvider which subclasses RouteProvider but could return early for most common routes.
That said, as I also note in that issue I'm not certain that ChainRouter still works, given the amount of work we have DynamicRouter and NestedRouter doing. Which is why removing those is completely off the table. It may be possible to refactor the pipeline in the router to have fewer moving parts, but that would be so not RC-safe it's not even funny. The availability of route filters and enhancers, eg, is part of our API, and we're using those in core, and I know some contribs are planning to do so. Eg, #2305199: Create a route for every variant and use a "route filter" to apply selection rules. Changing away from NestedMatcher/DynamicRouter is D9 material.
What I'd really like to see us do is try to use ChainRouter and make sure it's still viable. Could we still build a useful and worthwhile early/fast router (which doesn't have to use DynamicRouter or NestedMatcher)? If not, then we cry a little and disable ChainRouter as it doesn't buy us anything anyway. If we can, then maybe we should go ahead and use it. :-) (Or at least keep it around to prove that sites can have custom optimized routers.)
Comment #20
dawehnerYeah no question, these concepts totally make sense. To be clear, my intention of this issue was to just deal with ChainRouter and see whether we could make that piece a little bit easier to understand for most people, because debugging the routing system is sadly an often usecase, when you get your routing definition wrong.
What about making it a on-demand thing? So we can somehow tag routers. If there is just one, use that one directly, otherwise get the ChainRouter in place. This gives people flexibilty, but also makes things easier in the first place. The other day I was always wondering whether a ChainRouter is really the best idea. Can't we have like a Stack of routers?
Comment #21
Crell CreditAttribution: Crell as a volunteer commentedWell ChainRouter is a stack of routers. It's the same chain approach we use in a half dozen places now.
Enhancing the compiler pass to automatically insert it sounds interesting. I'm not opposed to that, although I'm a bit concerned at making it more non-deterministic, especially for debuggers. Really, if you're messing with the router services you need to really know what you're doing.
Comment #22
kgoel CreditAttribution: kgoel as a volunteer commentedI am not sure how to use ChainRouter and I am sure others might be wondering about the practical usage of this. Could someone please given an example how to use it so more than one person can test to see if it works or not? It will help us understand better.
We need to document when ChainRouters can be use or if NestedMatcher, DynamicRouter would be enough.
Can you give example? I didn't see ChainRouter anywhere in core except in AccessAwareRouter.php unless I am missing something.
Comment #23
dawehnerWell I was more thinking of an onion style approach.
This here fixes some of the failures but yeah, as we talked on the criticals meeting, this is not something we will do before 8.0.x and will require a BC layer in 8.1.x, but adding it conditionally can land later.
Comment #25
kgoel CreditAttribution: kgoel as a volunteer commentedHas it been decided to remove this from core?
Comment #26
dawehner.
Comment #27
Crell CreditAttribution: Crell as a volunteer commentedThe onion is just for access aware router, which is an entirely separate question. I was actually quite against that thing, actually, but didn't have the energy to fight it.
I don't understand dawehner's comment either. I was not in any discussion about deciding what to do here. My statements in #19 and #21 still stand.
Comment #28
dawehnerWell, I know but I was simply wondering whether someone could implement something like the chained router using an onion as well.
Does someone mind me having my own thoughts without having to discuss them with people first?
At least at the moment you need to alter the container via code, given that the calls to it have to be hardcoded, so I'm not sure anyone actually implemented it
So the most obvious implementation would be to use it just for the frontpage, it even will not require upcasting, I guess? The other most accessed page will be
/node/{node}
for which we though already need manual upcasting, aka. initialize the entity manager, which is not for free at all?Comment #29
dawehnerI just tried to write a frontpage router and well, the fact that we expose the route object has lead to horrible problems, see patch.
Comment #30
Crell CreditAttribution: Crell as a volunteer commentedAside from the potential route recompilation, what's the issue?
Comment #31
dawehnerWell you need an entire full working route object (with processed access checks etc.), I guess we should fetch that directly from the RouteProvider?
Comment #32
Crell CreditAttribution: Crell as a volunteer commentedAh, hm. If we're fetching from RouteProvider anyway, do we even gain that much? It would still be a DB hit, unless we had an alternate RouteProvider for the alternate router that had a handful of compiled-to-PHP routes.
Comment #33
dawehnerWell, its basically at least for the frontpage we could apply the caching of the inbound path + routing on that level as well, which will give us already the route object and what not.
Comment #36
dawehnerIMHO this would still be nice to try to land in a minor.
Comment #37
Wim Leers+1
Comment #38
dawehnerThis is doable during the 8.x.y. cycle.
Comment #39
dawehnerNote: #2810303: Reunite the router: One router to rule them all is effectively a duplicate now.
Comment #40
tstoecklerYou are very mean @dawehner! My heart started racing, when I saw this issue green in the tracker... ;-)