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

implement PrependExtensionInterface #48

Closed
lsmith77 opened this issue Jun 4, 2013 · 9 comments · Fixed by #79
Closed

implement PrependExtensionInterface #48

lsmith77 opened this issue Jun 4, 2013 · 9 comments · Fixed by #79
Assignees
Milestone

Comments

@lsmith77
Copy link
Member

lsmith77 commented Jun 4, 2013

see also http://symfony.com/doc/master/components/dependency_injection/compilation.html#prepending-configuration-passed-to-the-extension

the idea would be to make it optionally possible to configure all the key aspects of the core bundles via the CoreBundle.

this would be a nice to have for 1.0 but not a must have as it should be possible to do this without BC breaks

@lsmith77
Copy link
Member Author

lsmith77 commented Jun 4, 2013

The goal would be to be able to replace all of this:

cmf_routing:
    route_basepath: /foo/routes
    dynamic:
        locales: %locales%

cmf_create:
    phpcr_odm: true

cmf_content:
    multilang:
      locales: %locales%

cmf_menu:
    menu_basepath: /foo/menu
    multilang:
      locales: %locales%

cmf_blog:
    blog_basepath: /foo/content

With:

cmf_core:
    phpcr_odm: true
    basepath: /foo
    multilang:
      locales: %locales%

@dantleech
Copy link
Member

Should we change basepath to base_path ?

@dbu
Copy link
Member

dbu commented Jun 6, 2013

to me route_basepath is more readable than route_base_path (then i don't know if its the path of the "route base" or the "base path" of the route"). and if we leave that, the cmf_core parameter should not have the underscore for consistency. i wonder if we should call it site_basepath but i think that could be a lie, in a multisite you might still share some of the trees.

do we need to split the cmf_menu basepath parameter (and route and so on)? if we inject cmf_core.basepath into the bundles, they would still need to know that they put their stuff under "menu" node (resp route, ...). and if x_basepath is set explicitly the param from cmf_core is ignored.

@dantleech
Copy link
Member

But then basepath would seem to be the exception in naming conventions, it is two words, "base" and "path" - it logically translates to "base_path". In the case of the individual bundles we can drop "route", "menu", etc.

cmf_menu:
    base_path: /foo

cmf_routing:
    base_path: /bar

But as you say we still need to have something specifying "menu" on top of the base path provided by the core bundle.

What if we have 2 parameters, "base_path" and "path". The base path would be optional and would prefix the path if supplied. Having a separate parameter would also enable a listener to automatically set the base path in case of a multi-site setup.

cmf_core:
    base_path: /cms

cmf_menu:
    path: /menu

@dbu
Copy link
Member

dbu commented Jun 6, 2013

yeah that makes sense indeed. can we call the second parameter something else than just path? maybe base_path and data_path? local_name is wrong as it should be possible to have a subpath that is more than simply a name.

@dantleech
Copy link
Member

actually, these settings are sonata specific are they not?

@cordoval
Copy link
Contributor

cordoval commented Jun 6, 2013

this would be a multiple PR or just for Core? @lsmith77

@lsmith77
Copy link
Member Author

lsmith77 commented Jun 6, 2013

the idea here is to have one bundle that effort effectively configures multiple other bundles

@dbu
Copy link
Member

dbu commented Jun 7, 2013

@dantleech no its not only sonata. its also for the menu provider to know where to find menus, for route provider to know where to look for routes. content and blocks is more open as they are usually referenced.

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 a pull request may close this issue.

4 participants