[FrameworkBundle] Determine templating.engine.php scope as late as possible #11961

lyrixx · Sep 19, 2014

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11653
License	MIT
Doc PR	-

lyrixx · Sep 19, 2014

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -536,6 +537,7 @@ protected function createContainerFromFile($file, $data = array())
    {
        $container = $this->createContainer($data);
        $container->registerExtension(new FrameworkExtension());
+        $container->addCompilerPass(new TemplatingAssetHelperPass());


If I had to make tests green without changing them, I had to add this one. So I don't know what should I do:

Remove $this->assertEquals('request', $container->getDefinition('templating.helper.assets')->getScope(), '->registerTemplatingConfiguration() sets request scope on assets helper if one or more packages are request-scoped'); line from this test suite and fully unit test TemplatingAssetHelperPass

or keep it like now.

mix both solution

I added some unit tests to the TemplatingAssetHelperPass. But my previous question stays open ;)

I would vote for the first option. It is not the job of the FrameworkExtension anymore to set this scope

stof · Sep 20, 2014

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingAssetHelperPass.php

+            return;
+        }
+
+        foreach ($assetsHelperDefinition->getArgument(1) as $arg) {


why not just $args[1] ?

I forgot to update this part of the code ;)

stof · Sep 20, 2014

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingAssetHelperPass.php

+        }
+
+        foreach ($args[1] as $arg) {
+            if ('request' === $this->getPackageScope($container, $arg)) {


this is a bit dangerous. If another compiler pass changes one of the arguments (for instance adding an anonymous service rather than creating a private service and referencing it), you will get a fatal error because of the typehint

I'm not sure to know how it's possible. Does the DIC component allow that?
I can check for Reference, but what kind of object / value should I check ?

An anonymous service will be a Definition object set in the arguments directly. And yes it is allowed (it cannot be done in YAML config files though, but XML supports it, as well as PHP as you build the Definition directly)

Hm ok. I never did that before, and I just try anonymous definition, and actually, a reference is created.

I don't know what I'm doing, please double check ;)

Nice,I just try $container->register('foo', 'stdClass')->addArgument(new Definition('stdClass')); in a real project and it works. I never did that before ;)

stof · Sep 20, 2014

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingAssetHelperPass.php

+    private function getPackageScope(ContainerBuilder $container, $package)
+    {
+        if ($package instanceof Reference) {
+            return $container->getDefinition((string) $package)->getScope();


you should use findDefinition rather than getDefinition to resolve aliases. Otherwise you will get an exception if the reference points to an alias rather than a definition

fabpot · Sep 21, 2014

👍

jderusse · Sep 21, 2014

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/TemplatingAssetHelperPass.php

+            return;
+        }
+
+        foreach ($args[1] as $arg) {


Should we asserts that $args[1] is an array?

I added a check, but it's a bit useless imho, because if it's not an array, the service will not be bootable, and will lead to a fatal error.

…ssible

fabpot · Sep 22, 2014

Thank you @lyrixx.

…e as late as possible (lyrixx) This PR was merged into the 2.6-dev branch. Discussion ---------- [FrameworkBundle] Determine templating.engine.php scope as late as possible | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11653 | License | MIT | Doc PR | - Commits ------- 169dadd [FrameworkBundle] Determine templating.engine.php scope as late as possible

lyrixx reviewed Sep 19, 2014
View reviewed changes

lyrixx force-pushed the asset-package-dic-scope-request branch from ea937be to e2bd6c1 Compare September 20, 2014 00:17

stof reviewed Sep 20, 2014
View reviewed changes

lyrixx force-pushed the asset-package-dic-scope-request branch 3 times, most recently from b5fc3c0 to f70d1cf Compare September 20, 2014 00:27

stof reviewed Sep 20, 2014
View reviewed changes

lyrixx force-pushed the asset-package-dic-scope-request branch 2 times, most recently from 5cfe93f to 12e544a Compare September 20, 2014 01:01

stof reviewed Sep 20, 2014
View reviewed changes

stof added the FrameworkBundle label Sep 20, 2014

lyrixx force-pushed the asset-package-dic-scope-request branch from 12e544a to 159a4bf Compare September 20, 2014 18:33

jderusse reviewed Sep 21, 2014
View reviewed changes

[FrameworkBundle] Determine templating.engine.php scope as late as po…

169dadd

…ssible

lyrixx force-pushed the asset-package-dic-scope-request branch from 159a4bf to 169dadd Compare September 22, 2014 08:43

fabpot merged commit 169dadd into symfony:master Sep 22, 2014

lyrixx deleted the asset-package-dic-scope-request branch September 22, 2014 12:41

Search code, repositories, users, issues, pull requests...

Uh oh!

[FrameworkBundle] Determine templating.engine.php scope as late as possible #11961

[FrameworkBundle] Determine templating.engine.php scope as late as possible #11961

Uh oh!

Conversation

lyrixx commented Sep 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 21, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 22, 2014

Uh oh!

Uh oh!