8000 [DI] Add compiler pass to check arguments type hint by alcalyn · Pull Request #27825 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Expand Up @@ -60,6 +60,10 @@
<tag name="console.command" command="debug:container" />
</service>

<service id="console.command.container_lint" class="Symfony\Bundle\FrameworkBundle\Command\ContainerLintCommand">
<tag name="console.command" command="lint:container" />
</service>

<service id="console.command.debug_autowiring" class="Symfony\Bundle\FrameworkBundle\Command\DebugAutowiringCommand">
<tag name="console.command" command="debug:autowiring" />
</service>
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* added `ServiceSubscriberTrait`
* added `ServiceLocatorArgument` for creating optimized service-locators
* added `CheckTypeHintsPass` to check injected parameters type during compilation

4.1.0
-----
Expand Down
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)
Copy link
Member

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

{
$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]);
Copy link
Contributor
@GuilhemN GuilhemN Jul 5, 2018

Choose a reason for hiding this comment

The 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(
Copy link

Choose a reason for hiding this comment

The 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) {
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iterable does seem to be case insensitive as it's a php builtin type, however Traversable is case sensitive indeed.

return;
}

$checkFunction = 'is_'.$parameter->getType()->getName();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sprintf('is_%s', $parameter->getType()->getName()) can be used here. Looks more neat than appending


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)
{
if (is_array($factory = $value->getFactory())) {
list($class, $method) = $factory;
if ($class instanceof Reference) {
$class = $this->container->findDefinition((string) $class)->getClass();
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 if (is_array($factory = $value->getFactory())) {, so I'd say factories uses cases are not enough covered

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me too we should always return $value->getClass(), we only need to handle factories for the constructor and it's done by getConstructor. Since getClassName is used to check whether a definition implements the right type hint, it should return the definition class.

} 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));
}
}
Loading
0