[DependencyInjection] Add support for named arguments #21383
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\DependencyInjection\Compiler; | ||
|
|
||
| use Symfony\Component\DependencyInjection\Definition; | ||
| use Symfony\Component\DependencyInjection\Exception\RuntimeException; | ||
|
|
||
| /** | ||
| * Checks if arguments of methods are properly configured. | ||
| * | ||
| * @author Kévin Dunglas <dunglas@gmail.com> | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class CheckArgumentsValidityPass extends AbstractRecursivePass | ||
| { | ||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function processValue($value, $isRoot = false) | ||
| { | ||
| if (!$value instanceof Definition) { | ||
| return parent::processValue($value, $isRoot); | ||
| } | ||
|
|
||
| $i = 0; | ||
| foreach ($value->getArguments() as $k => $v) { | ||
| if ($k !== $i++) { | ||
| if (!is_int($k)) { | ||
| throw new RuntimeException(sprintf('Invalid constructor argument for service "%s": integer expected but found string "%s". Check your service definition.', $this->currentId, $k)); | ||
| } | ||
|
There was a problem hiding this comment. This is probably ok, but just to make sure we're all on the same page: it's not possible to hit this exception (unless, maybe you're doing it in a compiler pass). The There was a problem hiding this comment. True, at least not when using PassConfig - still possible if the pass is used standalone, if anyone does that. There was a problem hiding this comment. It seems like with autowiring and named args, we've really moved to almost requiring PassConfig to get a working container :). I'm not sure that's a problem - just pointing that out. |
||
|
|
||
| throw new RuntimeException(sprintf('Invalid constructor argument %d for service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $this->currentId, $i)); | ||
|
There was a problem hiding this comment. This issue is triggered because we see argument 2, and that makes us notice that arg 1 is missing. But really, the problem is entirely that argument 1 is missing. So, mentioning argument 2 in the error might be counter-productive. What about:
... and it would be even more awesome if we mentioned the argument name:
There was a problem hiding this comment. Giving the name of the argument is possible, but will add significant complexity in the code. Not sure it's worth it. There was a problem hiding this comment. The actual message looks good to me because the order matter. It makes it easier to understand the problem. |
||
| } | ||
| } | ||
|
|
||
| foreach ($value->getMethodCalls() as $methodCall) { | ||
| $i = 0; | ||
| foreach ($methodCall[1] as $k => $v) { | ||
| if ($k !== $i++) { | ||
| if (!is_int($k)) { | ||
| throw new RuntimeException(sprintf('Invalid argument for method call "%s" of service "%s": integer expected but found string "%s". Check your service definition.', $methodCall[0], $this->currentId, $k)); | ||
| } | ||
|
|
||
| throw new RuntimeException(sprintf('Invalid argument %d for method call "%s" of service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $methodCall[0], $this->currentId, $i)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\DependencyInjection\Compiler; | ||
|
|
||
| use Symfony\Component\DependencyInjection\Definition; | ||
| use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException; | ||
|
|
||
| /** | ||
| * Resolves named arguments to their corresponding numeric index. | ||
| * | ||
| * @author Kévin Dunglas <dunglas@gmail.com> | ||
| */ | ||
| class ResolveNamedArgumentsPass extends AbstractRecursivePass | ||
| { | ||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function processValue($value, $isRoot = false) | ||
| { | ||
| if (!$value instanceof Definition) { | ||
| return parent::processValue($value, $isRoot); | ||
| } | ||
|
|
||
| $parameterBag = $this->container->getParameterBag(); | ||
|
|
||
| if ($class = $value->getClass()) { | ||
| $class = $parameterBag->resolveValue($class); | ||
| } | ||
|
|
||
| $calls = $value->getMethodCalls(); | ||
| $calls[] = array('__construct', $value->getArguments()); | ||
|
|
||
| foreach ($calls as $i => $call) { | ||
| list($method, $arguments) = $call; | ||
| $method = $parameterBag->resolveValue($method); | ||
| $parameters = null; | ||
| $resolvedArguments = array(); | ||
|
|
||
| foreach ($arguments as $key => $argument) { | ||
| if (is_int($key) || '' === $key || '$' !== $key[0]) { | ||
| if (!is_int($key)) { | ||
| @trigger_error(sprintf('Using key "%s" for defining arguments of method "%s" for service "%s" is deprecated since Symfony 3.3 and will throw an exception in 4.0. Use no keys or $named arguments instead.', $key, $method, $this->currentId), E_USER_DEPRECATED); | ||
| } | ||
| $resolvedArguments[] = $argument; | ||
| continue; | ||
| } | ||
|
|
||
| $parameters = null !== $parameters ? $parameters : $this->getParameters($class, $method); | ||
|
|
||
| foreach ($parameters as $j => $p) { | ||
| if ($key === '$'.$p->name) { | ||
| $resolvedArguments[$j] = $argument; | ||
|
|
||
| continue 2; | ||
| } | ||
| } | ||
|
|
||
| throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" has no argument named "%s". Check your service definition.', $this->currentId, $class, $method, $key)); | ||
| } | ||
|
|
||
| if ($resolvedArguments !== $call[1]) { | ||
| ksort($resolvedArguments); | ||
| $calls[$i][1] = $resolvedArguments; | ||
| } | ||
| } | ||
|
|
||
| list(, $arguments) = array_pop($calls); | ||
|
|
||
| if ($arguments !== $value->getArguments()) { | ||
| $value->setArguments($arguments); | ||
| } | ||
| if ($calls !== $value->getMethodCalls()) { | ||
| $value->setMethodCalls($calls); | ||
| } | ||
|
|
||
| return parent::processValue($value, $isRoot); | ||
| } | ||
|
|
||
| /** | ||
| * @param string|null $class | ||
| * @param string $method | ||
| * | ||
| * @throws InvalidArgumentException | ||
| * | ||
| * @return array | ||
|
There was a problem hiding this comment. Should be hinted as There was a problem hiding this comment. This method has been removed during a subsequent refactoring. |
||
| */ | ||
| private function getParameters($class, $method) | ||
| { | ||
| if (!$class) { | ||
| throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId)); | ||
| } | ||
|
|
||
| if (!$r = $this->container->getReflectionClass($class)) { | ||
| throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": class "%s" does not exist.', $this->currentId, $class)); | ||
| } | ||
|
|
||
| if (!$r->hasMethod($method)) { | ||
| throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" does not exist.', $this->currentId, $class, $method)); | ||
| } | ||
|
|
||
| $method = $r->getMethod($method); | ||
| if (!$method->isPublic()) { | ||
| throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" must be public.', $this->currentId, $class, $method->name)); | ||
| } | ||
|
|
||
| return $method->getParameters(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\DependencyInjection\Tests\Compiler; | ||
|
|
||
| use Symfony\Component\DependencyInjection\Compiler\CheckArgumentsValidityPass; | ||
| use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
|
|
||
| /** | ||
| * @author Kévin Dunglas <dunglas@gmail.com> | ||
| */ | ||
| class CheckArgumentsValidityPassTest extends \PHPUnit_Framework_TestCase | ||
| { | ||
| public function testProcess() | ||
| { | ||
| $container = new ContainerBuilder(); | ||
| $definition = $container->register('foo'); | ||
| $definition->setArguments(array(null, 1, 'a')); | ||
| $definition->setMethodCalls(array( | ||
| array('bar', array('a', 'b')), | ||
| array('baz', array('c', 'd')), | ||
| )); | ||
|
|
||
| $pass = new CheckArgumentsValidityPass(); | ||
| $pass->process($container); | ||
|
|
||
| $this->assertEquals(array(null, 1, 'a'), $container->getDefinition('foo')->getArguments()); | ||
| $this->assertEquals(array( | ||
| array('bar', array('a', 'b')), | ||
| array('baz', array('c', 'd')), | ||
| ), $container->getDefinition('foo')->getMethodCalls()); | ||
| } | ||
|
|
||
| /** | ||
| * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException | ||
| * @dataProvider definitionProvider | ||
| */ | ||
| public function testException(array $arguments, array $methodCalls) | ||
| { | ||
| $container = new ContainerBuilder(); | ||
| $definition = $container->register('foo'); | ||
| $definition->setArguments($arguments); | ||
| $definition->setMethodCalls($methodCalls); | ||
|
|
||
| $pass = new CheckArgumentsValidityPass(); | ||
| $pass->process($container); | ||
| } | ||
|
|
||
| public function definitionProvider() | ||
| { | ||
| return array( | ||
| array(array(null, 'a' => 'a'), array()), | ||
| array(array(1 => 1), array()), | ||
| array(array(), array(array('baz', array(null, 'a' => 'a')))), | ||
| array(array(), array(array('baz', array(1 => 1)))), | ||
| ); | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the phpdoc of $index needs to be updated to allow string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also ChildDefinition::getArgument doesn't work with string yet. whereas you changed the parent Definition::getArgument.