Extension load #589

Closed
wants to merge 3 commits into from

7 participants

@lsmith77

tag is no longer necessary, doctrine etc needs a new layer in the configs to separate dbal from orm:
app:
charset: UTF-8
error_handler: null
csrf_protection:
enabled: true
secret: xxxxxxxxxx
router: { resource: "%kernel.root_dir%/config/routing.yml" }
validation: { enabled: true, annotations: true }
templating: { engines: ['twig', 'php'] } #assets_version: SomeVersionScheme
session:
default_locale: en
lifetime: 3600
auto_start: true

# Twig Configuration
twig:
    debug:            %kernel.debug%
    strict_variables: %kernel.debug%

## Doctrine Configuration
doctrine:
    dbal:
      dbname:   test
      driver:   pdo_sqlite
      path:     %kernel.root_dir%/app.db
      logging:  %kernel.debug%

    orm:
        auto_generate_proxy_classes: %kernel.debug%
        mappings:
            HelloBundle: ~
            FOSUserBundle: ~
lsmith77 added some commits Feb 5, 2011
@lsmith77 lsmith77 implicitly load all registered bundles, all loading is now handled by…
… load(), disable loading of an extension explcitly via setting the extension config to false (for now only Yaml is implemented)
e47336e
@lsmith77 lsmith77 only call load() if its actually callable 6d05039
@lsmith77 lsmith77 also made things work for Xml 63a3371
@johnwards

From a newbie to Symfony2, this change makes the config.yml's make much more sense.

@jmikola

There were some concerns in IRC this weekend that this change might break XML; however, Lukas proposed adding a common "config" tag in the appropriate namespace to wrap the now-second-level options that were formerly top-level tags.

<doctrine:config>
    <doctrine:orm />
    <doctrine:dbal />
</doctrine:config>

And <doctrine:config /> would be pointless, as would "doctrine: ~" in YAML, since bundles are implicitly configured just by being enabled.

So this change would require adjustments to each bundle's XSD (to add a top-level wrapping tag), but I don't see a problem.

@lsmith77

furthermore if a user would forget the configure doctrine, but enables it .. he would not get a proper error message telling him that he has enabled but not configured doctrine, which is of course pointless.

@lsmith77

btw .. i assume the XSD's would now also need to be updated, which i havent done yet ..

@fabpot
Owner

Does it make sense to go one step further and only allow one extension per bundle?

If yes, why not trying to standardize the alias/namespace to the bundle name then. For instance, for FrameworkBundle, the alias would be 'framework'. For SensioBlogBundle, 'sensio_blog', ... If we do that, we will also be able to have better exception messages like "The 'SensioBlogBundle' is not enabled, but you defined settings for it under 'sensio_blog'".

@lsmith77

Sure.

@schmittjoh

I'm not sure whether any of this is really necessary, it seems only to take away flexibility.

I don't think that you will have that many third party bundles where you don't have anything to configure. Naturally, since you didn't write the bundle, you most likely have to make at least some kind of configuration for it to be usable. And for app bundles which you did write, you can simply import the services.xml (@SomeBundle/Resources/services.xml) into your app config.

How about improving the installation process of bundles (which we don't have yet) to improve the user experience?

@lsmith77

Actually about half of my Bundles do not need any config. Furthermore right now if you add a Bundle into the kernel, there is no clear error message if that Bundle requires configuration (like the Doctrine bundle). With this change we could provide a very clear error message with instructions on the next step.

@schmittjoh

Are these bundles third-party bundles or app bundles (written by yourself)? For app bundles, there shouldn't be a need for an Extension at all.

I think if we install a default configuration when a bundle is installed, it is much more helpful for the user than an implicit loading of the Extension. The default configuration could ship with comments and serve as a form of documentation. Similar to when you install a Debian package, you also get a default configuration where you can comment lines out, comment them in, etc.

@lsmith77

these are mostly app bundles. and yes there is a need for extensions for those, because they all define their own services, for controllers, for helpers etc. and many of those are indeed configurable, but always and not in all scenarios. trying to add yet another approach for configuration will just increase confusion.

as for flexibility. what is really lost? you can still pass multiple config arrays to a single load method, which can then load up any other class, split things up in multiple methods etc. so multiple extensions and extension load methods is just syntax sugar, that for the edge cases where you need it you can just implement inside the given bundle's load() method.

@schmittjoh

The question is what are the benefits of this, and is it the best way to get them?

The benefit is not having to write "namespace.tag: ~", right? So, what I'm proposing would get you this benefit plus better documentation for actually configuring the bundle by installing a default config with inline comments. Right now, we do not have an automated installation process at all, so I'm not sure if this patch is not solving a problem that should rather be solved by such an installation process.

@lsmith77

what you are proposing is yet another way to do configuration instead of making the current approach easier to use.

also the special meaning of the tag is usually hard for beginners to grasp.

finally with that fabien was suggesting it also does away with the issue of non unique alias names.

@monofone

+1, good idea to get rid of empty extension.config: ~
and also the understanding for newbies would be better,I think

@weaverryan

I'm also behind this. If we don't do this, we'll get questions daily from users that enable a bundle and then complain that the bundle doesn't work. This is a very pragmatic approach and, as Lukas points out, the error you receive when you forget to include the bundle configuration is very poor.

I'd also second Fabien's idea to enforce one extension per bundle and rename the existing aliases to something directly related to the bundle name.

These changes will make bundles and configuration less mystifying and troublesome for users, and I don't see much "cost" to the change. I'm really hoping we can make this work.

Thanks!

@schmittjoh

I'm not generally against this. All I'm saying is that we should probably wait until the bundle installation/distribution system that currently is being worked on is in place, and then revisit if we still need this change, or if it can be handled better there.

Having a default configuration installed automatically would also go more into the direction of good documentation, and you wouldn't have to copy/paste anymore.

@lsmith77

i dont think there is much sense in waiting. first up the distribution system is a big change and i am not 100% we will be able to deliver it for RC1, nor do i personally think its critical for a 2.0.

but more importantly i do not see how the proposed changes would become irrelevant. actually this is a simplification that will make whatever tools we might come up with to install + activate bundles easier. so this is all more a reason to do it now, rather than later.

@fabpot
Owner

@lsmith77: At first glance, the tests do not pass. Can you check that before I work on the other changes we've talked about in this discussion? Thanks.

@lsmith77

yeah .. i didnt update the tests at all. will work on that tomorrow before lunch, unless someone else steps in beforehand :)

@fabpot
Owner

@lsmith77: nevermind, I'm working on it right now, and with the other changes, it will be much easier for me to do evrything.

@fabpot
Owner

ok, I've fixed everything. I will now update the doc and the sandbox and merge.

@fabpot
Owner

merged.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment