Add the RegisterRouteEnhancersPass to the bundle for tagged route enhancers

[benglass]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	?
License	MIT
Doc PR	https://github.com/symfony-cmf/symfony-cmf-docs/pull/320

[dbu]

hi ben, thanks a lot. fully agree that we should have this. i added the pull request template to the PR, as the contribution requirements say. http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request can you please create a pull request to add some information to the documentation of the simple cms bundle? (i guess its too specific for the book section, but the bundle section should list all features)

i just realized that right before tagging RoutingBundle 1.1 we added priorities to the enhancers to allow to properly insert custom enhancers at the right place: https://github.com/symfony-cmf/RoutingBundle/blob/master/DependencyInjection/CmfRoutingExtension.php#L142 i wonder if we could extract that code in a way that the simple cms extension class can reuse it. maybe a static public method? then we can release that as RoutingBundle 1.1.1 and start to depend on that version and remove some duplicated code here. would that make sense for you? we should not block this PR for that however. and it would be a BC break if anybody registered enhancers for simple cms already, as currently all default enhancers are at priority 0 and "accidentally" in the right order. so i guess it would be for 1.1 - which has feature freeze end of this month, so pretty soon as well.

[benglass]

@dbu I have created the PR you requested for docs and I will review the PR instructions you linked and try to get in the habit of adding the header.

In terms of the code you are talking about re-using, you just mean the code in the Extension that conditionally adds enhancers based on configuration settings? There are a number of ways in which the simple cms bundle provides its own versions of services that are already defined by the routing bundle (id prefix listener, dynamic router, etc.). It feels a little to me like if those services were more configurable in some way the code duplication could potentially be avoided and also possibly some confusion could be avoided but I'm not clear on how this would be approached. This could also help to reduce the need to add documentation to simple cms for features like route enhancers that are part of the normal cmf docs.

I will work up the docs pr.

[dbu]

cool. i commented on your issue in symfony-cmf-docs: lets keep the doc minimal and not duplicate anything more than a teaser for what the enhancers are useful.

regarding the configuration and services we have: i was thinking about the code that adds the enhancers, yes. the configuration options are called the same so it would work to have a static method that we can call from simplecms, passing the configuration array and the container and config and prefix.

then again, as you said on the mailinglist, there is a lot of duplication of services going on. for performance, you could simply disable the dynamic router in the RoutingBundle if you only have routes under the root path of the simple cms router. but this is not a good answer and keeps things too complicated.

maybe we should just make the RouteProvider and the IdPrefixListener of RoutingBundle capable of handling multiple roots? then we could drop all the duplicated services and people would configure routing things at the cmf_routing configuration point. another option would be to avoid the duplicated route tree and have people put simple cms pages under /cms/routes (or configure a different path). that way, we would not need to change anything in the RoutingBundle but could reduce complexity of services and avoid the duplicated DynamicRouter. at that point we probably should drop the configuration for the base path in simple cms and just use the one from routing. this would of course be a BC break, but 1.1 is close enough already. what do you think?

[benglass]

Making the router bundle capable of handling multiple roots would be a step towards multi site support but i'm not sure its the right decision unless we can identify the use case where you would want do that. For multi-site you dont want multiple roots, you just want a different root depending on the site.

Is there any way to solve the issue by having the simple cms bundle use a compiler pass to modify the configuration of the routing bundle similar to the way that the cmf core modifies the cmf content/menu/routing bundle configurations so you can configure everything in one place?

On Mon, Nov 4, 2013 at 3:44 PM, David Buchmann notifications@github.comwrote:

cool. i commented on your issue in symfony-cmf-docs: lets keep the doc minimal and not duplicate anything more than a teaser for what the enhancers are useful.

regarding the configuration and services we have: i was thinking about the code that adds the enhancers, yes. the configuration options are called the same so it would work to have a static method that we can call from simplecms, passing the configuration array and the container and config and prefix.

then again, as you said on the mailinglist, there is a lot of duplication of services going on. for performance, you could simply disable the dynamic router in the RoutingBundle if you only have routes under the root path of the simple cms router. but this is not a good answer and keeps things too complicated.

maybe we should just make the RouteProvider and the IdPrefixListener of RoutingBundle capable of handling multiple roots? then we could drop all the duplicated services and people would configure routing things at the cmf_routing configuration point. another option would be to avoid the duplicated route tree and have people put simple cms pages under /cms/routes (or configure a different path). that way, we would not need to change anything in the RoutingBundle but could reduce complexity of services and avoid the duplicated DynamicRouter. at that point we probably should drop the configuration for the base path in simple cms and just use the one from routing. this would of course be a BC break, but 1.1 is close enough already. what do you think?

