-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Add compiler pass to check arguments type hint #27825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4b59201
1aefc17
cc8c4de
ddba549
9740eee
361e8a6
b305a69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
<?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\Bundle\FrameworkBundle\Command; | ||
|
||
use Symfony\Component\Console\Command\Command; | ||
use Symfony\Component\Console\Input\InputInterface; | ||
use Symfony\Component\Console\Input\InputOption; | ||
use Symfony\Component\Console\Output\OutputInterface; | ||
use Symfony\Component\Config\ConfigCache; | ||
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Compiler\CheckTypeHintsPass; | ||
use Symfony\Component\DependencyInjection\Compiler\PassConfig; | ||
use Symfony\Component\Config\FileLocator; | ||
|
||
class ContainerLintCommand extends Command | ||
{ | ||
/** | ||
* @var ContainerBuilder | ||
*/ | ||
private $containerBuilder; | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function configure() | ||
{ | ||
$this | ||
->setDescription('Lints container for services arguments type hints') | ||
->setHelp('This command will parse all your defined services and check that you are injecting service without type error based on type hints.') | ||
->addOption('only-used-services', 'o', InputOption::VALUE_NONE, 'Check only services that are used in your application') | ||
; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function execute(InputInterface $input, OutputInterface $output) | ||
{ | ||
$container = $this->getContainerBuilder(); | ||
|
||
$container->setParameter('container.build_id', 'lint_container'); | ||
|
||
$container->addCompilerPass( | ||
new CheckTypeHintsPass(), | ||
$input->getOption('only-used-services') ? PassConfig::TYPE_AFTER_REMOVING : PassConfig::TYPE_BEFORE_OPTIMIZATION | ||
); | ||
|
||
$container->compile(); | ||
} | ||
|
||
/** | ||
* Loads the ContainerBuilder from the cache. | ||
* | ||
* @return ContainerBuilder | ||
* | ||
* @throws \LogicException | ||
*/ | ||
protected function getContainerBuilder() | ||
{ | ||
if ($this->containerBuilder) { | ||
return $this->containerBuilder; | ||
} | ||
|
||
$kernel = $this->getApplication()->getKernel(); | ||
|
||
if (!$kernel->isDebug() || !(new ConfigCache($kernel->getContainer()->getParameter('debug.container.dump'), true))->isFresh()) { | ||
$buildContainer = \Closure::bind(function () { return $this->buildContainer(); }, $kernel, get_class($kernel)); | ||
$container = $buildContainer(); | ||
$container->getCompilerPassConfig()->setRemovingPasses(array()); | ||
$container->compile(); | ||
} else { | ||
(new XmlFileLoader($container = new ContainerBuilder(), new FileLocator()))->load($kernel->getContainer()->getParameter('debug.container.dump')); | ||
} | ||
|
||
return $this->containerBuilder = $container; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,184 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony pack 8000 age. | ||
* | ||
* (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\Reference; | ||
use Symfony\Component\DependencyInjection\ServiceLocator; | ||
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException; | ||
use Symfony\Component\DependencyInjection\Exception\InvalidParameterTypeHintException; | ||
use Symfony\Component\DependencyInjection\Argument\IteratorArgument; | ||
|
||
/** | ||
* Checks whether injected parameters types are compatible with type hints. | ||
* This pass should be run after all optimization passes. | ||
* So it can be added either: | ||
* * before removing (PassConfig::TYPE_BEFORE_REMOVING) so that it will check | ||
* all services, even if they are not currently used, | ||
* * after removing (PassConfig::TYPE_AFTER_REMOVING) so that it will check | ||
* only services you are using. | ||
* | ||
* @author Nicolas Grekas <p@tchwork.com> | ||
* @author Julien Maulny <jmaulny@darkmira.fr> | ||
*/ | ||
class CheckTypeHintsPass extends AbstractRecursivePass | ||
{ | ||
/** | ||
* If set to true, allows to autoload classes during compilation | ||
* in order to check type hints on parameters that are not yet loaded. | ||
* Defaults to false to prevent code loading during compilation. | ||
* | ||
* @param bool | ||
*/ | ||
private $autoload; | ||
|
||
public function __construct(bool $autoload = false) | ||
{ | ||
$this->autoload = $autoload; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function processValue($value, $isRoot = false) | ||
{ | ||
if (!$value instanceof Definition) { | ||
return parent::processValue($value, $isRoot); | ||
} | ||
|
||
if (!$this->autoload && !class_exists($className = $this->getClassName($value), false) && !interface_exists($className, false)) { | ||
return parent::processValue($value, $isRoot); | ||
} | ||
|
||
if (ServiceLocator::class === $value->getClass()) { | ||
return parent::processValue($value, $isRoot); | ||
} | ||
|
||
if (null !== $constructor = $this->getConstructor($value, false)) { | ||
$this->checkArgumentsTypeHints($constructor, $value->getArguments()); | ||
} | ||
|
||
foreach ($value->getMethodCalls() as $methodCall) { | ||
$reflectionMethod = $this->getReflectionMethod($value, $methodCall[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case an interface is used, the real implementation may have methods the interface doesn't have. Should we support this case and not throw an exception for unexistent methods for objects not instantiated by sf (factories and shared services at least)? |
||
|
||
$this->checkArgumentsTypeHints($reflectionMethod, $methodCall[1]); | ||
} | ||
|
||
return parent::processValue($value, $isRoot); | ||
} | ||
|
||
/** | ||
* Check type hints for every parameter of a method/constructor. | ||
* | ||
* @throws InvalidArgumentException on type hint incompatibility | ||
*/ | ||
private function checkArgumentsTypeHints(\ReflectionFunctionAbstract $reflectionFunction, array $configurationArguments): void | ||
{ | ||
$numberOfRequiredParameters = $reflectionFunction->getNumberOfRequiredParameters(); | ||
|
||
if (count($configurationArguments) < $numberOfRequiredParameters) { | ||
throw new InvalidArgumentException(sprintf( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation only shows an error if not enough parameters are given. Wouldn't it be nice to see where too many parameters are given. Even though the compiler will not throw an error; a warning would be nice. This, for example, may occur if someone changes the signature of the constructor without changing the service definition... This may prevent issues on the long term as someone may re-add a parameter to the signature, what will then break all implementations. |
||
'Invalid definition for service "%s": "%s::%s()" requires %d arguments, %d passed.', $this->currentId, $reflectionFunction->class, $reflectionFunction->name, $numberOfRequiredParameters, count($configurationArguments))); | ||
} | ||
|
||
$reflectionParameters = $reflectionFunction->getParameters(); | ||
$checksCount = min($reflectionFunction->getNumberOfParameters(), count($configurationArguments)); | ||
|
||
for ($i = 0; $i < $checksCount; ++$i) { | ||
if (!$reflectionParameters[$i]->hasType() || $reflectionParameters[$i]->isVariadic()) { | ||
continue; | ||
} | ||
|
||
$this->checkTypeHint($configurationArguments[$i], $reflectionParameters[$i]); | ||
} | ||
|
||
if ($reflectionFunction->isVariadic() && ($lastParameter = end($reflectionParameters))->hasType()) { | ||
$variadicParameters = array_slice($configurationArguments, $lastParameter->getPosition()); | ||
|
||
foreach ($variadicParameters as $variadicParameter) { | ||
$this->checkTypeHint($variadicParameter, $lastParameter); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Check type hints compatibility between | ||
* a definition argument and a reflection parameter. | ||
* | ||
F438 * @throws InvalidArgumentException on type hint incompatibility | ||
*/ | ||
private function checkTypeHint($configurationArgument, \ReflectionParameter $parameter): void | ||
{ | ||
$referencedDefinition = $configurationArgument; | ||
|
||
if ($referencedDefinition instanceof Reference) { | ||
$referencedDefinition = $this->container->findDefinition((string) $referencedDefinition); | ||
} | ||
|
||
if ($referencedDefinition instanceof Definition) { | ||
$class = $this->getClassName($referencedDefinition); | ||
|
||
if (!$this->autoload && !class_exists($class, false)) { | ||
return; | ||
} | ||
|
||
if (!is_a($class, $parameter->getType()->getName(), true)) { | ||
throw new InvalidParameterTypeHintException($this->currentId, null === $class ? 'null' : $class, $parameter); | ||
} | ||
} else { | ||
if (null === $configurationArgument && $parameter->allowsNull()) { | ||
return; | ||
} | ||
|
||
if ($parameter->getType()->isBuiltin() && is_scalar($configurationArgument)) { | ||
return; | ||
} | ||
|
||
if ('iterable' === $parameter->getType()->getName() && $configurationArgument instanceof IteratorArgument) { | ||
return; | ||
} | ||
|
||
if ('Traversable' === $parameter->getType()->getName() && $configurationArgument instanceof IteratorArgument) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I do not like this, as it seems that iterable and Traversable seems to be case sensitive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return; | ||
} | ||
|
||
$checkFunction = 'is_'.$parameter->getType()->getName(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
if (!$parameter->getType()->isBuiltin() || !$checkFunction($configurationArgument)) { | ||
throw new InvalidParameterTypeHintException($this->currentId, gettype($configurationArgument), $parameter); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Get class name from value that can have a factory. | ||
* | ||
* @return string|null | ||
*/ | ||
private function getClassName($value) | ||
{ | ||
|
||
list($class, $method) = $factory; | ||
if ($class instanceof Reference) { | ||
$class = $this->container->findDefinition((string) $class)->getClass(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my mistake I think: here we're looking for the class of a referenced definition - not sure the class of its factory should be used for the validation. just the class of the definition, factory or not, should be more correct. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about this part as only one test enters inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me too we should always return |
||
} elseif (null === $class) { | ||
$class = $value->getClass(); | ||
} elseif ($class instanceof Definition) { | ||
$class = $this->getClassName($class); | ||
} | ||
} else { | ||
$class = $value->getClass(); | ||
} | ||
|
||
return $class; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
<?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\Exception; | ||
|
||
/** | ||
* Thrown when trying to inject a parameter into a constructor/method | ||
* with a type that does not match type hint. | ||
* | ||
* @author Nicolas Grekas <p@tchwork.com> | ||
* @author Julien Maulny <jmaulny@darkmira.fr> | ||
*/ | ||
class InvalidParameterTypeHintException extends InvalidArgumentException | ||
{ | ||
public function __construct(string $serviceId, string $typeHint, \ReflectionParameter $parameter) | ||
{ | ||
parent::__construct(sprintf( | ||
'Invalid definition for service "%s": argument %d of "%s::%s" requires a "%s", "%s" passed.', $serviceId, $parameter->getPosition(), $parameter->getDeclaringClass()->getName(), $parameter->getDeclaringFunction()->getName(), $parameter->getType()->getName(), $typeHint)); | ||
} | ||
} |
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.
we could have a sentence telling what $autoload is useful for