Skip to content

Commit 1991256

Browse files
nicolas-grekasnicolas-grekas
committed
[DI] Fix inlining conflict by restricting IteratorArgument to Reference[]
1 parent 9c0067b commit 1991256

File tree

14 files changed

+84
-85
lines changed

14 files changed

+84
-85
lines changed

‎src/Symfony/Component/DependencyInjection/Argument/IteratorArgument.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Argument;
1313

14+
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
15+
use Symfony\Component\DependencyInjection\Reference;
16+
1417
/**
1518
* Represents a collection of values to lazily iterate over.
1619
*
@@ -20,9 +23,12 @@ class IteratorArgument implements ArgumentInterface
2023
{
2124
private $values;
2225

26+
/**
27+
* @param Reference[] $values
28+
*/
2329
public function __construct(array $values)
2430
{
25-
$this->values = $values;
31+
$this->setValues($values);
2632
}
2733

2834
/**
@@ -34,10 +40,16 @@ public function getValues()
3440
}
3541

3642
/**
37-
* @param array $values The values to lazily iterate over
43+
* @param Reference[] $values The service references to lazily iterate over
3844
*/
3945
public function setValues(array $values)
4046
{
47+
foreach ($values as $k => $v) {
48+
if (null !== $v && !$v instanceof Reference) {
49+
throw new InvalidArgumentException(sprintf('An IteratorArgument must hold only Reference instances, "%s" given.', is_object($v) ? get_class($v) : gettype($v)));
50+
}
51+
}
52+
4153
$this->values = $values;
4254
}
4355
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ public function setRepeatedPass(RepeatedPass $repeatedPass)
3838
protected function processValue($value, $isRoot = false)
3939
{
4040
if ($value instanceof ArgumentInterface) {
41-
$this->processValue($value->getValues());
42-
41+
// Reference found in ArgumentInterface::getValues() are not inlineable
4342
return $value;
4443
}
4544
if ($value instanceof Reference && $this->container->hasDefinition($id = (string) $value)) {

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1125,16 +1125,15 @@ public function resolveServices($value)
11251125
return $this->resolveServices($reference);
11261126
};
11271127
} elseif ($value instanceof IteratorArgument) {
1128-
$parameterBag = $this->getParameterBag();
1129-
$value = new RewindableGenerator(function () use ($value, $parameterBag) {
1128+
$value = new RewindableGenerator(function () use ($value) {
11301129
foreach ($value->getValues() as $k => $v) {
11311130
foreach (self::getServiceConditionals($v) as $s) {
11321131
if (!$this->has($s)) {
11331132
continue 2;
11341133
}
11351134
}
11361135

1137-
yield $k => $this->resolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($v)));
1136+
yield $k => $this->resolveServices($v);
11381137
}
11391138
}, function () use ($value) {
11401139
$count = 0;
@@ -1409,7 +1408,7 @@ private function callMethod($service, $call)
14091408
* Shares a given service in the container.
14101409
*
14111410
* @param Definition $definition
1412-
* @param mixed $service
1411+
* @param object $service
14131412
* @param string|null $id
14141413
*/
14151414
private function shareService(Definition $definition, $service, $id)

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

Lines changed: 38 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -202,19 +202,19 @@ private function getProxyDumper()
202202
/**
203203
* Generates Service local temp variables.
204204
*
205-
* @param string $cId
206-
* @param string $definition
205+
* @param string $cId
206+
* @param array $localDefinitions
207+
* @param Definition|null $definition
207208
*
208209
* @return string
209210
*/
210-
private function addServiceLocalTempVariables($cId, $definition)
211+
private function addServiceLocalTempVariables($cId, array $localDefinitions, Definition $definition = null)
211212
{
212213
static $template = " \$%s = %s;\n";
213214

214-
$localDefinitions = array_merge(
215-
array($definition),
216-
$this->getInlinedDefinitions($definition)
217-
);
215+
if (null !== $definition) {
216+
array_unshift($localDefinitions, $definition);
217+
}
218218

219219
$calls = $behavior = array();
220220
foreach ($localDefinitions as $iDefinition) {
@@ -280,12 +280,13 @@ private function addProxyClasses()
280280
/**
281281
* Generates the require_once statement for service includes.
282282
*
283-
* @param string $id The service id
283+
* @param string $id
284284
* @param Definition $definition
285+
* @param array $inlinedDefinitions
285286
*
286287
* @return string
287288
*/
288-
private function addServiceInclude($id, $definition)
289+
private function addServiceInclude($id, Definition $definition, array $inlinedDefinitions)
289290
{
290291
$template = " require_once %s;\n";
291292
$code = '';
@@ -294,7 +295,7 @@ private function addServiceInclude($id, $definition)
294295
$code .= sprintf($template, $this->dumpValue($file));
295296
}
296297

297-
foreach ($this->getInlinedDefinitions($definition) as $definition) {
298+
foreach ($inlinedDefinitions as $definition) {
298299
if (null !== $file = $definition->getFile()) {
299300
$code .= sprintf($template, $this->dumpValue($file));
300301
}
@@ -310,21 +311,20 @@ private function addServiceInclude($id, $definition)
310311
/**
311312
* Generates the inline definition of a service.
312313
*
313-
* @param string $id
314-
* @param Definition $definition
314+
* @param string $id
315+
* @param array $inlinedDefinitions
315316
*
316317
* @return string
317318
*
318319
* @throws RuntimeException When the factory definition is incomplete
319320
* @throws ServiceCircularReferenceException When a circular reference is detected
320321
*/
321-
private function addServiceInlinedDefinitions($id, $definition)
322+
private function addServiceInlinedDefinitions($id, array $inlinedDefinitions)
322323
{
323324
$code = '';
324325
$variableMap = $this->definitionVariables;
325326
$nbOccurrences = new \SplObjectStorage();
326327
$processed = new \SplObjectStorage();
327-
$inlinedDefinitions = $this->getInlinedDefinitions($definition);
328328

329329
foreach ($inlinedDefinitions as $definition) {
330330
if (false === $nbOccurrences->contains($definition)) {
@@ -375,14 +375,14 @@ private function addServiceInlinedDefinitions($id, $definition)
375375
/**
376376
* Adds the service return statement.
377377
*
378-
* @param string $id Service id
379-
* @param Definition $definition
378+
* @param string $id
379+
* @param bool $isSimpleInstance
380380
*
381381
* @return string
382382
*/
383-
private function addServiceReturn($id, $definition)
383+
private function addServiceReturn($id, $isSimpleInstance)
384384
{
385-
if ($this->isSimpleInstance($id, $definition)) {
385+
if ($isSimpleInstance) {
386386
return " }\n";
387387
}
388388

@@ -394,13 +394,14 @@ private function addServiceReturn($id, $definition)
394394
*
395395
* @param string $id
396396
* @param Definition $definition
397+
* @param bool $simple
397398
*
398399
* @return string
399400
*
400401
* @throws InvalidArgumentException
401402
* @throws RuntimeException
402403
*/
403-
private function addServiceInstance($id, Definition $definition)
404+
private function addServiceInstance($id, Definition $definition, $simple)
404405
{
405406
$class = $definition->getClass();
406407

@@ -414,7 +415,6 @@ private function addServiceInstance($id, Definition $definition)
414415
throw new InvalidArgumentException(sprintf('"%s" is not a valid class name for the "%s" service.', $class, $id));
415416
}
416417

417-
$simple = $this->isSimpleInstance($id, $definition);
418418
$isProxyCandidate = $this->getProxyDumper()->isProxyCandidate($definition);
419419
$instantiation = '';
420420

@@ -500,20 +500,21 @@ private function addServiceProperties($id, Definition $definition, $variableName
500500
/**
501501
* Generates the inline definition setup.
502502
*
503-
* @param string $id
504-
* @param Definition $definition
503+
* @param string $id
504+
* @param array $inlinedDefinitions
505+
* @param bool $isSimpleInstance
505506
*
506507
* @return string
507508
*
508509
* @throws ServiceCircularReferenceException when the container contains a circular reference
509510
*/
510-
private function addServiceInlinedDefinitionsSetup($id, Definition $definition)
511+
private function addServiceInlinedDefinitionsSetup($id, array $inlinedDefinitions, $isSimpleInstance)
511512
{
512513
$this->referenceVariables[$id] = new Variable('instance');
513514

514515
$code = '';
515516
$processed = new \SplObjectStorage();
516-
foreach ($this->getInlinedDefinitions($definition) as $iDefinition) {
517+
foreach ($inlinedDefinitions as $iDefinition) {
517518
if ($processed->contains($iDefinition)) {
518519
continue;
519520
}
@@ -525,7 +526,7 @@ private function addServiceInlinedDefinitionsSetup($id, Definition $definition)
525526

526527
// if the instance is simple, the return statement has already been generated
527528
// so, the only possible way to get there is because of a circular reference
528-
if ($this->isSimpleInstance($id, $definition)) {
529+
if ($isSimpleInstance) {
529530
throw new ServiceCircularReferenceException($id, array($id));
530531
}
531532

@@ -683,16 +684,19 @@ private function addService($id, Definition $definition)
683684
$code .= sprintf(" @trigger_error(%s, E_USER_DEPRECATED);\n\n", $this->export($definition->getDeprecationMessage($id)));
684685
}
685686

687+
$inlinedDefinitions = $this->getInlinedDefinitions($definition);
688+
$isSimpleInstance = $this->isSimpleInstance($id, $definition, $inlinedDefinitions);
689+
686690
$code .=
687-
$this->addServiceInclude($id, $definition).
688-
$this->addServiceLocalTempVariables($id, $definition).
689-
$this->addServiceInlinedDefinitions($id, $definition).
690-
$this->addServiceInstance($id, $definition).
691-
$this->addServiceInlinedDefinitionsSetup($id, $definition).
691+
$this->addServiceInclude($id, $definition, $inlinedDefinitions).
692+
$this->addServiceLocalTempVariables($id, $inlinedDefinitions, $definition).
693+
$this->addServiceInlinedDefinitions($id, $inlinedDefinitions).
694+
$this->addServiceInstance($id, $definition, $isSimpleInstance).
695+
$this->addServiceInlinedDefinitionsSetup($id, $inlinedDefinitions, $isSimpleInstance).
692696
$this->addServiceProperties($id, $definition).
693697
$this->addServiceMethodCalls($id, $definition).
694698
$this->addServiceConfigurator($id, $definition).
695-
$this->addServiceReturn($id, $definition)
699+
$this->addServiceReturn($id, $isSimpleInstance)
696700
;
697701

698702
$this->definitionVariables = null;
@@ -1332,8 +1336,6 @@ private function getDefinitionsFromArguments(array $arguments)
13321336
foreach ($arguments as $argument) {
13331337
if (is_array($argument)) {
13341338
$definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument));
1335-
} elseif ($argument instanceof ArgumentInterface) {
1336-
$definitions = array_merge($definitions, $this->getDefinitionsFromArguments($argument->getValues()));
13371339
} elseif ($argument instanceof Definition) {
13381340
$definitions = array_merge(
13391341
$definitions,
@@ -1452,6 +1454,9 @@ private function dumpValue($value, $interpolate = true)
14521454
if ($value->getMethodCalls()) {
14531455
throw new RuntimeException('Cannot dump definitions which have method calls.');
14541456
}
1457+
if ($value->getProperties()) {
1458+
throw new RuntimeException('Cannot dump definitions which have properties.');
1459+
}
14551460
if (null !== $value->getConfigurator()) {
14561461
throw new RuntimeException('Cannot dump definitions which have a configurator.');
14571462
}

‎src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,11 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $lowercase = true,
480480
$arguments[$key] = $this->getArgumentsAsPhp($arg, $name, false);
481481
break;
482482
case 'iterator':
483-
$arguments[$key] = new IteratorArgument($this->getArgumentsAsPhp($arg, $name, false));
483+
try {
484+
$arguments[$key] = new IteratorArgument($this->getArgumentsAsPhp($arg, $name, false));
485+
} catch (InvalidArgumentException $e) {
486+
throw new InvalidArgumentException(sprintf('Tag "<%s>" with type="iterator" only accepts collections of type="service" references.', $name));
487+
}
484488
break;
485489
case 'string':
486490
$arguments[$key] = $arg->nodeValue;

‎src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -654,14 +654,18 @@ private function resolveServices($value, $file, $isParameter = false)
654654
$argument = $value->getValue();
655655
if ('iterator' === $value->getTag()) {
656656
if (!is_array($argument)) {
657-
throw new InvalidArgumentException('"!iterator" tag only accepts sequences.');
657+
throw new InvalidArgumentException(sprintf('"!iterator" tag only accepts sequences in "%s".', $file));
658658
}
659659

660-
return new IteratorArgument($this->resolveServices($argument, $file, $isParameter));
660+
try {
661+
return new IteratorArgument($this->resolveServices($argument, $file, $isParameter));
662+
} catch (InvalidArgumentException $e) {
663+
throw new InvalidArgumentException(sprintf('"!iterator" tag only accepts arrays of "@service" references in "%s".', $file));
664+
}
661665
}
662666
if ('closure_proxy' === $value->getTag()) {
663667
if (!is_array($argument) || array(0, 1) !== array_keys($argument) || !is_string($argument[0]) || !is_string($argument[1]) || 0 !== strpos($argument[0], '@') || 0 === strpos($argument[0], '@@')) {
664-
throw new InvalidArgumentException('"!closure_proxy" tagged values must be arrays of [@service, method].');
668+
throw new InvalidArgumentException(sprintf('"!closure_proxy" tagged values must be arrays of [@service, method] in "%s".', $file));
665669
}
666670

667671
if (0 === strpos($argument[0], '@?')) {

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ public function testLazyArgumentProvideGenerator()
434434
$container->register('lazy_referenced', 'stdClass');
435435
$container
436436
->register('lazy_context', 'LazyContext')
437-
->setArguments(array(new IteratorArgument(array('foo', new Reference('lazy_referenced'), 'k1' => array('foo' => 'bar'), true, 'k2' => new Reference('service_container')))))
437+
->setArguments(array(new IteratorArgument(array('k1' => new Reference('lazy_referenced'), 'k2' => new Reference('service_container')))))
438438
;
439439
$container->compile();
440440

@@ -450,24 +450,12 @@ public function testLazyArgumentProvideGenerator()
450450
foreach ($lazyContext->lazyValues as $k => $v) {
451451
switch (++$i) {
452452
case 0:
453-
$this->assertEquals(0, $k);
454-
$this->assertEquals('foo', $v);
455-
break;
456-
case 1:
457-
$this->assertEquals(1, $k);
458-
$this->assertInstanceOf('stdClass', $v);
459-
break;
460-
case 2:
461453
$this->assertEquals('k1', $k);
462-
$this->assertEquals(array('foo' => 'bar'), $v);
454+
$this->assertInstanceOf('stdCLass', $v);
463455
break;
464-
case 3:
465-
$this->assertEquals(2, $k);
466-
$this->assertTrue($v);
467-
break;
468-
case 4:
456+
case 1:
469457
$this->assertEquals('k2', $k);
470-
$this->assertInstanceOf('\Symfony_DI_PhpDumper_Test_Lazy_Argument_Provide_Generator', $v);
458+
$this->assertInstanceOf('Symfony_DI_PhpDumper_Test_Lazy_Argument_Provide_Generator', $v);
471459
break;
472460
}
473461
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@
134134
;
135135
$container
136136
->register('lazy_context', 'LazyContext')
137-
->setArguments(array(new IteratorArgument(array('foo', new Reference('foo.baz'), array('%foo%' => 'foo is %foo%', 'foobar' => '%foo%'), true, new Reference('service_container')))))
137+
->setArguments(array(new IteratorArgument(array('k1' => new Reference('foo.baz'), 'k2' => new Reference('service_container')))))
138138
;
139139
$container
140140
->register('lazy_context_ignore_invalid_ref', 'LazyContext')

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -321,12 +321,9 @@ protected function getFooWithInlineService()
321321
protected function getLazyContextService()
322322
{
323323
return $this->services['lazy_context'] = new \LazyContext(new RewindableGenerator(function () {
324-
yield 0 => 'foo';
325-
yield 1 => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
326-
yield 2 => array($this->getParameter('foo') => 'foo is '.$this->getParameter('foo').'', 'foobar' => $this->getParameter('foo'));
327-
yield 3 => true;
328-
yield 4 => $this;
329-
}, 5));
324+
yield 'k1' => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
325+
yield 'k2' => $this;
326+
}, 2));
330327
}
331328

332329
/**

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,9 @@ protected function getFooWithInlineService()
328328
protected function getLazyContextService()
329329
{
330330
return $this->services['lazy_context'] = new \LazyContext(new RewindableGenerator(function () {
331-
yield 0 => 'foo';
332-
yield 1 => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
333-
yield 2 => array('bar' => 'foo is bar', 'foobar' => 'bar');
334-
yield 3 => true;
335-
yield 4 => $this;
336-
}, 5));
331+
yield 'k1' => ${($_ = isset($this->services['foo.baz']) ? $this->services['foo.baz'] : $this->get('foo.baz')) && false ?: '_'};
332+
yield 'k2' => $this;
333+
}, 2));
337334
}
338335

339336
/**

0 commit comments

Comments
 (0)