Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[2.2] add possibility for bundles extensions to prepend the app configs #5566

Merged
merged 1 commit into from

5 participants

@lsmith77
Collaborator

Bug fix: #4652
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

As can be seen in the patch the extensions that should prepend the configuration are enabled automatically if they implement PrependExtensionInterface.

Just as an example, here an extension, which checks if SonataAdminBundle is available and if not disables integration with it in several Bundles. It also sets some default settings for document_class and default_document_manager_name:

diff --git a/DependencyInjection/SymfonyCmfCoreExtension.php b/DependencyInjection/SymfonyCmfCoreExtension.php
index 9f92410..c0a8dbb 100644
--- a/DependencyInjection/SymfonyCmfCoreExtension.php
+++ b/DependencyInjection/SymfonyCmfCoreExtension.php
@@ -3,11 +3,12 @@
 namespace Symfony\Cmf\Bundle\CoreBundle\DependencyInjection;

 use Symfony\Component\HttpKernel\DependencyInjection\Extension;
+use Symfony\Component\\DependencyInjection\PrependExtensionInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 use Symfony\Component\Config\FileLocator;

-class SymfonyCmfCoreExtension extends Extension
+class SymfonyCmfCoreExtension extends Extension implements PrependExtensionInterface
 {
     public function load(array $configs, ContainerBuilder $container)
     {
@@ -15,4 +16,45 @@ class SymfonyCmfCoreExtension extends Extension
         $loader->load('config.xml');
         $loader->load('services.xml');
     }
+
+    public function prepend(ContainerBuilder $container)
+    {
+        $bundles = $container->getParameter('kernel.bundles');
+        if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
+            // disable SonataDoctrinePHPCRAdminBundle admin support in Bundles
+            $config = array('use_sonata_admin' => false);
+            foreach ($container->getExtensions() as $name => $extension) {
+                switch ($name) {
+                    case 'symfony_cmf_menu':
+                    case 'symfony_cmf_routing_extra':
+                    case 'symfony_cmf_simple_cms':
+                        $container->prependExtensionConfig($name, $config);
+                        break;
+                }
+            }
+        }
+
+        // process the configuration of SymfonyCmfCoreExtension
+        $configs = $container->getExtensionConfig($this->getAlias());
+        $config = $this->processConfiguration(new Configuration(), $configs);
+        // add the default configs to various Bundles
+        foreach ($container->getExtensions() as $name => $extension) {
+            switch ($name) {
+                case 'symfony_cmf_content':
+                case 'symfony_cmf_simple_cms':
+                    $container->prependExtensionConfig($name, $config);
+                    break;
+                }
+        }
+    }
 }
.../DependencyInjection/PreProcessExtensionInterface.php
((8 lines not shown))
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\DependencyInjection;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+interface PreProcessExtensionInterface
+{
+ /**
+ * Allow an extension to pre-process the extension configurations.
+ *
+ * @param ContainerBuilder $container
+ */
+ function preProcess(ContainerBuilder $container);
@stof Collaborator
stof added a note

missing visibility

@lsmith77 Collaborator

is this a new PSR-1/2 rule?

@stof Collaborator
stof added a note

yes. It was one of the only points where the Symfony CS were incompatible with PSR-2 (symfony was saying "visibility everywhere except interfaces" and PSR-2 says "visibility everywhere") so @fabpot changed the rule for Symfony to follow PSR-2 (another point was the order of static and the visibility and it was also changed to follow PSR-2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Collaborator

I think you are giving too much power to bundles here: a bundle becomes able to modify all the config defined explicitly by the user if it wants to do it.

I think it would be safer to give them the possibility to load an additional config file which would be prepended (so that user-defined config would still win). Giving the ability to load files means passing the loader used by the kernel, and it should then be called before calling the load method on the kernel itself (to respect the order of loaded files)

@lsmith77
Collaborator

Not sure how a config file helps solve anything. I mean they can load as many config files as they want already. The key is being able to automatically apply configuration to multiple Bundles as well as enabling/disabling features based on if certain Bundles are registered.

BTW the end result in my examples is also prepended, so that user config wins. However yes this would be up to the person implementing the Bundle. We could however provide a dedicated method for prepending in addition to or instead of setExtensionConfig.

@stof
Collaborator

@lsmith77 If you can load a file with the main loader, this file can provide some app-level configuration (be it for your own bundle or others).
And your code example is indeed prepending. But imagine what would occur when someone uses this feature without knowing well how the component works: he will likely call setExtensionConfig in a first implementation, thus dropping all userland config for the bundle. Your setup does not only allow to make a file win over the userland config but makes it even easier to remove the userland config.

@lsmith77
Collaborator
@stof
Collaborator

@lsmith77 I agree about the feature, not about the way to implement it. If you allow bundles to load a file as it it were some app-level config, they would become able to provide some config for other bundles (and you could load several files depending of which bundles are enabled), but without allowing bundles to remove the userland config.

@lsmith77
Collaborator
@lsmith77
Collaborator

@stof thought about your comments, are you suggesting for a Bundle to be able to generate a config file that is prepended? in that case the current behavior would already be that if we change setExtensionConfig to just be a prependExtensionConfig .. however i am not sure if we really need this limitation since as i point out this would still by far be less dangerous than compiler passes and also i expect this to be used mainly by open source applications on top of Symfony2 rather than standard bundles.

@lsmith77
Collaborator

@lolautruche i also think this is relevant for you guys. this way you could start preconfiguring 3rd party bundles as part of your main ezPublish bundle.

@lolautruche

While I suspect a nice feature, the implementation looks obscure to me...

@lsmith77
Collaborator

The implementation of the example extension or the implementation of the actual changes proposed in this PR?

@lsmith77 lsmith77 closed this
@lsmith77 lsmith77 reopened this
@lolautruche

The example, sorry :smiley:

@lsmith77
Collaborator

The example was fairly quickly hacked together. The basic thing you need to do is fetch the config for the bundle you want to change, manipulate the config (usually by appending an array to the array of configs so that you dont affect explicit configuration) and then set it again.

As I explained to @stof it would alternative/additionally be possible to support a method that pushes a config array to the top of the array of config stack. Such a method might make the necessary code simpler.

@stof
Collaborator

@fabpot what do you think about it ?

@jrobeson

I've been porting much of an existing framework over to use more symfony components and bundles. I think that this might help some of the problems i've been having. I would really appreciate some better examples as how to one would use it (same for the cmf router, but that's another story).

@lsmith77
Collaborator

not really sure what other examples i could give. the process is quite simple:
1) determine what configuration options to add to other Bundles

for example with the following code I determine that SonataAdmin for PHPCR is not installed (which means i should disable using it in other Bundles):

$bundles = $container->getParameter('kernel.bundles');
if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {

alternatively I could simply already process the configuration and then pick all or some of these configuration options:

$configs = $container->getExtensionConfig($this->getAlias());
$config = $this->processConfiguration(new Configuration(), $configs);

2) then add these configuration to what other Bundles I feel should get these options

usually I will add these to the top of the config array stack. this means that if the user would manually set the same setting in most cases the user setting will override what the pre-processor set.

$container->unshiftExtensionConfig($name, array('use_sonata_admin' => false));
@lsmith77
Collaborator

added ContainerBuilder::unshiftExtensionConfig since this is the usual use case. with this method added it could be discussed if ContainerBuilder::setExtensionConfig is still needed or not.

...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -425,6 +425,33 @@ public function getExtensionConfig($name)
}
/**
+ * Prepend a config array to the configs of the given extension
+ *
+ * @param string $name The name of the extension
+ * @param array $config The config to set
+ *
+ * @api
+ */
+ public function unshiftExtensionConfig($name, $config)
@stof Collaborator
stof added a note

you should typehint the array

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ny/Component/DependencyInjection/ContainerBuilder.php
((10 lines not shown))
+ */
+ public function unshiftExtensionConfig($name, $config)
+ {
+ $configs = $this->getExtensionConfig($name);
+ $this->extensionConfigs[$name] = array_unshift($configs, $config);
+ }
+
+ /**
+ * Sets the configuration array for the given extension.
+ *
+ * @param string $name The name of the extension
+ * @param array $configs The configs to set
+ *
+ * @api
+ */
+ public function setExtensionConfig($name, $configs)
@stof Collaborator
stof added a note

I vote for removing this method, to avoid that a bundle remove the config set by the end user

@lsmith77 Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ny/Component/DependencyInjection/ContainerBuilder.php
((13 lines not shown))
+ if (!isset($this->extensionConfigs[$name])) {
+ $this->extensionConfigs[$name] = array();
+ }
+
+ array_unshift($this->extensionConfigs[$name], $config);
+ }
+
+ /**
+ * Sets the configuration array for the given extension.
+ *
+ * @param string $name The name of the extension
+ * @param array $configs The configs to set
+ *
+ * @api
+ */
+ public function setExtensionConfig($name, array $configs)
@lsmith77 Collaborator

this method should likely be removed. anything that cannot be done via prepending likely means that the given Bundle should be adjusted to handle merging of configuration.

