Skip to content

Commit 5edbc13

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#22581 [DI] Fix invalid callables dumped for ArgumentInterface objects (nicolas-grekas, iltar)
This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Fix invalid callables dumped for ArgumentInterface objects | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Follow up of symfony#22511, from @iltar: > Currently, when having a service definition which has more than 1 usage of that service, it will put the argument in `$a` for example. By doing so, it will also use `$a` in the callable of the `ServiceLocator`, which result in an undefined variable `$a`. > I managed to trigger this with the translator service, where I have my own loader, that I add for NL and EN, causing 2 method calls to turn the direct getter into a variable. I've added 2 test cases, 1 with only 1 method call and 1 with 2 method calls. Commits ------- f81c577 [DI] Test references inside ServiceLocator are not inlined 8783602 [DI] Fix invalid callables dumped for ArgumentInterface objects
2 parents b9e19f6 + f81c577 commit 5edbc13

File tree

5 files changed

+235
-53
lines changed

5 files changed

+235
-53
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\DependencyInjection\Argument;
1313

1414
use Symfony\Component\DependencyInjection\ContainerInterface;
15+
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
1516
use Symfony\Component\DependencyInjection\Reference;
1617

1718
/**
@@ -41,6 +42,9 @@ public function getValues()
4142
*/
4243
public function setValues(array $values)
4344
{
45+
if (!$values[0] instanceof Reference) {
46+
throw new InvalidArgumentException(sprintf('A ClosureProxyArgument must hold a Reference, "%s" given.', is_object($values[0]) ? get_class($values[0]) : gettype($values[0])));
47+
}
4448
list($this->reference, $this->method) = $values;
4549
}
4650
}

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

Lines changed: 66 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1410,40 +1410,79 @@ private function dumpValue($value, $interpolate = true)
14101410
}
14111411

14121412
return sprintf('array(%s)', implode(', ', $code));
1413-
} elseif ($value instanceof ServiceClosureArgument) {
1414-
$value = $value->getValues()[0];
1415-
$code = $this->dumpValue($value, $interpolate);
1416-
1417-
if ($value instanceof TypedReference) {
1418-
$code = sprintf('$f = function (\\%s $v%s) { return $v; }; return $f(%s);', $value->getType(), ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $value->getInvalidBehavior() ? ' = null' : '', $code);
1419-
} else {
1420-
$code = sprintf('return %s;', $code);
1421-
}
1413+
} elseif ($value instanceof ArgumentInterface) {
1414+
$scope = array($this->definitionVariables, $this->referenceVariables, $this->variableCount);
1415+
$this->definitionVariables = $this->referenceVariables = null;
1416+
1417+
try {
1418+
if ($value instanceof ServiceClosureArgument) {
1419+
$value = $value->getValues()[0];
1420+
$code = $this->dumpValue($value, $interpolate);
1421+
1422+
if ($value instanceof TypedReference) {
1423+
$code = sprintf('$f = function (\\%s $v%s) { return $v; }; return $f(%s);', $value->getType(), ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $value->getInvalidBehavior() ? ' = null' : '', $code);
1424+
} else {
1425+
$code = sprintf('return %s;', $code);
1426+
}
14221427

1423-
return sprintf("function () {\n %s\n }", $code);
1424-
} elseif ($value instanceof IteratorArgument) {
1425-
$countCode = array();
1426-
$countCode[] = 'function () {';
1427-
$operands = array(0);
1428+
return sprintf("function () {\n %s\n }", $code);
1429+
}
14281430

1429-
$code = array();
1430-
$code[] = 'new RewindableGenerator(function () {';
1431-
foreach ($value->getValues() as $k => $v) {
1432-
($c = $this->getServiceConditionals($v)) ? $operands[] = "(int) ($c)" : ++$operands[0];
1433-
$v = $this->wrapServiceConditionals($v, sprintf(" yield %s => %s;\n", $this->dumpValue($k, $interpolate), $this->dumpValue($v, $interpolate)));
1434-
foreach (explode("\n", $v) as $v) {
1435-
if ($v) {
1436-
$code[] = ' '.$v;
1431+
if ($value instanceof IteratorArgument) {
1432+
$countCode = array();
1433+
$countCode[] = 'function () {';
1434+
$operands = array(0);
1435+
1436+
$code = array();
1437+
$code[] = 'new RewindableGenerator(function () {';
1438+
foreach ($value->getValues() as $k => $v) {
1439+
($c = $this->getServiceConditionals($v)) ? $operands[] = "(int) ($c)" : ++$operands[0];
1440+
$v = $this->wrapServiceConditionals($v, sprintf(" yield %s => %s;\n", $this->dumpValue($k, $interpolate), $this->dumpValue($v, $interpolate)));
1441+
foreach (explode("\n", $v) as $v) {
1442+
if ($v) {
1443+
$code[] = ' '.$v;
1444+
}
1445+
}
14371446
}
1447+
1448+
$countCode[] = sprintf(' return %s;', implode(' + ', $operands));
1449+
$countCode[] = ' }';
1450+
1451+
$code[] = sprintf(' }, %s)', count($operands) > 1 ? implode("\n", $countCode) : $operands[0]);
1452+
1453+
return implode("\n", $code);
14381454
}
1439-
}
14401455

1441-
$countCode[] = sprintf(' return %s;', implode(' + ', $operands));
1442-
$countCode[] = ' }';
1456+
if ($value instanceof ClosureProxyArgument) {
1457+
list($reference, $method) = $value->getValues();
1458+
$method = substr($this->dumpLiteralClass($this->dumpValue($method)), 1);
1459+
1460+
if ('service_container' === (string) $reference) {
1461+
$class = $this->baseClass;
1462+
} elseif (!$this->container->hasDefinition((string) $reference) && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
1463+
return 'null';
1464+
} else {
1465+
$class = substr($this->dumpLiteralClass($this->dumpValue($this->container->findDefinition((string) $reference)->getClass())), 1);
1466+
}
1467+
if (false !== strpos($class, '$') || false !== strpos($method, '$')) {
1468+
throw new RuntimeException(sprintf('Cannot dump definition for service "%s": dynamic class names or methods, and closure-proxies are incompatible with each other.', $reference));
1469+
}
1470+
if (!method_exists($class, $method)) {
1471+
throw new InvalidArgumentException(sprintf('Cannot create closure-proxy for service "%s": method "%s::%s" does not exist.', $reference, $class, $method));
1472+
}
1473+
$r = $this->container->getReflectionClass($class)->getMethod($method);
1474+
if (!$r->isPublic()) {
1475+
throw new InvalidArgumentException(sprintf('Cannot create closure-proxy for service "%s": method "%s::%s" must be public.', $reference, $class, $method));
1476+
}
1477+
$signature = preg_replace('/^(&?)[^(]*/', '$1', ProxyHelper::getSignature($r, $call));
14431478

1444-
$code[] = sprintf(' }, %s)', count($operands) > 1 ? implode("\n", $countCode) : $operands[0]);
1479+
$return = 'void' !== ProxyHelper::getTypeHint($r);
14451480

1446-
return implode("\n", $code);
1481+
return sprintf("/** @closure-proxy %s::%s */ function %s {\n %s%s->%s;\n }", $class, $method, $signature, $return ? 'return ' : '', $this->dumpValue($reference), $call);
1482+
}
1483+
} finally {
1484+
list($this->definitionVariables, $this->referenceVariables, $this->variableCount) = $scope;
1485+
}
14471486
} elseif ($value instanceof Definition) {
14481487
if (null !== $this->definitionVariables && $this->definitionVariables->contains($value)) {
14491488
return $this->dumpValue($this->definitionVariables->offsetGet($value), $interpolate);
@@ -1497,32 +1536,6 @@ private function dumpValue($value, $interpolate = true)
14971536
}
14981537

14991538
return sprintf('new %s(%s)', $this->dumpLiteralClass($this->dumpValue($class)), implode(', ', $arguments));
1500-
} elseif ($value instanceof ClosureProxyArgument) {
1501-
list($reference, $method) = $value->getValues();
1502-
$method = substr($this->dumpLiteralClass($this->dumpValue($method)), 1);
1503-
1504-
if ('service_container' === (string) $reference) {
1505-
$class = $this->baseClass;
1506-
} elseif (!$this->container->hasDefinition((string) $reference) && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
1507-
return 'null';
1508-
} else {
1509-
$class = substr($this->dumpLiteralClass($this->dumpValue($this->container->findDefinition((string) $reference)->getClass())), 1);
1510-
}
1511-
if (false !== strpos($class, '$') || false !== strpos($method, '$')) {
1512-
throw new RuntimeException(sprintf('Cannot dump definition for service "%s": dynamic class names or methods, and closure-proxies are incompatible with each other.', $reference));
1513-
}
1514-
if (!method_exists($class, $method)) {
1515-
throw new InvalidArgumentException(sprintf('Cannot create closure-proxy for service "%s": method "%s::%s" does not exist.', $reference, $class, $method));
1516-
}
1517-
$r = $this->container->getReflectionClass($class)->getMethod($method);
1518-
if (!$r->isPublic()) {
1519-
throw new InvalidArgumentException(sprintf('Cannot create closure-proxy for service "%s": method "%s::%s" must be public.', $reference, $class, $method));
1520-
}
1521-
$signature = preg_replace('/^(&?)[^(]*/', '$1', ProxyHelper::getSignature($r, $call));
1522-
1523-
$return = 'void' !== ProxyHelper::getTypeHint($r);
1524-
1525-
return sprintf("/** @closure-proxy %s::%s */ function %s {\n %s%s->%s;\n }", $class, $method, $signature, $return ? 'return ' : '', $this->dumpValue($reference), $call);
15261539
} elseif ($value instanceof Variable) {
15271540
return '$'.$value;
15281541
} elseif ($value instanceof Reference) {

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
2323
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
2424
use Symfony\Component\DependencyInjection\Reference;
25+
use Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator;
2526
use Symfony\Component\DependencyInjection\TypedReference;
2627
use Symfony\Component\DependencyInjection\Definition;
2728
use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;
@@ -534,6 +535,40 @@ public function testServiceLocator()
534535
'nil' => $nil = new ServiceClosureArgument(new Reference('nil')),
535536
))
536537
;
538+
539+
// no method calls
540+
$container->register('translator.loader_1', 'stdClass');
541+
$container->register('translator.loader_1_locator', ServiceLocator::class)
542+
->setPublic(false)
543+
->addArgument(array(
544+
'translator.loader_1' => new ServiceClosureArgument(new Reference('translator.loader_1')),
545+
));
546+
$container->register('translator_1', StubbedTranslator::class)
547+
->addArgument(new Reference('translator.loader_1_locator'));
548+
549+
// one method calls
550+
$container->register('translator.loader_2', 'stdClass');
551+
$container->register('translator.loader_2_locator', ServiceLocator::class)
552+
->setPublic(false)
553+
->addArgument(array(
554+
'translator.loader_2' => new ServiceClosureArgument(new Reference('translator.loader_2')),
555+
));
556+
$container->register('translator_2', StubbedTranslator::class)
557+
->addArgument(new Reference('translator.loader_2_locator'))
558+
->addMethodCall('addResource', array('db', new Reference('translator.loader_2'), 'nl'));
559+
560+
// two method calls
561+
$container->register('translator.loader_3', 'stdClass');
562+
$container->register('translator.loader_3_locator', ServiceLocator::class)
563+
->setPublic(false)
564+
->addArgument(array(
565+
'translator.loader_3' => new ServiceClosureArgument(new Reference('translator.loader_3')),
566+
));
567+
$container->register('translator_3', StubbedTranslator::class)
568+
->addArgument(new Reference('translator.loader_3_locator'))
569+
->addMethodCall('addResource', array('db', new Reference('translator.loader_3'), 'nl'))
570+
->addMethodCall('addResource', array('db', new Reference('translator.loader_3'), 'en'));
571+
537572
$nil->setValues(array(null));
538573
$container->register('bar_service', 'stdClass')->setArguments(array(new Reference('baz_service')));
539574
$container->register('baz_service', 'stdClass')->setPublic(false);

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
13+
14+
use Psr\Container\ContainerInterface;
15+
16+
/**
17+
* @author Iltar van der Berg <[email protected]>
18+
*/
19+
class StubbedTranslator
20+
{
21+
public function __construct(ContainerInterface $container)
22+
{
23+
24+
}
25+
26+
public function addResource($format, $resource, $locale, $domain = null)
27+
{
28+
}
29+
}

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

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,12 @@ public function __construct()
3131
'bar_service' => 'getBarServiceService',
3232
'baz_service' => 'getBazServiceService',
3333
'foo_service' => 'getFooServiceService',
34+
'translator.loader_1' => 'getTranslator_Loader1Service',
35+
'translator.loader_2' => 'getTranslator_Loader2Service',
36+
'translator.loader_3' => 'getTranslator_Loader3Service',
37+
'translator_1' => 'getTranslator1Service',
38+
'translator_2' => 'getTranslator2Service',
39+
'translator_3' => 'getTranslator3Service',
3440
);
3541
$this->privates = array(
3642
'baz_service' => true,
@@ -97,6 +103,101 @@ protected function getFooServiceService()
97103
}));
98104
}
99105

106+
/**
107+
* Gets the 'translator.loader_1' service.
108+
*
109+
* This service is shared.
110+
* This method always returns the same instance of the service.
111+
*
112+
* @return \stdClass A stdClass instance
113+
*/
114+
protected function getTranslator_Loader1Service()
115+
{
116+
return $this->services['translator.loader_1'] = new \stdClass();
117+
}
118+
119+
/**
120+
* Gets the 'translator.loader_2' service.
121+
*
122+
* This service is shared.
123+
* This method always returns the same instance of the service.
124+
*
125+
* @return \stdClass A stdClass instance
126+
*/
127+
protected function getTranslator_Loader2Service()
128+
{
129+
return $this->services['translator.loader_2'] = new \stdClass();
130+
}
131+
132+
/**
133+
* Gets the 'translator.loader_3' service.
134+
*
135+
* This service is shared.
136+
* This method always returns the same instance of the service.
137+
*
138+
* @return \stdClass A stdClass instance
139+
*/
140+
protected function getTranslator_Loader3Service()
141+
{
142+
return $this->services['translator.loader_3'] = new \stdClass();
143+
}
144+
145+
/**
146+
* Gets the 'translator_1' service.
147+
*
148+
* This service is shared.
149+
* This method always returns the same instance of the service.
150+
*
151+
* @return \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator A Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator instance
152+
*/
153+
protected function getTranslator1Service()
154+
{
155+
return $this->services['translator_1'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator(new \Symfony\Component\DependencyInjection\ServiceLocator(array('translator.loader_1' => function () {
156+
return ${($_ = isset($this->services['translator.loader_1']) ? $this->services['translator.loader_1'] : $this->get('translator.loader_1')) && false ?: '_'};
157+
})));
158+
}
159+
160+
/**
161+
* Gets the 'translator_2' service.
162+
*
163+
* This service is shared.
164+
* This method always returns the same instance of the service.
165+
*
166+
* @return \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator A Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator instance
167+
*/
168+
protected function getTranslator2Service()
169+
{
170+
$this->services['translator_2'] = $instance = new \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator(new \Symfony\Component\DependencyInjection\ServiceLocator(array('translator.loader_2' => function () {
171+
return ${($_ = isset($this->services['translator.loader_2']) ? $this->services['translator.loader_2'] : $this->get('translator.loader_2')) && false ?: '_'};
172+
})));
173+
174+
$instance->addResource('db', ${($_ = isset($this->services['translator.loader_2']) ? $this->services['translator.loader_2'] : $this->get('translator.loader_2')) && false ?: '_'}, 'nl');
175+
176+
return $instance;
177+
}
178+
179+
/**
180+
* Gets the 'translator_3' service.
181+
*
182+
* This service is shared.
183+
* This method always returns the same instance of the service.
184+
*
185+
* @return \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator A Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator instance
186+
*/
187+
protected function getTranslator3Service()
188+
{
189+
$a = ${($_ = isset($this->services['translator.loader_3']) ? $this->services['translator.loader_3'] : $this->get('translator.loader_3')) && false ?: '_'};
190+
191+
$this->services['translator_3'] = $instance = new \Symfony\Component\DependencyInjection\Tests\Fixtures\StubbedTranslator(new \Symfony\Component\DependencyInjection\ServiceLocator(array('translator.loader_3' => function () {
192+
return ${($_ = isset($this->services['translator.loader_3']) ? $this->services['translator.loader_3'] : $this->get('translator.loader_3')) && false ?: '_'};
193+
})));
194+
195+
$instance->addResource('db', $a, 'nl');
196+
$instance->addResource('db', $a, 'en');
197+
198+
return $instance;
199+
}
200+
100201
/**
101202
* Gets the 'baz_service' service.
102203
*

0 commit comments

Comments
 (0)