— Reply to this email directly or view it on GitHubhttps://github.com/symfony-cmf/SimpleCmsBundle/pull/89#issuecomment-27720917 .

[dbu]

the core bundle is using the "prepend configuration" functionality. but i think we should not do this for the templates by class and such configuration. this would end up quite confusing if you can configure such things in two places.

so the question is:

• a) do we drop the extra router service of simplecms and tell people to configure things on cmf_routing and to have one tree of routes and pages
• b) or we keep services duplicated but make sure the actual code creating the configuration is not duplicated, and that they do never collide with each other.

i will delay merging this pull request until we decided - if we go for a) this PR would go in the wrong direction.

[dantleech]

I vote for removing the seperate Routing class. I think the SimpleCms bundle should be like what I think the BlogBundle should be - a bundle which brings together existing CMF features and does not introduce any features of its own - it should be an example of one possible implementation.

If the CMF does not provide a way to do what we want, then it probably should ...

[benglass]

I also think that the dynamic router can be removed if the necessity for it only exists due to different basepath. I'm not sure I agree that being able to configure the basepath via simple cms is bad, I feel like thats the purpose of the prepend configuration functionality (being able to centralize configuration in a single bundle).

I do seem to recall though that there may have been another function that the dynamic router (or perhaps the nested matcher) from the simple cms provided which may have been another reason it was overloaded. I think its possible it was also detecting a locale prefix like /en and setting the current locale strategy based on that. I would have to look at the code more closely.

On Tue, Nov 5, 2013 at 7:50 AM, dantleech notifications@github.com wrote:

I vote for removing the seperate Routing class. I think the SimpleCms bundle should be like what I think the BlogBundle should be - a bundle which brings together existing CMF features and does not introduce any features of its own - it should be an example of one possible implementation.

If the CMF does not provide a way to do what we want, then it probably should ...

— Reply to this email directly or view it on GitHubhttps://github.com/symfony-cmf/SimpleCmsBundle/pull/89#issuecomment-27769756 .

[dbu]

there is indeed something about consuming the locale out of the url. maybe we can make the "thing" in RoutingBundle that does this more versatile so it can optionally do this. we would have to check if there is any other additional code or logic introduced here that we would have to port over in order to fully use the RoutingBundle router. after a quick glance over the bundle, i fail to find any additional code for the locale handling - maybe its already in the Routing component. @dantleech there is no additional code in SimpleCms, only a second service on the code from RoutingBundle

@benglass would you have time and energy to try this out?

[benglass]

Found the code I had seen

https://github.com/symfony-cmf/SimpleCmsBundle/blob/master/Doctrine/Phpcr/PageRouteProvider.php

This route provider is passed to the nested matcher which is passed to the dynamic route. Possibly the need for a custom dynamic router could be handled by adding some method calls to the existing nested matcher in the extensions for simple cms to setRouteProvider with this route provider.

I dont think I can sign on for taking care of this particular change right now.

On Wed, Nov 6, 2013 at 2:51 AM, David Buchmann notifications@github.comwrote:

there is indeed something about consuming the locale out of the url. maybe we can make the "thing" in RoutingBundle that does this more versatile so it can optionally do this. we would have to check if there is any other additional code or logic introduced here that we would have to port over in order to fully use the RoutingBundle router. after a quick glance over the bundle, i fail to find any additional code for the locale handling - maybe its already in the Routing component. @dantleech https://github.com/dantleech there is no additional code in SimpleCms, only a second service on the code from RoutingBundle

@benglass https://github.com/benglass would you have time and energy to try this out?

— Reply to this email directly or view it on GitHubhttps://github.com/symfony-cmf/SimpleCmsBundle/pull/89#issuecomment-27848429 .

[dbu]

ah, there it is! thanks for digging it up again. yeah, maybe we could check to make sure that this provider does only add options and not break anything when used in place of the normal route loader, and then have simple cms just replace this loader, if locales handling is enabled, but for all other purposes use the routing bundle services.

lets see if somebody gets around to do it this month, would be great to have this cleaned up in the next cmf release.

[dbu]

so with the refactoring, this is finally not needed anymore. thanks a lot for raising the issue and your input on it!