Add custom Symfony CMF Dynamic Router / Route Provider to new Route Bundle #7860
Conversation
alexander-schranz
force-pushed
the
feature/move-cmf-routing-in-new-route-bundle
branch
2 times, most recently
from
53f4dda to
c794039
Compare
alexander-schranz
force-pushed
the
feature/move-cmf-routing-in-new-route-bundle
branch
from
c794039 to
32d082f
Compare
| /** | ||
| * @final | ||
| * | ||
| * @internal No BC promises are given for this class. It may be changed or removed at any time. | ||
| */ | ||
| class CmfRouteProvider implements RouteProviderInterface | ||
| { | ||
| /** | ||
| * @param RouteCollectionForRequestLoaderInterface[] $routeCollectionForRequestLoaders | ||
| */ | ||
| public function __construct( | ||
| private iterable $routeCollectionForRequestLoaders, | ||
| ) { | ||
| } |
There was a problem hiding this comment.
The whole idea is that this is our only connection point with the Symfony\Cmf. Marked it as @internal as this shouldn't be changed or used by projects. Also we are even free the replace it if in the future dynamic router is maybe part of symfony core or a new major version of it is released with changes.
With our own RouteCollectionForRequestLoaderInterface which just has public function getRouteCollectionForRequest(Request $request): RouteCollection; we have all about routing then in our own control. This removes the complex service definitions which we have here:
sulu/src/Sulu/Bundle/RouteBundle/Resources/config/routing.xml
Lines 11 to 63 in b605ff3
sulu/src/Sulu/Bundle/CustomUrlBundle/Resources/config/routing.xml
Lines 11 to 108 in b605ff3
- https://github.com/sulu/SuluRedirectBundle/blob/6964ba5714cc1849e3243b6882a289fc845595f9/Resources/config/router.xml#L7-L51
So we are replacing the different DynamicRouter of 1.x and 2.x with a single DynamicRouter. Which config is a lot easier:
$builder->prependExtensionConfig(
'cmf_routing',
[
'chain' => [
'routers_by_id' => [
'router.default' => 100,
'cmf_routing.dynamic_router' => 20,
],
],
'dynamic' => [
'route_provider_service_id' => 'sulu_route.symfony_cmf_route_provider',
'enabled' => true,
],
]
); if ($builder->hasExtension('cmf_routing')) {
$services->set('sulu_route.symfony_cmf_route_provider')
->class(CmfRouteProvider::class)
->args([
tagged_iterator('sulu_route.route_collection_for_request_loader'),
]);
}I also added the RouteDefaultsProviderInterface which is very similar to our existing RouteBundle entity of 2.6 version. But simplified it that every resourceKey requires an own defaults provider.
alexander-schranz
force-pushed
the
feature/move-cmf-routing-in-new-route-bundle
branch
from
eba11ee to
d8f3ef2
Compare
alexander-schranz
force-pushed
the
feature/move-cmf-routing-in-new-route-bundle
branch
9 times, most recently
from
646c1eb to
98e0089
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements the CMF Dynamic Router by integrating a custom Symfony router into the new route bundle to handle CMS-based routing using an internal Route Model/Entity.
- Implement the dynamic router for CMS routes.
- Add service configuration for ExampleSiteRouteGenerator in the test application configuration.
Files not reviewed (18)
- config/bundles.php: Language not supported
- packages/route/src/Application/Routing/Generator/RouteGenerator.php: Language not supported
- packages/route/src/Application/Routing/Generator/RouteGeneratorInterface.php: Language not supported
- packages/route/src/Application/Routing/Generator/SiteRouteGeneratorInterface.php: Language not supported
- packages/route/src/Application/Routing/Matcher/RouteCollectionForRequestLoaderInterface.php: Language not supported
- packages/route/src/Application/Routing/Matcher/RouteCollectionForRequestRouteLoader.php: Language not supported
- packages/route/src/Application/Routing/Matcher/RouteDefaultsProviderInterface.php: Language not supported
- packages/route/src/Application/Routing/Matcher/RouteHistoryDefaultsProvider.php: Language not supported
- packages/route/src/Domain/Exception/MissingRequestContextParameterException.php: Language not supported
- packages/route/src/Domain/Model/Route.php: Language not supported
- packages/route/src/Domain/Value/RequestAttributeEnum.php: Language not supported
- packages/route/src/Infrastructure/Symfony/HttpKernel/SuluRouteBundle.php: Language not supported
- packages/route/src/Infrastructure/SymfonyCmf/Routing/CmfRouteProvider.php: Language not supported
- packages/route/tests/Application/ExampleSiteRouteGenerator.php: Language not supported
- packages/route/tests/Application/Kernel.php: Language not supported
- packages/route/tests/Functional/Infrastructure/Doctrine/Repository/RouteRepositoryTest.php: Language not supported
- packages/route/tests/Functional/Infrastructure/Symfony/HttpKernel/SuluRouteBundleTest.php: Language not supported
- packages/route/tests/Functional/Infrastructure/SymfonyCmf/Routing/CmfRouteProviderTest.php: Language not supported
Comments suppressed due to low confidence (1)
packages/route/tests/Application/config/config.yml:11
- [nitpick] Consider verifying if the service name 'ExampleSiteRouteGenerator' clearly reflects its intended use. If it's solely for testing purposes, a more descriptive name might help distinguish its role.
+
98e0089 to
5d5c1b6
Compare
…for request loader and history route defaults provider
alexander-schranz
force-pushed
the
feature/move-cmf-routing-in-new-route-bundle
branch
from
5d5c1b6 to
46ce49c
Compare
| $hasDynamicRouting = false; | ||
| foreach ($bundles as $key => $bundle) { | ||
| // remove old route bundle to avoid conflicts | ||
| if (DeprecatedSuluRouteBundle::class === $bundle::class | ||
| || SuluPreviewBundle::class === $bundle::class | ||
| || \str_contains($bundle::class, 'Sulu') | ||
| || \str_contains($bundle::class, 'Massive') | ||
| || \str_contains($bundle::class, 'PHPCR') | ||
| || \str_contains($bundle::class, 'SecurityBundle') |
There was a problem hiding this comment.
currently having lot of conflicts with Sulu and other bundles as that combinations not work properly so I removed all bundles which are not required to run the RouteBundle tests.
packages/route/src/Application/Routing/Matcher/RouteHistoryDefaultsProvider.php
Outdated
Show resolved
Hide resolved
packages/route/src/Application/Routing/Matcher/RouteHistoryDefaultsProvider.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Prokyonn <[email protected]>
alexander-schranz
deleted the
feature/move-cmf-routing-in-new-route-bundle
branch
| public function getRouteCollectionForRequest(Request $request): RouteCollection | ||
| { | ||
| foreach ($this->routeCollectionForRequestLoaders as $routeCollectionLoader) { | ||
| $routeCollection = $routeCollectionLoader->getRouteCollectionForRequest($request); |
There was a problem hiding this comment.
I'm little bit unsure about giving here the Request $request here. I see that the Symfony router is also using the Request or string pathInfo for the UrlMatcher: https://github.com/symfony/routing/blob/ee9a67edc6baa33e5fae662f94f91fd262930996/Matcher/UrlMatcher.php#L85
But from my point of view the RequestContext would maybe be a better option 🤔 to be more consistent between Matcher and Generator, maybe I miss understand the concept of the RequestContext, and we should stay with Request for matching and context for generating.
There was a problem hiding this comment.
as i understand, the matcher uses the Request as that holds a rich model of the whole request including headers. the header bag gives you precise access to headers, even parsing cache headers, or more relevant to routing the accept header etc.
There was a problem hiding this comment.
i would look if you can make this method a custom RouterInterface implementation that either throws the route not found exception or returns the one route that matches the slug.
the dynamic router is built around the idea of finding several candidates and not reimplement the logic to decide which route candidate should win, with accept or parameters and regex etc.
There was a problem hiding this comment.
Okay, the Symfony RouterInterface just has the getRouteCollection without any parameters and so Request so to achieve same logic I would create own router then and inject there the requestStack to get currentRequest for the matching. It doesn't make any problems implementing that interface and return a empty collection in none match / cli context? Thought maybe it would cache the empty collection and will then not longer be called again.
it has a strange comment on it:
* WARNING: This method should never be used at runtime as it is SLOW. * You might use it in a cache warmer though.
| return new RouteCollection(); | ||
| } | ||
|
|
||
| $route = $this->routeRepository->findOneBy([ |
There was a problem hiding this comment.
the general phylosophy of the dynamic router is that there could be several database entries potentially matching the request (the default entity of the cmf bundle allows for a parameter matching part in the url, and the usual constraints of symfony routes, so that the db could hold /data/{id} and /data/special and with the right rules the symfony matcher then picks the correct one.
or multiple routes with different requirements on the accept header.
if you only ever have one route per site & slug, fetching only one is fine, but if you are sure about that, i am not sure about doing a collection loader - you could also implement the RouterInterface directly and have it match the one route you found. (or do you expect that sometimes what you find with the slug is going to be not matched, because it has furhter constraints?)
There was a problem hiding this comment.
Really thank you @dbu for taking a look at this,
I think we really have never the case that multiple routes are returned. If a "static route" is matched it should not call our Route Entity. If a Slug exists it will return that route entity, and go only to the next loader / provider if it didn't match.
That two or more routes are returned I really think never happens in our case as we only handling "direct" matches. The only thing is that multiple URLs can match the same route e.g. /blog and /blog.rss where we skipt the .rss in loading.
(or do you expect that sometimes what you find with the slug is going to be not matched, because it has furhter constraints?)
Even it has further constraint it is route independent — so the controller may throw a 404 if now /blog.rss is requested but no blog.rss.twig exists. But that is a process totally outside of routing and don't need to be handled there.
To be sure the ChainRouter will only call the others if it could not find any match?
There was a problem hiding this comment.
yes, the first router to match wins.
the routers can have a priority number to control the order (so you can have slugs overwrite symfony system routes, or protect system routes from being overwritten by slugs).
i would tend to put the symfony system router first, to avoid database queries when a system route is matched, and to prevent somebody creating a page at /login or whatever, which could at least mess up things, or in the worst case post security risks.
There was a problem hiding this comment.
I see, thank you. Static Routes are prioritzed also in your case. We still have on our list to not allow creating dynamic patches which may match a static route for better UX, but from the priority view its not a problem.
There was a problem hiding this comment.
for that you could have a service that is triggered during save that does not have the chain router but only the symfony router and tries to match the slug and complains if it matches...
| $defaults['_sulu_route'] = $route; | ||
|
|
||
| $routeCollection = new RouteCollection(); | ||
| $symfonyRoute = new SymfonyRoute( |
There was a problem hiding this comment.
again, just a note, not something i think you have to change: in the cmf bundle, we had our entity extend the symfony route model class. it needed a few hacks however, but as we potentially match many more than 1 single route, it seemed worthwile to not have to convert all the objects.
There was a problem hiding this comment.
Yeah we avoid extend from third party when it is not required. We burned in the past often our hands when third party classes added new methods or return types. So we go over composition over inheritance where possible :) But still thx for the suggestion.
There was a problem hiding this comment.
yep, makes a lot of sense to do it this way. the constructor certainly is covered by the symfony BC promise. random internal methods and properties can be tricky.
What's in this PR?
Implement the cmf dynamic router into the new route bundle. The implementation will be kept internal. The new router uses our own Route Model / Entity: https://github.com/sulu/sulu/blob/3.0/packages/route/config/doctrine/Route/Route.orm.xml
Why?
The new route bundle should be the only bundle in core handling the CMS based routing.