From a6bfe1c6c5e457cbbaba7e6d341ac076cff3668c Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 17 Mar 2017 10:21:51 +0100 Subject: [PATCH] [DI] Remove skipping magic for autowired methods --- .../Compiler/AutowirePass.php | 78 +++++----- .../Tests/Compiler/AutowirePassTest.php | 136 +++++++++--------- .../Fixtures/AbstractGetterOverriding.php | 20 --- .../Tests/Fixtures/GetterOverriding.php | 28 ---- 4 files changed, 103 insertions(+), 159 deletions(-) delete mode 100644 src/Symfony/Component/DependencyInjection/Tests/Fixtures/AbstractGetterOverriding.php diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php index 42d4fba96d04a..baa64e6b60fdf 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php @@ -26,16 +26,6 @@ */ class AutowirePass extends AbstractRecursivePass { - /** - * @internal - */ - const MODE_REQUIRED = 1; - - /** - * @internal - */ - const MODE_OPTIONAL = 2; - private $definedTypes = array(); private $types; private $ambiguousServiceTypes = array(); @@ -170,13 +160,6 @@ private function getMethodsToAutowire(\ReflectionClass $reflectionClass) } foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC | \ReflectionMethod::IS_PROTECTED) as $reflectionMethod) { - if ($reflectionMethod->isStatic()) { - continue; - } - if ($reflectionMethod->isAbstract() && !$reflectionMethod->getNumberOfParameters()) { - $methodsToAutowire[strtolower($reflectionMethod->name)] = $reflectionMethod; - continue; - } $r = $reflectionMethod; while (true) { @@ -225,7 +208,7 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC } } - $arguments = $this->autowireMethod($reflectionMethod, $arguments, self::MODE_REQUIRED); + $arguments = $this->autowireMethod($reflectionMethod, $arguments); if ($arguments !== $call[1]) { $methodCalls[$i][1] = $arguments; @@ -233,9 +216,13 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC } foreach ($autowiredMethods as $lcMethod => $reflectionMethod) { - if ($reflectionMethod->isPublic() && $arguments = $this->autowireMethod($reflectionMethod, array(), self::MODE_OPTIONAL)) { - $methodCalls[] = array($reflectionMethod->name, $arguments); + if (!$reflectionMethod->getNumberOfParameters()) { + continue; // skip getters } + if (!$reflectionMethod->isPublic()) { + throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s::%s() must be public.', $this->currentId, $reflectionClass->name, $reflectionMethod->name)); + } + $methodCalls[] = array($reflectionMethod->name, $this->autowireMethod($reflectionMethod, array())); } return $methodCalls; @@ -246,20 +233,24 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC * * @param \ReflectionMethod $reflectionMethod * @param array $arguments - * @param int $mode * * @return array The autowired arguments * * @throws RuntimeException */ - private function autowireMethod(\ReflectionMethod $reflectionMethod, array $arguments, $mode) + private function autowireMethod(\ReflectionMethod $reflectionMethod, array $arguments) { - $didAutowire = false; // Whether any arguments have been autowired or not + $isConstructor = $reflectionMethod->isConstructor(); + + if (!$isConstructor && !$arguments && !$reflectionMethod->getNumberOfRequiredParameters()) { + throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s::%s() has only optional arguments, thus must be wired explicitly.', $this->currentId, $reflectionMethod->class, $reflectionMethod->name)); + } + foreach ($reflectionMethod->getParameters() as $index => $parameter) { if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) { continue; } - if (self::MODE_OPTIONAL === $mode && $parameter->isOptional() && !array_key_exists($index, $arguments)) { + if (!$isConstructor && $parameter->isOptional() && !array_key_exists($index, $arguments)) { break; } if (method_exists($parameter, 'isVariadic') && $parameter->isVariadic()) { @@ -271,11 +262,7 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu if (!$typeName) { // no default value? Then fail if (!$parameter->isOptional()) { - if (self::MODE_REQUIRED === $mode) { - throw new RuntimeException(sprintf('Cannot autowire service "%s": argument $%s of method %s::%s() must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $reflectionMethod->class, $reflectionMethod->name)); - } - - return array(); + throw new RuntimeException(sprintf('Cannot autowire service "%s": argument $%s of method %s::%s() must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $reflectionMethod->class, $reflectionMethod->name)); } if (!array_key_exists($index, $arguments)) { @@ -287,13 +274,12 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu } if ($value = $this->getAutowiredReference($typeName)) { - $didAutowire = true; $this->usedTypes[$typeName] = $this->currentId; } elseif ($parameter->isDefaultValueAvailable()) { $value = $parameter->getDefaultValue(); } elseif ($parameter->allowsNull()) { $value = null; - } elseif (self::MODE_REQUIRED === $mode) { + } else { if ($classOrInterface = class_exists($typeName, false) ? 'class' : (interface_exists($typeName, false) ? 'interface' : null)) { $message = sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeName, $this->currentId, $classOrInterface); } else { @@ -301,17 +287,11 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu } throw new RuntimeException($message); - } else { - return array(); } $arguments[$index] = $value; } - if (self::MODE_REQUIRED !== $mode && !$didAutowire) { - return array(); - } - // it's possible index 1 was set, then index 0, then 2, etc // make sure that we re-order so they're injected as expected ksort($arguments); @@ -330,16 +310,24 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu private function autowireOverridenGetters(array $overridenGetters, array $autowiredMethods) { foreach ($autowiredMethods as $lcMethod => $reflectionMethod) { - if (isset($overridenGetters[$lcMethod]) - || !method_exists($reflectionMethod, 'getReturnType') - || 0 !== $reflectionMethod->getNumberOfParameters() - || $reflectionMethod->isFinal() - || $reflectionMethod->returnsReference() - || !($typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod, null, true)) - || !($typeRef = $this->getAutowiredReference($typeName)) - ) { + if (isset($overridenGetters[$lcMethod]) || $reflectionMethod->getNumberOfParameters() || $reflectionMethod->isConstructor()) { continue; } + if (!$typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod, null, true)) { + $typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod); + + throw new RuntimeException(sprintf('Cannot autowire service "%s": getter %s::%s() must%s be given a return value explicitly.', $this->currentId, $reflectionMethod->class, $reflectionMethod->name, $typeName ? '' : ' have a return-type hint or')); + } + + if (!$typeRef = $this->getAutowiredReference($typeName)) { + if ($classOrInterface = class_exists($typeName, false) ? 'class' : (interface_exists($typeName, false) ? 'interface' : null)) { + $message = sprintf('Unable to autowire return type "%s" for service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeName, $this->currentId, $classOrInterface); + } else { + $message = sprintf('Cannot autowire return type of getter %s::%s() for service "%s": Class %s does not exist.', $reflectionMethod->class, $reflectionMethod->name, $this->currentId, $typeName); + } + + throw new RuntimeException($message); + } $overridenGetters[$lcMethod] = $typeRef; $this->usedTypes[$typeName] = $this->currentId; diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php index fd3e9965ce85d..77de75189be0c 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php @@ -14,8 +14,8 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\Compiler\AutowirePass; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Exception\RuntimeException; use Symfony\Component\DependencyInjection\Reference; -use Symfony\Component\DependencyInjection\Tests\Fixtures\AbstractGetterOverriding; use Symfony\Component\DependencyInjection\Tests\Fixtures\includes\FooVariadic; use Symfony\Component\DependencyInjection\Tests\Fixtures\GetterOverriding; use Symfony\Component\DependencyInjection\TypedReference; @@ -535,27 +535,6 @@ public function testTypedReference() $this->assertSame(A::class, $container->getDefinition('autowired.'.A::class)->getClass()); } - /** - * @requires PHP 7.0 - */ - public function testAbstractGetterOverriding() - { - $container = new ContainerBuilder(); - - $container - ->register('getter_overriding', AbstractGetterOverriding::class) - ->setAutowired(true) - ; - - $pass = new AutowirePass(); - $pass->process($container); - - $overridenGetters = $container->getDefinition('getter_overriding')->getOverriddenGetters(); - $this->assertEquals(array( - 'abstractgetfoo' => new Reference('autowired.Symfony\Component\DependencyInjection\Tests\Compiler\Foo'), - ), $overridenGetters); - } - /** * @requires PHP 7.1 */ @@ -707,6 +686,44 @@ public function provideAutodiscoveredAutowiringOrder() array('CannotBeAutowiredReverseOrder'), ); } + + /** + * @dataProvider provideNotWireableCalls + * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException + */ + public function testNotWireableCalls($method, $expectedMsg) + { + $container = new ContainerBuilder(); + + $foo = $container->register('foo', NotWireable::class)->setAutowired(true); + + if ($method) { + $foo->addMethodCall($method, array()); + } + + $pass = new AutowirePass(); + + if (method_exists($this, 'expectException')) { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage($expectedMsg); + } else { + $this->setExpectedException(RuntimeException::class, $expectedMsg); + } + + $pass->process($container); + } + + public function provideNotWireableCalls() + { + return array( + array('setNotAutowireable', 'Cannot autowire argument $n of method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setNotAutowireable() for service "foo": Class Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass does not exist.'), + array('setBar', 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setBar() has only optional arguments, thus must be wired explicitly.'), + array('setOptionalNotAutowireable', 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setOptionalNotAutowireable() has only optional arguments, thus must be wired explicitly.'), + array('setOptionalNoTypeHint', 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setOptionalNoTypeHint() has only optional arguments, thus must be wired explicitly.'), + array('setOptionalArgNoAutowireable', 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setOptionalArgNoAutowireable() has only optional arguments, thus must be wired explicitly.'), + array(null, 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setProtectedMethod() must be public.'), + ); + } } class Foo @@ -917,43 +934,6 @@ public function setDependencies(Foo $foo, A $a) // should be called } - /** - * @required*/ - public function setBar() - { - // should not be called - } - - /** @required prefix is on purpose */ - public function setNotAutowireable(NotARealClass $n) - { - // should not be called - } - - /** @required */ - public function setArgCannotAutowire($foo) - { - // should not be called - } - - /** @required */ - public function setOptionalNotAutowireable(NotARealClass $n = null) - { - // should not be called - } - - /** @required */ - public function setOptionalNoTypeHint($foo = null) - { - // should not be called - } - - /** @required */ - public function setOptionalArgNoAutowireable($other = 'default_val') - { - // should not be called - } - /** {@inheritdoc} */ public function setWithCallsConfigured(A $a) { @@ -965,12 +945,8 @@ public function notASetter(A $a) // should be called only when explicitly specified } - /** @required */ - protected function setProtectedMethod(A $a) - { - // should not be called - } - + /** + * @required*/ public function setChildMethodWithoutDocBlock(A $a) { } @@ -989,7 +965,7 @@ public function notASetter(A $a) // @required should be ignored when the child does not add @inheritdoc } - /** @required */ + /** @required prefix is on purpose */ public function setWithCallsConfigured(A $a) { } @@ -1012,3 +988,31 @@ public function setMultipleInstancesForOneArg(CollisionInterface $collision) // should throw an exception } } + +class NotWireable +{ + public function setNotAutowireable(NotARealClass $n) + { + } + + public function setBar() + { + } + + public function setOptionalNotAutowireable(NotARealClass $n = null) + { + } + + public function setOptionalNoTypeHint($foo = null) + { + } + + public function setOptionalArgNoAutowireable($other = 'default_val') + { + } + + /** @required */ + protected function setProtectedMethod(A $a) + { + } +} diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/AbstractGetterOverriding.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/AbstractGetterOverriding.php deleted file mode 100644 index 4f8a65aed4dd3..0000000000000 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/AbstractGetterOverriding.php +++ /dev/null @@ -1,20 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\DependencyInjection\Tests\Fixtures; - -use Symfony\Component\DependencyInjection\Tests\Compiler\Foo; - -abstract class AbstractGetterOverriding -{ - abstract public function abstractGetFoo(): Foo; - abstract public function abstractDoFoo($arg = null): Foo; -} diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/GetterOverriding.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/GetterOverriding.php index ffb46657b68a5..81abdbf542709 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/GetterOverriding.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/GetterOverriding.php @@ -35,37 +35,9 @@ protected function getBar(): Bar // should be called } - /** @required */ - public function getNoTypeHint() - { - // should not be called - } - - /** @required */ - public function getUnknown(): NotExist - { - // should not be called - } - /** @required */ public function getExplicitlyDefined(): B { // should be called but not autowired } - - /** @required */ - public function getScalar(): string - { - // should not be called - } - - final public function getFinal(): A - { - // should not be called - } - - public function &getReference(): A - { - // should not be called - } }