[DI] Do not set values of lazy arguments after inlining them #21312

chalasr · 2017-01-16T18:55:02Z

Q	A
Branch?	master
Bug fix?	not yet
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21299
License	MIT
Doc PR	n/a

nicolas-grekas · 2017-01-16T18:59:01Z

Missing test ;)

chalasr · 2017-01-16T20:14:24Z

Test added!

nicolas-grekas · 2017-01-17T09:10:34Z

👍

nicolas-grekas · 2017-01-17T13:45:10Z

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

@@ -68,8 +67,6 @@ private function inlineArguments(ContainerBuilder $container, array $arguments,
            }
            if (is_array($argument)) {
                $arguments[$k] = $this->inlineArguments($container, $argument);
-            } elseif ($argument instanceof ArgumentInterface) {
-                $argument->setValues($this->inlineArguments($container, $argument->getValues()));


by removing this, we miss the opportunity to inline inside lazy arguments.
only the call to setValue should be removed in fact

I didn't dare to ask you about that. Updated

xabbuh · 2017-01-17T21:07:51Z

src/Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php

+
+        $this->process($container);
+
+        $values = $container->getDefinition('closure-proxy')->getArguments()[0]->getValues();


just use getArgument(0) here

it was a quick copy-paste, fixed

xabbuh · 2017-01-17T21:07:59Z

src/Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php

+        $this->assertInstanceOf(Reference::class, $values[0]);
+        $this->assertSame('inline', (string) $values[0]);
+
+        $values = $container->getDefinition('iterator')->getArguments()[0]->getValues();


xabbuh · 2017-01-17T21:22:29Z

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

@@ -69,7 +69,7 @@ private function inlineArguments(ContainerBuilder $container, array $arguments,
            if (is_array($argument)) {
                $arguments[$k] = $this->inlineArguments($container, $argument);
            } elseif ($argument instanceof ArgumentInterface) {
-                $argument->setValues($this->inlineArguments($container, $argument->getValues()));
+                $this->inlineArguments($container, $argument->getValues());


Is there any case where this could break? For example, if someone uses their own implementation of ArgumentInterface for whatever reason?

To answer my own question: the ArgumentInterface wasn't part of any release yet.

xabbuh · 2017-01-17T21:25:01Z

👍

Status: Reviewed

fabpot · 2017-01-18T05:25:32Z

Thank you @chalasr.

…em (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Do not set values of lazy arguments after inlining them | Q | A | ------------- | --- | Branch? | master | Bug fix? | not yet | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21299 | License | MIT | Doc PR | n/a Commits ------- 04da7c3 [DI] Do not inline values of lazy arguments

carsonbot added Status: Needs Review DependencyInjection labels Jan 16, 2017

chalasr mentioned this pull request Jan 16, 2017

[DI] ClosureProxyArgument: Definition could not be converted to string #21299

Closed

chalasr force-pushed the di/inlined-def-fix branch from 6a8b3ac to ad5abb7 Compare January 16, 2017 18:55

chalasr force-pushed the di/inlined-def-fix branch 3 times, most recently from 5523b21 to bb0ad49 Compare January 16, 2017 19:59

nicolas-grekas reviewed Jan 17, 2017

View reviewed changes

chalasr force-pushed the di/inlined-def-fix branch from bb0ad49 to 98c975c Compare January 17, 2017 13:58

chalasr changed the title ~~[DependencyInjection] Do not inline values of lazy arguments~~ [DependencyInjection] Do not set values of lazy arguments after inlining them Jan 17, 2017

chalasr mentioned this pull request Jan 17, 2017

[Console][FrameworkBundle] Log console exceptions #21003

Merged

chalasr changed the title ~~[DependencyInjection] Do not set values of lazy arguments after inlining them~~ [DI] Do not set values of lazy arguments after inlining them Jan 17, 2017

xabbuh reviewed Jan 17, 2017

View reviewed changes

[DI] Do not inline values of lazy arguments

04da7c3

chalasr force-pushed the di/inlined-def-fix branch from 98c975c to 04da7c3 Compare January 17, 2017 21:13

xabbuh reviewed Jan 17, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 17, 2017

fabpot merged commit 04da7c3 into symfony:master Jan 18, 2017

chalasr deleted the di/inlined-def-fix branch January 18, 2017 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Do not set values of lazy arguments after inlining them #21312

[DI] Do not set values of lazy arguments after inlining them #21312

chalasr commented Jan 16, 2017

nicolas-grekas commented Jan 16, 2017

chalasr commented Jan 16, 2017

nicolas-grekas commented Jan 17, 2017

nicolas-grekas Jan 17, 2017

chalasr Jan 17, 2017

xabbuh Jan 17, 2017

chalasr Jan 17, 2017

xabbuh Jan 17, 2017

xabbuh Jan 17, 2017

xabbuh Jan 17, 2017

xabbuh commented Jan 17, 2017

fabpot commented Jan 18, 2017


		$this->process($container);

		$values = $container->getDefinition('closure-proxy')->getArguments()[0]->getValues();

[DI] Do not set values of lazy arguments after inlining them #21312

[DI] Do not set values of lazy arguments after inlining them #21312

Conversation

chalasr commented Jan 16, 2017

nicolas-grekas commented Jan 16, 2017

chalasr commented Jan 16, 2017

nicolas-grekas commented Jan 17, 2017

nicolas-grekas Jan 17, 2017

Choose a reason for hiding this comment

chalasr Jan 17, 2017

Choose a reason for hiding this comment

xabbuh Jan 17, 2017

Choose a reason for hiding this comment

chalasr Jan 17, 2017

Choose a reason for hiding this comment

xabbuh Jan 17, 2017

Choose a reason for hiding this comment

xabbuh Jan 17, 2017

Choose a reason for hiding this comment

xabbuh Jan 17, 2017

Choose a reason for hiding this comment

xabbuh commented Jan 17, 2017

fabpot commented Jan 18, 2017