Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update 'dynamic' routing based on names of content types and taxonomies #3095

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

simongroenewolt
Copy link
Contributor

@simongroenewolt simongroenewolt commented Feb 19, 2022

This change moves content-type and taxonomy routing config from Kernel + annotations to a RouteLoader service class. By doing so it removes (some of) the dependencies of Kernel.php on config parsers, the use of these parsers by Kernel.php can be problematic/confusing for developers working on or using Bolt because it removes the possibility to override (parts of) the Config.php class, for example during testing.
As the Config.php class is itself a service, it can not be used by the kernel (you cannot use services while configuring the service container)

@simongroenewolt
Copy link
Contributor Author

simongroenewolt commented Feb 19, 2022

Hmm, the phpunit errors do not look like they are related to my changes, or are they?

Update: there were related to my changes, pushed a fix.

@simongroenewolt
Copy link
Contributor Author

I've been thinking a bit more about the general Bolt configuration setup, and while I do think this PR could make sense, I'm wondering if it is the right approach after all. This is because of on the way Config.php is used, being injected as a service, vs. the 'general' way configuration is handled by symfony and related packages/extensions.
If I understand correctly most bundles handle config parsing in the container building phase (via an Extension class), which is more or less the use in the same phase as I've removed in this PR.
My guess is that when you decided on the bolt config setup there probably were good reasons to make it like it is now - it would be interesting to know more about the reasons why it is like it is.

@bobdenotter
Copy link
Member

My guess is that when you decided on the bolt config setup there probably were good reasons to make it like it is now - it would be interesting to know more about the reasons why it is like it is.

It is the way it is now, because I didn't know how else I could (programmatically) add the requirements to the routing. Personally, I think this is an improvement, because it removes black magic from Kernel. It might look like it's more code than before, but if you take into account a large part of the new code is comments, it seems to be about the same.

I'm 👍 on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants