[DI] Auto register extension configuration classes as a resource #21057

ro0NL · 2016-12-26T23:07:38Z

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Auto-register an extension configuration class as a resource from a compiler pass; not implicitly by the base extension class.

Causing some extensions to register its configuration, whereas others dont (e.g. framework bundle).

Fixes consistent cache invalidation whenever a configuration definition changes.

nicolas-grekas · 2016-12-27T13:52:31Z

src/Symfony/Component/DependencyInjection/Extension/Extension.php

@@ -82,13 +81,8 @@ public function getConfiguration(array $config, ContainerBuilder $container)
        $namespace = $reflected->getNamespaceName();

        $class = $namespace.'\\Configuration';
-        if (class_exists($class)) {
-            $r = new \ReflectionClass($class);
-            $container->addResource(new FileResource($r->getFileName()));


does it hurt to keep this one so that people not using MergeExtensionConfigurationPass don't experience a breakage?

It hurts SF by default :) in terms of registering a duplicate resource. Imo. it just dont belong here..

breakage >>> duplicate resource :)

Understand.. but it's a breakage in behavior.. ie. it doesnt qualify a technical "BC Break".

I'd prefer master+changelog on those, rather then keeping quirky code. Trying to do all.

Note that duplicate resources are not actually stored in the cache.

public function getResources() { return array_unique($this->resources); }

Fair enough. I still think the "change" is considerable, but we can keep things as is here. Doing things twice by default.

ro0NL · 2016-12-28T10:15:37Z

Updated to 2.7, reverted base Extension.

However, we could update built in bundles to bypass Extension::getConfiguration right? Initializing the configuration class explicitly per extension (like the framework bundle already does).

That should improve something.

nicolas-grekas · 2016-12-28T11:49:56Z

This should be considered as a bug, isn't it? What about adding a test case?

xabbuh · 2016-12-28T23:34:38Z

src/Symfony/Component/DependencyInjection/Compiler/MergeExtensionConfigurationPass.php

@@ -47,6 +48,9 @@ public function process(ContainerBuilder $container)
            $tmpContainer = new ContainerBuilder($container->getParameterBag());
            $tmpContainer->setResourceTracking($container->isTrackingResources());
            $tmpContainer->addObjectResource($extension);
+            if ($extension instanceof ConfigurationExtensionInterface && null !== $configuration = $extension->getConfiguration($config, $tmpContainer)) {
+                $tmpContainer->addObjectResource($configuration);


Now, we have two resources being registered when getConfiguration() is used.

Correct :) see #21057 (comment)

I first targeted master, doing it only here (with in mind we do a changelog on behavioral change).

However now we target 2.7, in order to fix the behavior in lower branches. Indeed giving multiple resources by default. But's not really a big a deal, only weird looking at it :)

I still think we need to do

Initializing the configuration class explicitly per extension

In core.

Hm yeah, thinking about this again two resources shouldn't really hurt.

ro0NL · 2016-12-29T09:43:23Z

Test added.

What about core bundles with different implementations regarding getConfiguration?

Then again, maye this should work vice-versa, updating bundles to consistently track resources from getConfiguration vs. keeping it as is.

I think moving the responsibility away from getConfiguration makes sense.

Im not sure.

xabbuh · 2016-12-29T10:08:48Z

👍

Status: Reviewed

ro0NL · 2016-12-29T10:15:14Z

👍 for keeping thing as is from here.

nicolas-grekas

👍

fabpot · 2017-02-14T18:32:07Z

Thank you @ro0NL.

…source (ro0NL) This PR was merged into the 2.7 branch. Discussion ---------- [DI] Auto register extension configuration classes as a resource | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Auto-register an extension configuration class as a resource from a compiler pass; not implicitly by the base extension class. Causing some extensions to register its configuration, whereas others dont (e.g. framework bundle). Fixes consistent cache invalidation whenever a configuration definition changes. Commits ------- 142416b [DI] Auto register extension configuration classes as a resource

carsonbot added Status: Needs Review DependencyInjection labels Dec 26, 2016

nicolas-grekas reviewed Dec 27, 2016

View reviewed changes

ro0NL changed the base branch from master to 2.7 December 28, 2016 10:12

xabbuh reviewed Dec 28, 2016

View reviewed changes

[DI] Auto register extension configuration classes as a resource

142416b

xabbuh added the Bug label Dec 29, 2016

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 29, 2016

xabbuh added this to the 2.7 milestone Dec 29, 2016

nicolas-grekas approved these changes Feb 14, 2017

View reviewed changes

fabpot merged commit 142416b into symfony:2.7 Feb 14, 2017

ro0NL deleted the di/auto-register-resource branch February 14, 2017 19:12

fabpot mentioned this pull request Feb 17, 2017

Release v3.2.4 #21640

Merged

chosroes mentioned this pull request Feb 17, 2017

Fix configuration validation since symfony 3.2.4: #21057 1up-lab/OneupFlysystemBundle#130

Merged

This was referenced Mar 6, 2017

Release v2.7.25 #21884

Merged

Release v2.8.18 #21885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Auto register extension configuration classes as a resource #21057

[DI] Auto register extension configuration classes as a resource #21057

ro0NL commented Dec 26, 2016 •

edited

nicolas-grekas Dec 27, 2016

ro0NL Dec 27, 2016

nicolas-grekas Dec 27, 2016

ro0NL Dec 27, 2016

sstok Dec 28, 2016 •

edited

ro0NL Dec 28, 2016

ro0NL commented Dec 28, 2016

nicolas-grekas commented Dec 28, 2016

xabbuh Dec 28, 2016

ro0NL Dec 29, 2016 •

edited

xabbuh Dec 29, 2016

ro0NL commented Dec 29, 2016 •

edited

xabbuh commented Dec 29, 2016

ro0NL commented Dec 29, 2016

nicolas-grekas left a comment

fabpot commented Feb 14, 2017

[DI] Auto register extension configuration classes as a resource #21057

[DI] Auto register extension configuration classes as a resource #21057

Conversation

ro0NL commented Dec 26, 2016 • edited

nicolas-grekas Dec 27, 2016

Choose a reason for hiding this comment

ro0NL Dec 27, 2016

Choose a reason for hiding this comment

nicolas-grekas Dec 27, 2016

Choose a reason for hiding this comment

ro0NL Dec 27, 2016

Choose a reason for hiding this comment

sstok Dec 28, 2016 • edited

Choose a reason for hiding this comment

ro0NL Dec 28, 2016

Choose a reason for hiding this comment

ro0NL commented Dec 28, 2016

nicolas-grekas commented Dec 28, 2016

xabbuh Dec 28, 2016

Choose a reason for hiding this comment

ro0NL Dec 29, 2016 • edited

Choose a reason for hiding this comment

xabbuh Dec 29, 2016

Choose a reason for hiding this comment

ro0NL commented Dec 29, 2016 • edited

xabbuh commented Dec 29, 2016

ro0NL commented Dec 29, 2016

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Feb 14, 2017

ro0NL commented Dec 26, 2016 •

edited

sstok Dec 28, 2016 •

edited

ro0NL Dec 29, 2016 •

edited

ro0NL commented Dec 29, 2016 •

edited