@stof Collaborator
stof added a note

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -425,6 +425,36 @@ public function getExtensionConfig($name)
}
/**
+ * Prepend a config array to the configs of the given extension
+ *
+ * @param string $name The name of the extension
+ * @param array $config The config to set
+ *
+ * @api
@stof Collaborator
stof added a note

you should not tag it as part of the stable API yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Collaborator

I spoke to @fabpot today and he said that since this patch just allows you to set defaults and not really "process" the actual configuration I shouldn't call it "preProcess" so I renamed it to "prepend".

Furthermore as its just prepending @fabpot said there isnt really a need to require manually enabling it, so here is a patch to auto-enable the prepending logic:

diff --git a/src/Symfony/Component/HttpKernel/Kernel.php b/src/Symfony/Component/HttpKernel/Kernel.php
index b890fbf..7374b87 100644
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -701,8 +701,8 @@ abstract class Kernel implements KernelInterface, TerminableInterface
      */
     protected function prependExtensionConfigs(ContainerBuilder $container)
     {
-        foreach ($this->getPrependingExtensions() as $name) {
-            $extension = $container->getExtension($name);
+        foreach ($this->bundles as $bundle) {
+            $extension = $bundle->getContainerExtension();
             if ($extension instanceof PrependExtensionInterface) {
                 $extension->prepend($container);
             }
@@ -710,16 +710,6 @@ abstract class Kernel implements KernelInterface, TerminableInterface
     }

     /**
-     * Returns the ordered list of extensions that may prepend extension configurations.
-     *
-     * @return array
-     */
-    protected function getPrependingExtensions()
-    {
-        return array();
-    }
-
-    /**
      * Gets a new ContainerBuilder instance used to build the service container.
      *
      * @return ContainerBuilder
@lsmith77
Collaborator

ok .. i pondered the code some more and now i have enabled registering of the prepending extensions by default, since its now quite easy to just override the prependExtensionConfigs() method since there is almost no logic in there anymore.

@fabpot i am not 100% sure with the naming yet ..

@lsmith77
Collaborator

@fabpot if you are ok with the PR as it is now, i can do the rebase so you can merge this?

...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -466,6 +466,21 @@ public function getExtensionConfig($name)
}
/**
+ * Prepend a config array to the configs of the given extension
@fabpot Owner
fabpot added a note

should be Prepends and should end with a dot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...nel/DependencyInjection/PrependExtensionInterface.php
@@ -0,0 +1,24 @@
+<?php
@fabpot Owner
fabpot added a note

Wouldn't be better to move this extension to the DependencyInjection component?

@lsmith77 Collaborator
lsmith77 added a note

somehow the boarders of HttpKernel and DIC are not quite clear to me here, so I will do what you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/Kernel.php
@@ -649,6 +653,8 @@ protected function buildContainer()
$container->addObjectResource($this);
+ $this->prependExtensionConfigs($container, $prependingExtensions);
@fabpot Owner
fabpot added a note

Wouldn't it be better to create a compiler pass class for that logic directly in the DependencyInjection component?

@lsmith77 Collaborator
lsmith77 added a note

or put it into MergeExtensionConfigurationPass since it seems there can only be one merge pass.

speaking of which, the phpdoc for PassConfig::getMergePass() seems out of date:
https://github.com/symfony/DependencyInjection/blob/master/Compiler/PassConfig.php#L179

@lsmith77 Collaborator
lsmith77 added a note

one thing to note .. because of prependExtensionConfigs() it would be fairly easy for someone to decide if they for some reason do not want some extension to be able to prepend in the rare case when its not wanted. it would be harder if it would be a compiler pass unless we add some method that can filter the array of prepending extensions.

@fabpot Owner
fabpot added a note

Doing that in the merge extension pass seems good to me.

What you describe about the rare case does not seem like something we want to support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Collaborator

@fabpot all good now? then i will squash the commits ..

@lsmith77
Collaborator

actually looking at the full change set again i am no longer sure if it makes sense to have PrependExtensionInterface in the DI rather than the HttpKernel.

...pendencyInjection/MergeExtensionConfigurationPass.php
((8 lines not shown))
{
$this->extensions = $extensions;
+ $this->prependingExtensions = $prependingExtensions;
@fabpot Owner
fabpot added a note

What about determining the prepending extensions in the process method directly? That way, the Kernel won't have to know about them at all and everything would be part of the container.

@lsmith77 Collaborator
lsmith77 added a note

adds overhead, but i guess we still work with the assumption that we can afford overhead during the compile phase?

@lsmith77 Collaborator
lsmith77 added a note

and you would then want to move that logic into Symfony\Component\DependencyInjection\Compiler\MergeExtensionConfigurationPass?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Collaborator

@fabpot all good now?

@fabpot fabpot commented on the diff
src/Symfony/Component/HttpKernel/.gitignore
@@ -1,4 +1,5 @@
vendor/
composer.lock
phpunit.xml
-
+Tests/ProjectContainer.php
@fabpot Owner
fabpot added a note

What is it for?

@lsmith77 Collaborator
lsmith77 added a note

no clue .. but those two files were created when i run the unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Owner

The code looks good to me now. There are two remaining task before merging:

  • Is it something we need to add somewhere in the documentation?
  • Can you add a note in the DI component CHANGELOG?

Thanks.

@lsmith77
Collaborator

i have added a changelog entry and squashed the commits.
i will also work on a documentation entry, i guess i will make it a cookbook entry. not sure if it should be included in http://symfony.com/doc/2.0/cookbook/bundles/extension.html .. but imho it would better be a separate entry.

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch lsmith77/pre_process_app_config (PR #5566)
This PR was merged into the master branch.

Commits
-------

d7a1154 make it possible for bundles extensions to prepend settings into the application configuration of any Bundle

Discussion
----------

[2.2] add possibility for bundles extensions to prepend the app configs

Bug fix: #4652
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

As can be seen in the patch the extensions that should prepend the configuration are enabled automatically if they implement ``PrependExtensionInterface``.

Just as an example, here an extension, which checks if SonataAdminBundle is available and if not disables integration with it in several Bundles. It also sets some default settings for ``document_class`` and ``default_document_manager_name``:
```
diff --git a/DependencyInjection/SymfonyCmfCoreExtension.php b/DependencyInjection/SymfonyCmfCoreExtension.php
index 9f92410..c0a8dbb 100644
--- a/DependencyInjection/SymfonyCmfCoreExtension.php
+++ b/DependencyInjection/SymfonyCmfCoreExtension.php
@@ -3,11 +3,12 @@
 namespace Symfony\Cmf\Bundle\CoreBundle\DependencyInjection;

 use Symfony\Component\HttpKernel\DependencyInjection\Extension;
+use Symfony\Component\\DependencyInjection\PrependExtensionInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 use Symfony\Component\Config\FileLocator;

-class SymfonyCmfCoreExtension extends Extension
+class SymfonyCmfCoreExtension extends Extension implements PrependExtensionInterface
 {
     public function load(array $configs, ContainerBuilder $container)
     {
@@ -15,4 +16,45 @@ class SymfonyCmfCoreExtension extends Extension
         $loader->load('config.xml');
         $loader->load('services.xml');
     }
+
+    public function prepend(ContainerBuilder $container)
+    {
+        $bundles = $container->getParameter('kernel.bundles');
+        if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
+            // disable SonataDoctrinePHPCRAdminBundle admin support in Bundles
+            $config = array('use_sonata_admin' => false);
+            foreach ($container->getExtensions() as $name => $extension) {
+                switch ($name) {
+                    case 'symfony_cmf_menu':
+                    case 'symfony_cmf_routing_extra':
+                    case 'symfony_cmf_simple_cms':
+                        $container->prependExtensionConfig($name, $config);
+                        break;
+                }
+            }
+        }
+
+        // process the configuration of SymfonyCmfCoreExtension
+        $configs = $container->getExtensionConfig($this->getAlias());
+        $config = $this->processConfiguration(new Configuration(), $configs);
+        // add the default configs to various Bundles
+        foreach ($container->getExtensions() as $name => $extension) {
+            switch ($name) {
+                case 'symfony_cmf_content':
+                case 'symfony_cmf_simple_cms':
+                    $container->prependExtensionConfig($name, $config);
+                    break;
+                }
+        }
+    }
 }
```

---------------------------------------------------------------------------

by stof at 2012-09-21T21:10:00Z

I think you are giving too much power to bundles here: a bundle becomes able to modify all the config defined explicitly by the user if it wants to do it.

I think it would be safer to give them the possibility to load an additional config file which would be prepended (so that user-defined config would still win). Giving the ability to load files means passing the loader used by the kernel, and it should then be called before calling the load method on the kernel itself (to respect the order of loaded files)

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T05:50:08Z

Not sure how a config file helps solve anything. I mean they can load as many config files as they want already. The key is being able to automatically apply configuration to multiple Bundles as well as enabling/disabling features based on if certain Bundles are registered.

BTW the end result in my examples is also prepended, so that user config wins. However yes this would be up to the person implementing the Bundle. We could however provide a dedicated method for prepending in addition to or instead of ``setExtensionConfig``.

---------------------------------------------------------------------------

by stof at 2012-09-22T11:40:29Z

@lsmith77 If you can load a file with the main loader, this file can provide some app-level configuration (be it for your own bundle or others).
And your code example is indeed prepending. But imagine what would occur when someone uses this feature without knowing well how the component works: he will likely call ``setExtensionConfig`` in a first implementation, thus dropping all userland config for the bundle. Your setup does not only allow to make a file win over the userland config but makes it even easier to remove the userland config.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T18:11:29Z

but i dont get how that would help. the point is to be able for one bundle to configure other bundles before the load as this is obviously alot cleaner than trying to do the same via a compiler pass. so imho this is what is needed to encourage decoupled bundles. otherwise for example CMS or other reuseable and extensible apps will be forced to always put everything in one bundle.

---------------------------------------------------------------------------

by stof at 2012-09-22T19:23:45Z

@lsmith77 I agree about the feature, not about the way to implement it. If you allow bundles to load a file as it it were some app-level config, they would become able to provide some config for other bundles (and you could load several files depending of which bundles are enabled), but without allowing bundles to remove the userland config.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T19:50:19Z

sorry but i dont understand what you suggest. more over i dont see the problem. its already possible to seriously break stuff with compiler passes which cannot be easily enabled/disabled. this is just convenience. if it doesnt work because of some obscure combo then simply dont use it for the app since it needs to be explicitly enabled in the kernel.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-24T09:25:10Z

@stof thought about your comments, are you suggesting for a Bundle to be able to generate a config file that is prepended? in that case the current behavior would already be that if we change ``setExtensionConfig`` to just be a ``prependExtensionConfig`` .. however i am not sure if we really need this limitation since as i point out this would still by far be less dangerous than compiler passes and also i expect this to be used mainly by open source applications on top of Symfony2 rather than standard bundles.

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T13:28:29Z

@lolautruche i also think this is relevant for you guys. this way you could start preconfiguring 3rd party bundles as part of your main ezPublish bundle.

---------------------------------------------------------------------------

by lolautruche at 2012-10-13T13:57:09Z

While I suspect a nice feature, the implementation looks obscure to me...

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T17:43:02Z

The implementation of the example extension or the implementation of the actual changes proposed in this PR?

---------------------------------------------------------------------------

by lolautruche at 2012-10-13T17:46:57Z

The example, sorry :smiley:

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T17:50:38Z

The example was fairly quickly hacked together. The basic thing you need to do is fetch the config for the bundle you want to change, manipulate the config (usually by appending an array to the array of configs so that you dont affect explicit configuration) and then set it again.

As I explained to @stof it would alternative/additionally be possible to support a method that pushes a config array to the top of the array of config stack. Such a method might make the necessary code simpler.

---------------------------------------------------------------------------

by stof at 2012-10-13T21:39:07Z

@fabpot what do you think about it ?

---------------------------------------------------------------------------

by jrobeson at 2012-10-20T15:45:18Z

I've been porting much of an existing framework over to use more symfony components and bundles. I think that this might help some of the problems i've been having. I would really appreciate some better examples as how to one would use it  (same for the cmf router, but that's another story).

---------------------------------------------------------------------------

by lsmith77 at 2012-10-21T07:28:52Z

not really sure what other examples i could give. the process is quite simple:
1) determine what configuration options to add to other Bundles

for example with the following code I determine that SonataAdmin for PHPCR is not installed (which means i should disable using it in other Bundles):
```
$bundles = $container->getParameter('kernel.bundles');
if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
```

alternatively I could simply already process the configuration and then pick all or some of these configuration options:
```
$configs = $container->getExtensionConfig($this->getAlias());
$config = $this->processConfiguration(new Configuration(), $configs);
```

2) then add these configuration to what other Bundles I feel should get these options

usually I will add these to the top of the config array stack. this means that if the user would manually set the same setting in most cases the user setting will override what the pre-processor set.
```
$container->unshiftExtensionConfig($name, array('use_sonata_admin' => false));
```

---------------------------------------------------------------------------

by lsmith77 at 2012-10-24T12:52:38Z

added ``ContainerBuilder::unshiftExtensionConfig`` since this is the usual use case. with this method added it could be discussed if ``ContainerBuilder::setExtensionConfig`` is still needed or not.

---------------------------------------------------------------------------

by lsmith77 at 2012-11-24T14:48:44Z

I spoke to @fabpot today and he said that since this patch just allows you to set defaults and not really "process" the actual configuration I shouldn't call it "preProcess" so I renamed it to "prepend".

Furthermore as its just prepending @fabpot said there isnt really a need to require manually enabling it, so here is a patch to auto-enable the prepending logic:
```
diff --git a/src/Symfony/Component/HttpKernel/Kernel.php b/src/Symfony/Component/HttpKernel/Kernel.php
index b890fbf..7374b87 100644
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -701,8 +701,8 @@ abstract class Kernel implements KernelInterface, TerminableInterface
      */
     protected function prependExtensionConfigs(ContainerBuilder $container)
     {
-        foreach ($this->getPrependingExtensions() as $name) {
-            $extension = $container->getExtension($name);
+        foreach ($this->bundles as $bundle) {
+            $extension = $bundle->getContainerExtension();
             if ($extension instanceof PrependExtensionInterface) {
                 $extension->prepend($container);
             }
@@ -710,16 +710,6 @@ abstract class Kernel implements KernelInterface, TerminableInterface
     }

     /**
-     * Returns the ordered list of extensions that may prepend extension configurations.
-     *
-     * @return array
-     */
-    protected function getPrependingExtensions()
-    {
-        return array();
-    }
-
-    /**
      * Gets a new ContainerBuilder instance used to build the service container.
      *
      * @return ContainerBuilder
```

---------------------------------------------------------------------------

by lsmith77 at 2012-11-25T19:31:01Z

ok .. i pondered the code some more and now i have enabled registering of the prepending extensions by default, since its now quite easy to just override the ``prependExtensionConfigs()`` method since there is almost no logic in there anymore.

@fabpot i am not 100% sure with the naming yet ..

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T14:03:43Z

@fabpot if you are ok with the PR as it is now, i can do the rebase so you can merge this?

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T18:30:29Z

@fabpot all good now? then i will squash the commits ..

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T18:34:50Z

actually looking at the full change set again i am no longer sure if it makes sense to have ``PrependExtensionInterface`` in the DI rather than the HttpKernel.

---------------------------------------------------------------------------

by lsmith77 at 2012-12-07T09:21:14Z

@fabpot all good now?

---------------------------------------------------------------------------

by fabpot at 2012-12-07T09:37:52Z

The code looks good to me now. There are two remaining task before merging:

* Is it something we need to add somewhere in the documentation?
* Can you add a note in the DI component CHANGELOG?

Thanks.

---------------------------------------------------------------------------

by lsmith77 at 2012-12-07T09:49:17Z

i have added a changelog entry and squashed the commits.
i will also work on a documentation entry, i guess i will make it a cookbook entry. not sure if it should be included in http://symfony.com/doc/2.0/cookbook/bundles/extension.html .. but imho it would better be a separate entry.
79b4ca6
@fabpot fabpot merged commit d7a1154 into symfony:master
@stof
Collaborator

@lsmith77 As it is also available when using the DI component standalone, I think it should go in the component doc rather than the cookbook

@ghost

@lsmith77 - has this been documented? I didnt see a reference to a documentation ticket in the PR description.

@lsmith77
Collaborator

@drak i am working on the docs atm .. will submit a PR today.

@lsmith77
Collaborator

@stof: ok .. i will add a note in the DIC docs, but i will show a practical example in a cookbook entry.

@lsmith77 lsmith77 referenced this pull request in symfony/symfony-docs
Merged

added docs for PrependExtensionInterface #2007

@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch lsmith77/pre_process_app_config (PR #5566)
This PR was merged into the master branch.

Commits
-------

d7a1154 make it possible for bundles extensions to prepend settings into the application configuration of any Bundle

Discussion
----------

[2.2] add possibility for bundles extensions to prepend the app configs

Bug fix: #4652
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

As can be seen in the patch the extensions that should prepend the configuration are enabled automatically if they implement ``PrependExtensionInterface``.

Just as an example, here an extension, which checks if SonataAdminBundle is available and if not disables integration with it in several Bundles. It also sets some default settings for ``document_class`` and ``default_document_manager_name``:
```
diff --git a/DependencyInjection/SymfonyCmfCoreExtension.php b/DependencyInjection/SymfonyCmfCoreExtension.php
index 9f92410..c0a8dbb 100644
--- a/DependencyInjection/SymfonyCmfCoreExtension.php
+++ b/DependencyInjection/SymfonyCmfCoreExtension.php
@@ -3,11 +3,12 @@
 namespace Symfony\Cmf\Bundle\CoreBundle\DependencyInjection;

 use Symfony\Component\HttpKernel\DependencyInjection\Extension;
+use Symfony\Component\\DependencyInjection\PrependExtensionInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 use Symfony\Component\Config\FileLocator;

-class SymfonyCmfCoreExtension extends Extension
+class SymfonyCmfCoreExtension extends Extension implements PrependExtensionInterface
 {
     public function load(array $configs, ContainerBuilder $container)
     {
@@ -15,4 +16,45 @@ class SymfonyCmfCoreExtension extends Extension
         $loader->load('config.xml');
         $loader->load('services.xml');
     }
+
+    public function prepend(ContainerBuilder $container)
+    {
+        $bundles = $container->getParameter('kernel.bundles');
+        if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
+            // disable SonataDoctrinePHPCRAdminBundle admin support in Bundles
+            $config = array('use_sonata_admin' => false);
+            foreach ($container->getExtensions() as $name => $extension) {
+                switch ($name) {
+                    case 'symfony_cmf_menu':
+                    case 'symfony_cmf_routing_extra':
+                    case 'symfony_cmf_simple_cms':
+                        $container->prependExtensionConfig($name, $config);
+                        break;
+                }
+            }
+        }
+
+        // process the configuration of SymfonyCmfCoreExtension
+        $configs = $container->getExtensionConfig($this->getAlias());
+        $config = $this->processConfiguration(new Configuration(), $configs);
+        // add the default configs to various Bundles
+        foreach ($container->getExtensions() as $name => $extension) {
+            switch ($name) {
+                case 'symfony_cmf_content':
+                case 'symfony_cmf_simple_cms':
+                    $container->prependExtensionConfig($name, $config);
+                    break;
+                }
+        }
+    }
 }
```

---------------------------------------------------------------------------

by stof at 2012-09-21T21:10:00Z

I think you are giving too much power to bundles here: a bundle becomes able to modify all the config defined explicitly by the user if it wants to do it.

I think it would be safer to give them the possibility to load an additional config file which would be prepended (so that user-defined config would still win). Giving the ability to load files means passing the loader used by the kernel, and it should then be called before calling the load method on the kernel itself (to respect the order of loaded files)

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T05:50:08Z

Not sure how a config file helps solve anything. I mean they can load as many config files as they want already. The key is being able to automatically apply configuration to multiple Bundles as well as enabling/disabling features based on if certain Bundles are registered.

BTW the end result in my examples is also prepended, so that user config wins. However yes this would be up to the person implementing the Bundle. We could however provide a dedicated method for prepending in addition to or instead of ``setExtensionConfig``.

---------------------------------------------------------------------------

by stof at 2012-09-22T11:40:29Z

@lsmith77 If you can load a file with the main loader, this file can provide some app-level configuration (be it for your own bundle or others).
And your code example is indeed prepending. But imagine what would occur when someone uses this feature without knowing well how the component works: he will likely call ``setExtensionConfig`` in a first implementation, thus dropping all userland config for the bundle. Your setup does not only allow to make a file win over the userland config but makes it even easier to remove the userland config.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T18:11:29Z

but i dont get how that would help. the point is to be able for one bundle to configure other bundles before the load as this is obviously alot cleaner than trying to do the same via a compiler pass. so imho this is what is needed to encourage decoupled bundles. otherwise for example CMS or other reuseable and extensible apps will be forced to always put everything in one bundle.

---------------------------------------------------------------------------

by stof at 2012-09-22T19:23:45Z

@lsmith77 I agree about the feature, not about the way to implement it. If you allow bundles to load a file as it it were some app-level config, they would become able to provide some config for other bundles (and you could load several files depending of which bundles are enabled), but without allowing bundles to remove the userland config.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-22T19:50:19Z

sorry but i dont understand what you suggest. more over i dont see the problem. its already possible to seriously break stuff with compiler passes which cannot be easily enabled/disabled. this is just convenience. if it doesnt work because of some obscure combo then simply dont use it for the app since it needs to be explicitly enabled in the kernel.

---------------------------------------------------------------------------

by lsmith77 at 2012-09-24T09:25:10Z

@stof thought about your comments, are you suggesting for a Bundle to be able to generate a config file that is prepended? in that case the current behavior would already be that if we change ``setExtensionConfig`` to just be a ``prependExtensionConfig`` .. however i am not sure if we really need this limitation since as i point out this would still by far be less dangerous than compiler passes and also i expect this to be used mainly by open source applications on top of Symfony2 rather than standard bundles.

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T13:28:29Z

@lolautruche i also think this is relevant for you guys. this way you could start preconfiguring 3rd party bundles as part of your main ezPublish bundle.

---------------------------------------------------------------------------

by lolautruche at 2012-10-13T13:57:09Z

While I suspect a nice feature, the implementation looks obscure to me...

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T17:43:02Z

The implementation of the example extension or the implementation of the actual changes proposed in this PR?

---------------------------------------------------------------------------

by lolautruche at 2012-10-13T17:46:57Z

The example, sorry :smiley:

---------------------------------------------------------------------------

by lsmith77 at 2012-10-13T17:50:38Z

The example was fairly quickly hacked together. The basic thing you need to do is fetch the config for the bundle you want to change, manipulate the config (usually by appending an array to the array of configs so that you dont affect explicit configuration) and then set it again.

As I explained to @stof it would alternative/additionally be possible to support a method that pushes a config array to the top of the array of config stack. Such a method might make the necessary code simpler.

---------------------------------------------------------------------------

by stof at 2012-10-13T21:39:07Z

@fabpot what do you think about it ?

---------------------------------------------------------------------------

by jrobeson at 2012-10-20T15:45:18Z

I've been porting much of an existing framework over to use more symfony components and bundles. I think that this might help some of the problems i've been having. I would really appreciate some better examples as how to one would use it  (same for the cmf router, but that's another story).

---------------------------------------------------------------------------

by lsmith77 at 2012-10-21T07:28:52Z

not really sure what other examples i could give. the process is quite simple:
1) determine what configuration options to add to other Bundles

for example with the following code I determine that SonataAdmin for PHPCR is not installed (which means i should disable using it in other Bundles):
```
$bundles = $container->getParameter('kernel.bundles');
if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
```

alternatively I could simply already process the configuration and then pick all or some of these configuration options:
```
$configs = $container->getExtensionConfig($this->getAlias());
$config = $this->processConfiguration(new Configuration(), $configs);
```

2) then add these configuration to what other Bundles I feel should get these options

usually I will add these to the top of the config array stack. this means that if the user would manually set the same setting in most cases the user setting will override what the pre-processor set.
```
$container->unshiftExtensionConfig($name, array('use_sonata_admin' => false));
```

---------------------------------------------------------------------------

by lsmith77 at 2012-10-24T12:52:38Z

added ``ContainerBuilder::unshiftExtensionConfig`` since this is the usual use case. with this method added it could be discussed if ``ContainerBuilder::setExtensionConfig`` is still needed or not.

---------------------------------------------------------------------------

by lsmith77 at 2012-11-24T14:48:44Z

I spoke to @fabpot today and he said that since this patch just allows you to set defaults and not really "process" the actual configuration I shouldn't call it "preProcess" so I renamed it to "prepend".

Furthermore as its just prepending @fabpot said there isnt really a need to require manually enabling it, so here is a patch to auto-enable the prepending logic:
```
diff --git a/src/Symfony/Component/HttpKernel/Kernel.php b/src/Symfony/Component/HttpKernel/Kernel.php
index b890fbf..7374b87 100644
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -701,8 +701,8 @@ abstract class Kernel implements KernelInterface, TerminableInterface
      */
     protected function prependExtensionConfigs(ContainerBuilder $container)
     {
-        foreach ($this->getPrependingExtensions() as $name) {
-            $extension = $container->getExtension($name);
+        foreach ($this->bundles as $bundle) {
+            $extension = $bundle->getContainerExtension();
             if ($extension instanceof PrependExtensionInterface) {
                 $extension->prepend($container);
             }
@@ -710,16 +710,6 @@ abstract class Kernel implements KernelInterface, TerminableInterface
     }

     /**
-     * Returns the ordered list of extensions that may prepend extension configurations.
-     *
-     * @return array
-     */
-    protected function getPrependingExtensions()
-    {
-        return array();
-    }
-
-    /**
      * Gets a new ContainerBuilder instance used to build the service container.
      *
      * @return ContainerBuilder
```

---------------------------------------------------------------------------

by lsmith77 at 2012-11-25T19:31:01Z

ok .. i pondered the code some more and now i have enabled registering of the prepending extensions by default, since its now quite easy to just override the ``prependExtensionConfigs()`` method since there is almost no logic in there anymore.

@fabpot i am not 100% sure with the naming yet ..

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T14:03:43Z

@fabpot if you are ok with the PR as it is now, i can do the rebase so you can merge this?

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T18:30:29Z

@fabpot all good now? then i will squash the commits ..

---------------------------------------------------------------------------

by lsmith77 at 2012-12-05T18:34:50Z

actually looking at the full change set again i am no longer sure if it makes sense to have ``PrependExtensionInterface`` in the DI rather than the HttpKernel.

---------------------------------------------------------------------------

by lsmith77 at 2012-12-07T09:21:14Z

@fabpot all good now?

---------------------------------------------------------------------------

by fabpot at 2012-12-07T09:37:52Z

The code looks good to me now. There are two remaining task before merging:

* Is it something we need to add somewhere in the documentation?
* Can you add a note in the DI component CHANGELOG?

Thanks.

---------------------------------------------------------------------------

by lsmith77 at 2012-12-07T09:49:17Z

i have added a changelog entry and squashed the commits.
i will also work on a documentation entry, i guess i will make it a cookbook entry. not sure if it should be included in http://symfony.com/doc/2.0/cookbook/bundles/extension.html .. but imho it would better be a separate entry.
8099ebe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 7, 2012
  1. @lsmith77

    make it possible for bundles extensions to prepend settings into the …

    lsmith77 authored
    …application configuration of any Bundle
This page is out of date. Refresh to see the latest.
View
6 src/Symfony/Component/DependencyInjection/CHANGELOG.md
@@ -1,6 +1,12 @@
CHANGELOG
=========
+2.2.0
+-----
+
+ * added PrependExtensionInterface (to be able to allow extensions to prepend
+ application configuration settings for any Bundle)
+
2.1.0
-----
View
6 src/Symfony/Component/DependencyInjection/Compiler/MergeExtensionConfigurationPass.php
@@ -29,6 +29,12 @@ public function process(ContainerBuilder $container)
$definitions = $container->getDefinitions();
$aliases = $container->getAliases();
+ foreach ($container->getExtensions() as $extension) {
+ if ($extension instanceof PrependExtensionInterface) {
+ $extension->prepend($container);
+ }
+ }
+
foreach ($container->getExtensions() as $name => $extension) {
if (!$config = $container->getExtensionConfig($name)) {
// this extension was not called
View
24 src/Symfony/Component/DependencyInjection/Compiler/PrependExtensionInterface.php
@@ -0,0 +1,24 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+interface PrependExtensionInterface
+{
+ /**
+ * Allow an extension to prepend the extension configurations.
+ *
+ * @param ContainerBuilder $container
+ */
+ public function prepend(ContainerBuilder $container);
+}
View
15 src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -466,6 +466,21 @@ public function getExtensionConfig($name)
}
/**
+ * Prepends a config array to the configs of the given extension.
+ *
+ * @param string $name The name of the extension
+ * @param array $config The config to set
+ */
+ public function prependExtensionConfig($name, array $config)
+ {
+ if (!isset($this->extensionConfigs[$name])) {
+ $this->extensionConfigs[$name] = array();
+ }
+
+ array_unshift($this->extensionConfigs[$name], $config);
+ }
+
+ /**
* Compiles the container.
*
* This method passes the container to compiler
View
22 src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -548,6 +548,28 @@ public function testThrowsExceptionWhenSetDefinitionOnAFrozenContainer()
$container->compile();
$container->setDefinition('a', new Definition());
}
+
+ /**
+ * @covers Symfony\Component\DependencyInjection\ContainerBuilder::getExtensionConfig
+ * @covers Symfony\Component\DependencyInjection\ContainerBuilder::prependExtensionConfig
+ */
+ public function testExtensionConfig()
+ {
+ $container = new ContainerBuilder();
+
+ $configs = $container->getExtensionConfig('foo');
+ $this->assertEmpty($configs);
+
+ $first = array('foo' => 'bar');
+ $container->prependExtensionConfig('foo', $first);
+ $configs = $container->getExtensionConfig('foo');
+ $this->assertEquals(array($first), $configs);
+
+ $second = array('ding' => 'dong');
+ $container->prependExtensionConfig('foo', $second);
+ $configs = $container->getExtensionConfig('foo');
+ $this->assertEquals(array($second, $first), $configs);
+ }
}
class FooClass {}
View
3  src/Symfony/Component/HttpKernel/.gitignore
@@ -1,4 +1,5 @@
vendor/
composer.lock
phpunit.xml
-
+Tests/ProjectContainer.php
@fabpot Owner
fabpot added a note

What is it for?

@lsmith77 Collaborator
lsmith77 added a note

no clue .. but those two files were created when i run the unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+Tests/classes.map
Something went wrong with that request. Please try again.