Put enabled bundles in a configuration file #598

gnugat · 2013-10-17T20:33:43Z

Enabled bundles in a configuration file

The problem

The simple task of enabling programmatically a bundle is too complex: it involves Reflection and Zend engine's lexical scanner.

The proposal

Put enabled bundles in a configuration file:

    <?php
    // File: app/AppKernel.php

    class AppKernel
    {
        public function registerBundles()
        {
            $bundles = $this->getConfiguredBundles(__DIR__.'/config/enabled_bundles.yml');

            // Bundles can still be enabled manually:
            // $bundles[] = new Acme\DemoBundle\AcmeDemoBundle($this);

            return $bundles;
        }

        protected function getConfiguredBundles($configurationPath)
        {
            return Yaml::parse(file_get_contents($configurationPath))['enabled_bundles']; // Replace with better logic
        }
    }

    # File: app/config/enabled_bundles.yml
    enabled_bundles:
        - Acme\DemoBundle\AcmeDemoBundle

Impact on the community

Bundles installation documentation

Current bundle installation instructions will be still valid.

KernelManipulator

BC break on KernelManipulator implementations, but easy to fix (manipulating a YAML file instead of actually modifying the source code).
It all depends on how many people would be affected.
Except the SensioGeneratorBundle, do you know any library that would be affected?

What do you think?

This is just a draft, so if you have any improvements, be sure to propose them!
If the idea of putting enabled bundles in a configuration file is terribly silly, go on and say it: I'd like to know your arguments.

Currently, the easiest way to enable a bundle is to manually edit the AppKernel (vs programmatically). Enabling a bundle programmatically involves [Reflection and Zend engine's lexical scanner](https://github.com/sensiolabs/SensioGeneratorBundle/blob/9e29c8e898b98da6a3b63de80ad7679305262308/Manipulator/KernelManipulator.php#L46), how ridiculously complex! So here's my proposal: use a configuration file to register bundles. We have a configuration file which allows to register unrestricted bundles (bundles available in every environment) and restricted bundles (bundles available in the given set of environments).

stof · 2013-10-17T20:36:30Z

app/AppKernel.php

+        $yaml = new Parser();
+        $bundlesConfig = $yaml->parse(file_get_contents(__DIR__.'/config/enabled_bundles.yml'));
+        foreach ($bundlesConfig['unrestricted_bundles'] as $bundle) {
+            $bundles[] = new $bundle();


This makes it impossible to use bundles requiring to pass the kernel as first argument (i.e. some JMS bundles)

I don't really see what you mean, could you give me some links so I can look at some code?

Ok I see, it's about annotations, right?

Is there anyway to work around?

Now that I think of it, "impossible" is a bit too strong: the AppKernel file is still open to modifications, so the good old way would stay possible (adding $bundles[] = new Acme\Bundle\DemoBundle($this); before the return statement).

Or if you like to make things complex, you could add a bit of reflexivity to check if the bundle needs an argument, and pass it $this.
Or you could add some more options in the YAML file to provide constructor arguments.

peterjmit · 2013-10-17T20:37:39Z

Bundles being enabled in configuration vs in AppKernel makes a lot more sense to me.

However we don't want to be parsing that yaml file on each request (as with 71f9989), it would need to be part of a caching pass

gnugat · 2013-10-17T20:52:31Z

@peterjmit that makes sense. Could you provide us with a prototype?

fabpot · 2013-10-17T20:52:34Z

This has already been discussed in the past and rejected.

gnugat · 2013-10-17T20:55:51Z

Oh, I didn't know (couldn't find an issue related to this, and google contains too much noise). Could you provide us with a link so we can be reminded of the reasons?

emerick42 · 2013-10-17T21:30:17Z

Hmm, if it's not possible to update the kernel to load bundles with a configuration file, maybe you can create a bundle that reads a configuration file (it can have dependencies on main config files parser) to automatically add other bundle. That way you will have only the BundleLoader registered in your appKernel, and every other bundle you want are loaded following your configuration file.

This should solve the problem mentioned by Stof (about bundles that require kernel as first element).

gnugat · 2013-10-18T06:14:11Z

@emerick42 as a matter of fact, this PR came to my mind because I'm already working on a bundle that would help to install bundles.
It's just that I thought that the registration of bundles sounded more like configuration. However it seems that it has already been discussed and declined :( . I'd really like to know the reasons.

The problem mentioned by Stof isn't really one, as the AppKernel file is still open to modifications (nothing prevents you to add a $bundles[] = new Acme\Bundle\DemoBundle($this); line just before the return statement :) .

Allows to show that bundles can still be manually registered (for example bundles that have a dependency on the AppKernel).

* usage of convenient wrapper Yaml::parse * combined usage of: * instanciator: anonymous function to convert fully qualified classnames into instances * array_map to apply instanciator * array_merge for restricted bundles

gnugat · 2013-10-18T20:31:09Z

I updated the code to show that bundles can still be manually registered (for example bundles that have a dependency on the AppKernel). Also refactored a bit.

lyrixx · 2013-10-19T09:41:34Z

@gnugat

(you are two user ? ;))
Your current implementation will parse the yaml file for each request. This will slow down symfony.
As @stof said it, it's now harder to give an argument to the kernel. Yes you can use both yaml and current php way. But It adds more complexity: it scatters the configuration (and the config is IMHO already too scattered in a classic app)
I guess you open this pr to make simpler the code of your bundle, but you can re-use the KernelManipulator ?

