Commit d6ad3ad1 authored by Oliver Hader's avatar Oliver Hader Committed by Oliver Hader
Browse files

[BUGFIX] Handle constructor arguments in extension scanner correctly

Handling constructor arguments in extension scanner did not work since
constructors are not directly called like a method would be called.
Constructors are invoked instead using the `new` statement. Besides
that `GeneratorClassResolver` only supported string class names (like
`'Example\\MyClass'`) when being invoked using
`GeneralUtility::makeInstance`.

Scanner configurations related to `__construct` invocation (in terms
of creating a new class instance) are now handled in a dedicated new
`ConstructorArgumentMatcher`. Besides that, `GeneratorClassResolver` now
supports `GeneralUtility::makeInstance(Example\MyClass::class, 123)`
as well.

`NameResolver` and `GeneratorClassesResolver` are executed in two
consecutive `NodeTraverser` instances - otherwise class names cannot
be resolved in a reliable way.

Resolves: #90722
Releases: master, 9.5
Change-Id: I0154b04674d2637fda817d39a1136c2c8fa389d8
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/63676


Tested-by: default avatarTYPO3com <noreply@typo3.com>
Tested-by: Andreas Fernandez's avatarAndreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
Reviewed-by: Andreas Fernandez's avatarAndreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
Reviewed-by: Georg Ringer's avatarGeorg Ringer <georg.ringer@gmail.com>
parent e787cedc
......@@ -43,6 +43,7 @@ use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ArrayGlobalMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ClassConstantMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ClassNameMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ConstantMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ConstructorArgumentMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\FunctionCallMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\InterfaceMethodChangedMatcher;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodAnnotationMatcher;
......@@ -129,6 +130,10 @@ class UpgradeController extends AbstractController
'class' => ConstantMatcher::class,
'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/ConstantMatcher.php',
],
[
'class' => ConstructorArgumentMatcher::class,
'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/ConstructorArgumentMatcher.php',
],
[
'class' => PropertyAnnotationMatcher::class,
'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/PropertyAnnotationMatcher.php',
......@@ -685,11 +690,17 @@ class UpgradeController extends AbstractController
// Parse PHP file to AST and traverse tree calling visitors
$statements = $parser->parse(file_get_contents($absoluteFilePath));
$traverser = new NodeTraverser();
// The built in NameResolver translates class names shortened with 'use' to fully qualified
// class names at all places. Incredibly useful for us and added as first visitor.
// IMPORTANT: first process completely to resolve fully qualified names of arguments
// (otherwise GeneratorClassesResolver will NOT get reliable results)
$traverser = new NodeTraverser();
$traverser->addVisitor(new NameResolver());
// Understand makeInstance('My\\Package\\Foo\\Bar') as fqdn class name in first argument
$statements = $traverser->traverse($statements);
// IMPORTANT: second process to actually work on the pre-resolved statements
$traverser = new NodeTraverser();
// Understand GeneralUtility::makeInstance('My\\Package\\Foo\\Bar') as fqdn class name in first argument
$traverser->addVisitor(new GeneratorClassesResolver());
// Count ignored lines, effective code lines, ...
$statistics = new CodeStatistics();
......
......@@ -16,10 +16,15 @@ namespace TYPO3\CMS\Install\ExtensionScanner\Php;
*/
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Expr\New_;
use PhpParser\Node\Expr\StaticCall;
use PhpParser\Node\Name\FullyQualified;
use PhpParser\Node\Scalar\String_;
use PhpParser\NodeVisitorAbstract;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\AbstractCoreMatcher;
/**
* Create a fully qualified class name object from first argument of
......@@ -42,13 +47,34 @@ class GeneratorClassesResolver extends NodeVisitorAbstract
{
if ($node instanceof StaticCall
&& $node->class instanceof FullyQualified
&& $node->class->toString() === \TYPO3\CMS\Core\Utility\GeneralUtility::class
&& $node->class->toString() === GeneralUtility::class
&& $node->name->name === 'makeInstance'
&& isset($node->args[0]->value)
&& $node->args[0]->value instanceof String_
&& $node->args[0]->value instanceof Expr
) {
$newSubNode = new FullyQualified($node->args[0]->value->value, $node->args[0]->value->getAttributes());
$node->args[0]->value = $newSubNode;
if (null === $className = $this->resolveClassName($node->args[0]->value)) {
return;
}
$node->args[0]->value = $className;
$node->setAttribute(AbstractCoreMatcher::NODE_RESOLVED_AS, new New_(
$className,
// remove first argument (class name)
array_slice($node->args, 1),
$node->getAttributes()
));
}
}
protected function resolveClassName(Expr $value): ?FullyQualified
{
if ($value instanceof String_) {
// 'TYPO3\\CMS\\ClassName'
return new FullyQualified($value->value, $value->getAttributes());
}
if ($value instanceof ClassConstFetch && $value->class instanceof FullyQualified) {
// \TYPO3\CMS\ClassName::class
return $value->class;
}
return null;
}
}
......@@ -30,6 +30,8 @@ use TYPO3\CMS\Install\ExtensionScanner\CodeScannerInterface;
*/
abstract class AbstractCoreMatcher extends NodeVisitorAbstract implements CodeScannerInterface
{
public const NODE_RESOLVED_AS = 'nodeResolvedAs';
/**
* Incoming main configuration array.
*
......@@ -90,31 +92,36 @@ abstract class AbstractCoreMatcher extends NodeVisitorAbstract implements CodeSc
protected function validateMatcherDefinitions(array $requiredArrayKeys = [])
{
foreach ($this->matcherDefinitions as $key => $matcherDefinition) {
// Each config must point to at least one .rst file
if (empty($matcherDefinition['restFiles'])) {
throw new \InvalidArgumentException(
'Each configuration must have at least one referenced "restFiles" entry. Offending key: ' . $key,
1500496068
);
}
foreach ($matcherDefinition['restFiles'] as $file) {
if (empty($file)) {
throw new \InvalidArgumentException(
'Empty restFiles definition',
1500735983
);
}
}
// Config broken if not all required array keys are specified in config
$sharedArrays = array_intersect(array_keys($matcherDefinition), $requiredArrayKeys);
if ($sharedArrays !== $requiredArrayKeys) {
$missingKeys = array_diff($requiredArrayKeys, array_keys($matcherDefinition));
$this->validateMatcherDefinitionKeys($key, $matcherDefinition, $requiredArrayKeys);
}
}
protected function validateMatcherDefinitionKeys(string $key, array $matcherDefinition, array $requiredArrayKeys = []): void
{
// Each config must point to at least one .rst file
if (empty($matcherDefinition['restFiles'])) {
throw new \InvalidArgumentException(
'Each configuration must have at least one referenced "restFiles" entry. Offending key: ' . $key,
1500496068
);
}
foreach ($matcherDefinition['restFiles'] as $file) {
if (empty($file)) {
throw new \InvalidArgumentException(
'Required matcher definitions missing: ' . implode(', ', $missingKeys) . ' offending key: ' . $key,
1500492001
'Empty restFiles definition',
1500735983
);
}
}
// Config broken if not all required array keys are specified in config
$sharedArrays = array_intersect(array_keys($matcherDefinition), $requiredArrayKeys);
if (count($sharedArrays) !== count($requiredArrayKeys)) {
$missingKeys = array_diff($requiredArrayKeys, array_keys($matcherDefinition));
throw new \InvalidArgumentException(
'Required matcher definitions missing: ' . implode(', ', $missingKeys) . ' offending key: ' . $key,
1500492001
);
}
}
/**
......
<?php
declare(strict_types = 1);
namespace TYPO3\CMS\Install\ExtensionScanner\Php\Matcher;
/*
* This file is part of the TYPO3 CMS project.
*
* It is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License, either version 2
* of the License, or any later version.
*
* For the full copyright and license information, please read the
* LICENSE.txt file that was distributed with this source code.
*
* The TYPO3 project - inspiring people to share!
*/
use PhpParser\Node;
use PhpParser\Node\Expr\ConstFetch;
use PhpParser\Node\Expr\New_;
/**
* Finds invocations to class constructors and the amount of passed arguments.
* This matcher supports direct `new MyClass(123)` invocations as well as delegated
* calls to `GeneralUtility::makeInstance(MyClass::class, 123)` using `GeneratorClassResolver`.
*
* These configuration property names are handled independently:
* + numberOfMandatoryArguments
* + maximumNumberOfArguments
* + unusedArgumentNumbers
*
* @internal This class is only meant to be used within EXT:install and is not part of the TYPO3 Core API.
*/
class ConstructorArgumentMatcher extends AbstractCoreMatcher
{
protected const TOPIC_TYPE_REQUIRED = 'required';
protected const TOPIC_TYPE_DROPPED = 'dropped';
protected const TOPIC_TYPE_CALLED = 'called';
protected const TOPIC_TYPE_UNUSED = 'unused';
/**
* Prepare $this->flatMatcherDefinitions once and validate config
*
* @param array $matcherDefinitions Incoming main configuration
*/
public function __construct(array $matcherDefinitions)
{
$this->matcherDefinitions = $matcherDefinitions;
$this->validateMatcherDefinitionsTopicRequirements([
self::TOPIC_TYPE_REQUIRED => ['numberOfMandatoryArguments'],
self::TOPIC_TYPE_DROPPED => ['maximumNumberOfArguments'],
self::TOPIC_TYPE_CALLED => ['numberOfMandatoryArguments', 'maximumNumberOfArguments'],
self::TOPIC_TYPE_UNUSED => ['unusedArgumentNumbers'],
]);
}
/**
* Called by PhpParser.
* Test for "->deprecated()" (weak match)
*
* @param Node $node
*/
public function enterNode(Node $node)
{
if ($this->isFileIgnored($node) || $this->isLineIgnored($node)) {
return;
}
$resolvedNode = $node->getAttribute(self::NODE_RESOLVED_AS, null) ?? $node;
if (!$resolvedNode instanceof New_
|| !array_key_exists((string)$resolvedNode->class, $this->matcherDefinitions)
) {
return;
}
// A method call is considered a match if it is not called with argument unpacking
// and number of used arguments is lower than numberOfMandatoryArguments
if ($this->isArgumentUnpackingUsed($resolvedNode->args)) {
return;
}
// $node reflects invocation, e.g. `GeneralUtility::makeInstance(MyClass::class, 123)`
// $resolvedNode reflects resolved and actual usage, e.g. `new MyClass(123)`
$this->handleRequiredArguments($node, $resolvedNode);
$this->handleDroppedArguments($node, $resolvedNode);
$this->handleCalledArguments($node, $resolvedNode);
$this->handleUnusedArguments($node, $resolvedNode);
}
/**
* @param Node $node reflects invocation, e.g. `GeneralUtility::makeInstance(MyClass::class, 123)`
* @param Node $resolvedNode reflects resolved and actual usage, e.g. `new MyClass(123)`
* @return bool
*/
protected function handleRequiredArguments(Node $node, Node $resolvedNode): bool
{
$className = (string)$resolvedNode->class;
$candidate = $this->matcherDefinitions[$className][self::TOPIC_TYPE_REQUIRED] ?? null;
$mandatoryArguments = $candidate['numberOfMandatoryArguments'] ?? null;
$numberOfArguments = count($resolvedNode->args);
if ($candidate === null || $numberOfArguments >= $mandatoryArguments) {
return false;
}
$this->matches[] = [
'restFiles' => $candidate['restFiles'],
'line' => $node->getAttribute('startLine'),
'message' => sprintf(
'%s::__construct requires at least %d arguments (%d given).',
$className,
$mandatoryArguments,
$numberOfArguments
),
'indicator' => 'strong',
];
return true;
}
/**
* @param Node $node reflects invocation, e.g. `GeneralUtility::makeInstance(MyClass::class, 123)`
* @param Node $resolvedNode reflects resolved and actual usage, e.g. `new MyClass(123)`
* @return bool
*/
protected function handleDroppedArguments(Node $node, Node $resolvedNode): bool
{
$className = (string)$resolvedNode->class;
$candidate = $this->matcherDefinitions[$className][self::TOPIC_TYPE_DROPPED] ?? null;
$maximumArguments = $candidate['maximumNumberOfArguments'] ?? null;
$numberOfArguments = count($resolvedNode->args);
if ($candidate === null || $numberOfArguments <= $maximumArguments) {
return false;
}
$this->matches[] = [
'restFiles' => $candidate['restFiles'],
'line' => $node->getAttribute('startLine'),
'message' => sprintf(
'%s::__construct supports only %d arguments (%d given).',
$className,
$maximumArguments,
$numberOfArguments
),
'indicator' => 'strong',
];
return true;
}
/**
* @param Node $node reflects invocation, e.g. `GeneralUtility::makeInstance(MyClass::class, 123)`
* @param Node $resolvedNode reflects resolved and actual usage, e.g. `new MyClass(123)`
* @return bool
*/
protected function handleCalledArguments(Node $node, Node $resolvedNode): bool
{
$className = (string)$resolvedNode->class;
$candidate = $this->matcherDefinitions[$className][self::TOPIC_TYPE_CALLED] ?? null;
$isArgumentUnpackingUsed = $this->isArgumentUnpackingUsed($resolvedNode->args);
$mandatoryArguments = $candidate['numberOfMandatoryArguments'] ?? null;
$maximumArguments = $candidate['maximumNumberOfArguments'] ?? null;
$numberOfArguments = count($resolvedNode->args);
if ($candidate === null
|| !$isArgumentUnpackingUsed
&& ($numberOfArguments < $mandatoryArguments || $numberOfArguments > $maximumArguments)) {
return false;
}
$this->matches[] = [
'restFiles' => $candidate['restFiles'],
'line' => $node->getAttribute('startLine'),
'message' => sprintf(
'%s::__construct being called (%d arguments given).',
$className,
$numberOfArguments
),
'indicator' => 'weak',
];
return true;
}
/**
* @param Node $node reflects invocation, e.g. `GeneralUtility::makeInstance(MyClass::class, 123)`
* @param Node $resolvedNode reflects resolved and actual usage, e.g. `new MyClass(123)`
* @return bool
*/
protected function handleUnusedArguments(Node $node, Node $resolvedNode): bool
{
$className = (string)$resolvedNode->class;
$candidate = $this->matcherDefinitions[$className][self::TOPIC_TYPE_UNUSED] ?? null;
// values in array (if any) are actual position counts
// e.g. `[2, 4]` refers to internal argument indexes `[1, 3]`
$unusedArgumentPositions = $candidate['unusedArgumentNumbers'] ?? null;
if ($candidate === null || empty($unusedArgumentPositions)) {
return false;
}
$arguments = $resolvedNode->args;
// keeping positions having argument values that are not null
$unusedArgumentPositions = array_filter(
$unusedArgumentPositions,
function (int $position) use ($arguments) {
$index = $position - 1;
return isset($arguments[$index]->value)
&& !$arguments[$index]->value instanceof ConstFetch
&& (
!isset($arguments[$index]->value->name->name->parts[0])
|| $arguments[$index]->value->name->name->parts[0] !== null
);
}
);
if (empty($unusedArgumentPositions)) {
return false;
}
$this->matches[] = [
'restFiles' => $candidate['restFiles'],
'line' => $node->getAttribute('startLine'),
'message' => sprintf(
'%s::__construct was called with argument positions %s not being null.',
$className,
implode(', ', $unusedArgumentPositions)
),
'indicator' => 'strong',
];
return true;
}
protected function validateMatcherDefinitionsTopicRequirements(array $topicRequirements): void
{
foreach ($this->matcherDefinitions as $key => $matcherDefinition) {
foreach ($topicRequirements as $topic => $requiredArrayKeys) {
if (empty($matcherDefinition[$topic])) {
continue;
}
$this->validateMatcherDefinitionKeys($key, $matcherDefinition[$topic], $requiredArrayKeys);
}
}
}
}
<?php
return [
'TYPO3\CMS\Core\Package\PackageManager' => [
'required' => [
'numberOfMandatoryArguments' => 1,
'maximumNumberOfArguments' => 1,
'restFiles' => [
'Deprecation-84109-DeprecateDependencyResolver.rst',
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
]
],
'TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController' => [
'dropped' => [
'maximumNumberOfArguments' => 7,
'restFiles' => [
'Breaking-82572-RDCTFunctionalityRemoved.rst',
],
],
'unused' => [
'unusedArgumentNumbers' => [ 4 ],
'restFiles' => [
'Deprecation-86002-TSFEConstructorWithNo_cacheArgument.rst',
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
],
],
'TYPO3\CMS\Extbase\Persistence\Generic\Mapper\DataMapper' => [
'called' => [
'numberOfMandatoryArguments' => 7,
'maximumNumberOfArguments' => 8,
'restFiles' => [
'Breaking-87305-UseConstructorInjectionInDataMapper.rst',
'Deprecation-87305-UseConstructorInjectionInDataMapper.rst',
],
]
],
];
......@@ -112,12 +112,6 @@ return [
'Breaking-55298-DecoupledHistoryFunctionality.rst',
],
],
'TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController->__construct' => [
'maximumNumberOfArguments' => 7,
'restFiles' => [
'Breaking-82572-RDCTFunctionalityRemoved.rst',
],
],
'TYPO3\CMS\Core\DataHandling\DataHandler->printLogErrorMessages' => [
'maximumNumberOfArguments' => 0,
'restFiles' => [
......
......@@ -7,14 +7,6 @@ return [
'Breaking-80700-DeprecatedFunctionalityRemoved.rst',
],
],
'TYPO3\CMS\Core\Package\PackageManager->__construct' => [
'numberOfMandatoryArguments' => 1,
'maximumNumberOfArguments' => 1,
'restFiles' => [
'Deprecation-84109-DeprecateDependencyResolver.rst',
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
],
'TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController->calculateLinkVars' => [
'numberOfMandatoryArguments' => 1,
'maximumNumberOfArguments' => 1,
......
......@@ -34,11 +34,4 @@ return [
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
],
'TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController->__construct' => [
'unusedArgumentNumbers' => [ 4 ],
'restFiles' => [
'Deprecation-86002-TSFEConstructorWithNo_cacheArgument.rst',
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
],
];
......@@ -4125,14 +4125,6 @@ return [
'Breaking-87193-DeprecatedFunctionalityRemoved.rst'
],
],
'TYPO3\CMS\Extbase\Persistence\Generic\Mapper\DataMapper->__construct' => [
'numberOfMandatoryArguments' => 7,
'maximumNumberOfArguments' => 8,
'restFiles' => [
'Breaking-87305-UseConstructorInjectionInDataMapper.rst',
'Deprecation-87305-UseConstructorInjectionInDataMapper.rst',
],
],
'TYPO3\CMS\Core\DataHandling\DataHandler->process_uploads' => [
'numberOfMandatoryArguments' => 1,
'maximumNumberOfArguments' => 1,
......
<?php
declare(strict_types = 1);
namespace TYPO3\CMS\Install\Tests\Unit\ExtensionScanner\Php\Matcher;
/*
* This file is part of the TYPO3 CMS project.
*
* It is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License, either version 2
* of the License, or any later version.
*
* For the full copyright and license information, please read the
* LICENSE.txt file that was distributed with this source code.
*
* The TYPO3 project - inspiring people to share!
*/
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitor\NameResolver;
use PhpParser\ParserFactory;
use TYPO3\CMS\Install\ExtensionScanner\Php\GeneratorClassesResolver;
use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\ConstructorArgumentMatcher;
use TYPO3\CMS\Install\Tests\Unit\ExtensionScanner\Php\Matcher\Fixtures\Subject;
/**
* Test case
*/
class ConstructorArgumentMatcherTest extends \PHPUnit\Framework\TestCase
{
public function hitsFromFixtureAreFoundDataProvider(): array
{
$defaults = [
'restFiles' => [
'Breaking-87193-DeprecatedFunctionalityRemoved.rst',
],
];
return [
'required' => [
[
'required' => array_merge($defaults, [
'numberOfMandatoryArguments' => 4,
]),
],
[32, 33, 34, 35, 40, 41],
],
'dropped' => [
[
'dropped' => array_merge($defaults, [
'maximumNumberOfArguments' => 2,
]),
],
[32, 33, 34, 35, 40, 41],
],
'called' => [
[
'called' => array_merge($defaults, [
'numberOfMandatoryArguments' => 1,
'maximumNumberOfArguments' => 3,
]),
],
[32, 33, 34, 35, 40, 41],
],
'unused' => [
[