gnugat · 2013-10-19T15:56:45Z

hey @lyrixx

oops, sorry. Do you think it's worth re-writing the history?
I gave some thought to @peterjmit comment, and I think it won't be possible to do otherwise at this stage of the kernel :/ . In the end, we can only think of a configuration that would generate the AppKernel has it already is currently.
you're right, scattering the configuration is of no interests.
if you recall my bundle already works and uses the KernelManipulator, so any decision wouldn't affect it ;)

I just felt that adding a bundle to the application was more a configuration thing... But I guess I was wrong.

Do you think the AppKernel generation would be wise? If you do I can try something in this direction.

lyrixx · 2013-10-19T16:21:06Z

For now, I don't have any thought ;) I guess we can wait for symfony/symfony#9341

Peter-Rene-Menges · 2013-11-07T12:23:22Z

Like I wrote, the process of registering bundles is actually a registration, so it should not need to be coded.
Because it's a registration process, we could use the registry The Kernel then asks the registry, which bundles to load , under which namespaces they exists and further. The registry can implement a cache, so it could be pretty fast.Write - Access to cache which is providing the information to the registry could be given to both the Console and programmatically. So this is a light weighted decoupling of the process of registering bundels from the kernel, while it shoudl be fast and reliable. Other bundles that need this information could query the registry too.

fabpot · 2014-03-14T07:35:00Z

Closing in favor of #639, which a meta-issue about things we might improve in the future but for which we don't have the solution yet.

stof reviewed Oct 17, 2013
View reviewed changes

Loïc Chardonnet added 2 commits October 18, 2013 09:45

Created enabled bundles import method

e87ac64

Allows to show that bundles can still be manually registered (for example bundles that have a dependency on the AppKernel).

Refactored getConfiguredBundles

82cf52c

* usage of convenient wrapper Yaml::parse * combined usage of: * instanciator: anonymous function to convert fully qualified classnames into instances * array_map to apply instanciator * array_merge for restricted bundles

gnugat mentioned this pull request Nov 7, 2013

Direct Registration of new Bundels / Adding bundles required by composer automatically symfony/symfony#9457

Closed

fabpot closed this Mar 14, 2014

This was referenced Jan 23, 2015

[WIP][DX] Automatic registration of installed bundles #695

Closed

[Kernel] [Bundle] Setting up bundle dependencies symfony/symfony#13505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put enabled bundles in a configuration file #598

Put enabled bundles in a configuration file #598

gnugat commented Oct 17, 2013

stof Oct 17, 2013

gnugat Oct 17, 2013

gnugat Oct 17, 2013

gnugat Oct 17, 2013

peterjmit commented Oct 17, 2013

gnugat commented Oct 17, 2013

fabpot commented Oct 17, 2013

gnugat commented Oct 17, 2013

emerick42 commented Oct 17, 2013

gnugat commented Oct 18, 2013

gnugat commented Oct 18, 2013

lyrixx commented Oct 19, 2013

gnugat commented Oct 19, 2013

lyrixx commented Oct 19, 2013

Peter-Rene-Menges commented Nov 7, 2013

fabpot commented Mar 14, 2014

Put enabled bundles in a configuration file #598

Put enabled bundles in a configuration file #598

Conversation

gnugat commented Oct 17, 2013

Enabled bundles in a configuration file

The problem

The proposal

Impact on the community

Bundles installation documentation

KernelManipulator

What do you think?

stof Oct 17, 2013

Choose a reason for hiding this comment

gnugat Oct 17, 2013

Choose a reason for hiding this comment

gnugat Oct 17, 2013

Choose a reason for hiding this comment

gnugat Oct 17, 2013

Choose a reason for hiding this comment

peterjmit commented Oct 17, 2013

gnugat commented Oct 17, 2013

fabpot commented Oct 17, 2013

gnugat commented Oct 17, 2013

emerick42 commented Oct 17, 2013

gnugat commented Oct 18, 2013

gnugat commented Oct 18, 2013

lyrixx commented Oct 19, 2013

gnugat commented Oct 19, 2013

lyrixx commented Oct 19, 2013

Peter-Rene-Menges commented Nov 7, 2013

fabpot commented Mar 14, 2014