[TASK] Introduce Matcher for required arguments in static calls 68/55268/6
authorAnja Leichsenring <aleichsenring@ab-softlab.de>
Fri, 5 Jan 2018 07:13:52 +0000 (08:13 +0100)
committerChristian Kuhn <lolli@schwarzbu.ch>
Fri, 12 Jan 2018 11:44:26 +0000 (12:44 +0100)
Add a matcher that is able to detect hits of method calls that do
not comply to a minimum amount of arguments, where new arguments
have been introduced or previously optional ones have been made
mandatory.

Resolves: #83471
Relates: #82899
Releases: master
Change-Id: I96eedb06bfcd88a8927902060224b12b64f6f470
Reviewed-on: https://review.typo3.org/55268
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Reiner Teubner <rteubner@me.com>
Tested-by: Reiner Teubner <rteubner@me.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
typo3/sysext/core/Documentation/Changelog/9.0/Breaking-82899-MoreRestrictingChecksForAPIMethodsInExtensionManagementUtility.rst
typo3/sysext/install/Classes/Controller/UpgradeController.php
typo3/sysext/install/Classes/ExtensionScanner/Php/Matcher/AbstractCoreMatcher.php
typo3/sysext/install/Classes/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcher.php [new file with mode: 0644]
typo3/sysext/install/Configuration/ExtensionScanner/Php/MethodArgumentRequiredStaticMatcher.php [new file with mode: 0644]
typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/Fixtures/MethodArgumentRequiredStaticMatcherFixture.php [new file with mode: 0644]
typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcherTest.php [new file with mode: 0644]

index 4546384..3b3bd2f 100644 (file)
@@ -40,4 +40,4 @@ Migration
 Add the required parameters to the API calls in your extension registration files, typically
 located within :file:`ext_localconf.php`, :file:`ext_tables.php` or :file:`Configuration/TCA/*` of a extension.
 
-.. index:: PHP-API, NotScanned
+.. index:: PHP-API, PartiallyScanned
index d84e580..766bd4c 100644 (file)
@@ -46,6 +46,7 @@ use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodAnnotationMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodArgumentDroppedMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodArgumentDroppedStaticMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodArgumentRequiredMatcher;
+use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodArgumentRequiredStaticMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodArgumentUnusedMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodCallMatcher;
 use TYPO3\CMS\Install\ExtensionScanner\Php\Matcher\MethodCallStaticMatcher;
@@ -130,6 +131,10 @@ class UpgradeController extends AbstractController
             'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/MethodArgumentRequiredMatcher.php',
         ],
         [
+            'class' => MethodArgumentRequiredStaticMatcher::class,
+            'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/MethodArgumentRequiredStaticMatcher.php',
+        ],
+        [
             'class' => MethodArgumentUnusedMatcher::class,
             'configurationFile' => 'EXT:install/Configuration/ExtensionScanner/Php/MethodArgumentUnusedMatcher.php',
         ],
index 90861bf..497a535 100644 (file)
@@ -89,7 +89,7 @@ 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 on .rst file
+            // 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,
diff --git a/typo3/sysext/install/Classes/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcher.php b/typo3/sysext/install/Classes/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcher.php
new file mode 100644 (file)
index 0000000..f3978b4
--- /dev/null
@@ -0,0 +1,107 @@
+<?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\StaticCall;
+use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Name\FullyQualified;
+use TYPO3\CMS\Install\ExtensionScanner\CodeScannerInterface;
+
+/**
+ * Find usages of static method calls which gained new mandatory arguments.
+ * This is a "strong" match if class name is given and "weak" if not.
+ */
+class MethodArgumentRequiredStaticMatcher extends AbstractCoreMatcher implements CodeScannerInterface
+{
+    /**
+     * Prepare $this->flatMatcherDefinitions once and validate config
+     *
+     * @param array $matcherDefinitions Incoming main configuration
+     */
+    public function __construct(array $matcherDefinitions)
+    {
+        $this->matcherDefinitions = $matcherDefinitions;
+        $this->validateMatcherDefinitions(['numberOfMandatoryArguments', 'maximumNumberOfArguments']);
+        $this->initializeFlatMatcherDefinitions();
+    }
+
+    /**
+     * Called by PhpParser.
+     * Test for "::function($1, $2, $3)" (strong match)
+     *
+     * @param Node $node
+     */
+    public function enterNode(Node $node)
+    {
+        // Match static method call
+        if (!$this->isFileIgnored($node)
+            && !$this->isLineIgnored($node)
+            && $node instanceof StaticCall
+        ) {
+            $isArgumentUnpackingUsed = $this->isArgumentUnpackingUsed($node->args);
+
+            if ($node->class instanceof FullyQualified) {
+                // 'Foo\Bar::aMethod()' -> strong match
+                $fqdnClassWithMethod = $node->class->toString() . '::' . $node->name;
+                $numberOfArguments = count($node->args);
+                if (!$isArgumentUnpackingUsed
+                    && in_array($fqdnClassWithMethod, array_keys($this->matcherDefinitions), true)
+                    && $numberOfArguments < $this->matcherDefinitions[$fqdnClassWithMethod]['numberOfMandatoryArguments']
+                    // maximum number of arguments is just a measure agains false positives
+                    && $numberOfArguments <= $this->matcherDefinitions[$fqdnClassWithMethod]['maximumNumberOfArguments']
+                ) {
+                    $this->matches[] = [
+                        'restFiles' => $this->matcherDefinitions[$fqdnClassWithMethod]['restFiles'],
+                        'line' => $node->getAttribute('startLine'),
+                        'message' => 'Method "' . $node->name . '()" needs at least '
+                            . $this->matcherDefinitions[$fqdnClassWithMethod]['numberOfMandatoryArguments']
+                            . ' arguments.',
+                        'indicator' => 'strong',
+                    ];
+                }
+            } elseif ($node->class instanceof Variable
+                && in_array($node->name, array_keys($this->flatMatcherDefinitions), true)
+            ) {
+                $match = [
+                    'restFiles' => [],
+                    'line' => $node->getAttribute('startLine'),
+                    'indicator' => 'weak',
+                ];
+
+                $numberOfArguments = count($node->args);
+                $isPossibleMatch = false;
+                foreach ($this->flatMatcherDefinitions[$node->name]['candidates'] as $candidate) {
+                    // A method call is considered a match if it is not called with argument unpacking
+                    // and number of used arguments is lesser than numberOfMandatoryArguments
+                    if (!$isArgumentUnpackingUsed
+                        && $numberOfArguments < $candidate['numberOfMandatoryArguments']
+                        // maximum number of arguments is just a measure agains false positives
+                        && $numberOfArguments <= $candidate['maximumNumberOfArguments']
+                    ) {
+                        $isPossibleMatch = true;
+                        $match['message'] = 'Method "' . $node->name . '()" needs at least '
+                            . $candidate['numberOfMandatoryArguments'] . ' arguments.';
+                        $match['restFiles'] = array_unique(array_merge($match['restFiles'], $candidate['restFiles']));
+                    }
+                }
+                if ($isPossibleMatch) {
+                    $this->matches[] = $match;
+                }
+            }
+        }
+    }
+}
diff --git a/typo3/sysext/install/Configuration/ExtensionScanner/Php/MethodArgumentRequiredStaticMatcher.php b/typo3/sysext/install/Configuration/ExtensionScanner/Php/MethodArgumentRequiredStaticMatcher.php
new file mode 100644 (file)
index 0000000..76f2d07
--- /dev/null
@@ -0,0 +1,10 @@
+<?php
+return [
+    'TYPO3\CMS\Core\Utility\ExtensionManagementUtility::addNavigationComponent' => [
+        'numberOfMandatoryArguments' => 3,
+        'maximumNumberOfArguments' => 3,
+        'restFiles' => [
+            'Breaking-82899-MoreRestrictingChecksForAPIMethodsInExtensionManagementUtility.rst',
+        ],
+    ]
+];
diff --git a/typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/Fixtures/MethodArgumentRequiredStaticMatcherFixture.php b/typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/Fixtures/MethodArgumentRequiredStaticMatcherFixture.php
new file mode 100644 (file)
index 0000000..b7895b7
--- /dev/null
@@ -0,0 +1,44 @@
+<?php
+declare(strict_types=1);
+namespace TYPO3\CMS\Install\Tests\Unit\ExtensionScanner\Php\Matcher\Fixtures;
+
+/*
+ * 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 TYPO3\CMS\Core\Utility\ExtensionManagementUtility;
+
+/**
+ * Fixture file
+ */
+class MethodArgumentRequiredStaticMatcherFixture
+{
+    public function aMethod()
+    {
+        // Match: addNavigationComponent() uses less than three arguments
+        \TYPO3\CMS\Core\Utility\ExtensionManagementUtility::addNavigationComponent('foo', 'bar');
+        // Match: Works with shortened class name, too
+        ExtensionManagementUtility::addNavigationComponent('foo', 'bar');
+        // Match: Weak match on method name
+        $foo::addNavigationComponent('foo', 'bar');
+
+        // No match: Three args are ok
+        ExtensionManagementUtility::addNavigationComponent('foo', 'bar', 'baz');
+        // No match: Argument unpacking used
+        ExtensionManagementUtility::addNavigationComponent('foo', ...'bar');
+        // No match: All needed args are given
+        $foo::addNavigationComponent('foo', 'bar', 'baz');
+        $foo::addNavigationComponent(...'foo');
+        // @extensionScannerIgnoreLine
+        ExtensionManagementUtility::addNavigationComponent('foo', 'bar');
+    }
+}
diff --git a/typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcherTest.php b/typo3/sysext/install/Tests/Unit/ExtensionScanner/Php/Matcher/MethodArgumentRequiredStaticMatcherTest.php
new file mode 100644 (file)
index 0000000..ba9e9f6
--- /dev/null
@@ -0,0 +1,215 @@
+<?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\Matcher\MethodArgumentRequiredStaticMatcher;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+/**
+ * Test case
+ */
+class MethodArgumentRequiredStaticMatcherTest extends UnitTestCase
+{
+    /**
+     * @test
+     */
+    public function hitsFromFixtureAreFound()
+    {
+        $parser = (new ParserFactory())->create(ParserFactory::PREFER_PHP7);
+        $fixtureFile = __DIR__ . '/Fixtures/MethodArgumentRequiredStaticMatcherFixture.php';
+        $statements = $parser->parse(file_get_contents($fixtureFile));
+
+        $traverser = new NodeTraverser();
+        $traverser->addVisitor(new NameResolver());
+
+        $configuration = [
+            'TYPO3\CMS\Core\Utility\ExtensionManagementUtility::addNavigationComponent' => [
+                'numberOfMandatoryArguments' => 3,
+                'maximumNumberOfArguments' => 3,
+                'restFiles' => [
+                    'Breaking-82899-MoreRestrictingChecksForAPIMethodsInExtensionManagementUtility.rst',
+                ],
+            ],
+        ];
+        $subject = new MethodArgumentRequiredStaticMatcher($configuration);
+        $traverser->addVisitor($subject);
+        $traverser->traverse($statements);
+        $expectedHitLineNumbers = [
+            28,
+            30,
+            32,
+        ];
+        $actualHitLineNumbers = [];
+        foreach ($subject->getMatches() as $hit) {
+            $actualHitLineNumbers[] = $hit['line'];
+        }
+        $this->assertEquals($expectedHitLineNumbers, $actualHitLineNumbers);
+    }
+
+    /**
+     * @return array
+     */
+    public function matchesReturnsExpectedRestFilesDataProvider()
+    {
+        return [
+            'two rest candidates with same number of arguments' => [
+                [
+                    'Foo::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'Foo-1.rst',
+                            'Foo-2.rst',
+                        ],
+                    ],
+                    'Bar::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'Bar-1.rst',
+                            'Bar-2.rst',
+                        ],
+                    ],
+                ],
+                '<?php
+                $someVar::aMethod();',
+                [
+                    0 => [
+                        'restFiles' => [
+                            'Foo-1.rst',
+                            'Foo-2.rst',
+                            'Bar-1.rst',
+                            'Bar-2.rst',
+                        ],
+                    ],
+                ],
+            ],
+            'three candidates, first and second hits' => [
+                [
+                    'Foo::aMethod' => [
+                        'numberOfMandatoryArguments' => 3,
+                        'maximumNumberOfArguments' => 3,
+                        'restFiles' => [
+                            'Foo-1.rst',
+                        ],
+                    ],
+                    'Bar::aMethod' => [
+                        'numberOfMandatoryArguments' => 3,
+                        'maximumNumberOfArguments' => 3,
+                        'restFiles' => [
+                            'Bar-1.rst',
+                        ],
+                    ],
+                    'FooBar::aMethod' => [
+                        'numberOfMandatoryArguments' => 2,
+                        'maximumNumberOfArguments' => 3,
+                        'restFiles' => [
+                            'FooBar-1.rst',
+                        ],
+                    ],
+                ],
+                '<?php
+                $someVar::aMethod(\'foo\', \'bar\');',
+                [
+                    0 => [
+                        'restFiles' => [
+                            'Foo-1.rst',
+                            'Bar-1.rst',
+                        ],
+                    ],
+                ],
+            ],
+            'one candidate, does not hit, enough arguments given' => [
+                [
+                    'Foo::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'Foo-1.rst',
+                        ],
+                    ],
+                ],
+                '<?php
+                $someVar::aMethod(\'foo\');',
+                [], // no hit
+            ],
+            'no match, method call using argument unpacking' => [
+                [
+                    'Foo::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'Foo-1.rst',
+                        ],
+                    ],
+                ],
+                '<?php
+                $args = [\'arg1\', \'arg2\', \'arg3\'];
+                $someVar::aMethod(...$args);',
+                [],
+            ],
+            'double linked .rst file is returned only once' => [
+                [
+                    'Foo::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'aRest.rst',
+                        ],
+                    ],
+                    'Bar::aMethod' => [
+                        'numberOfMandatoryArguments' => 1,
+                        'maximumNumberOfArguments' => 1,
+                        'restFiles' => [
+                            'aRest.rst',
+                        ],
+                    ],
+                ],
+                '<?php
+                $someVar::aMethod();',
+                [
+                    0 => [
+                        'restFiles' => [
+                            'aRest.rst',
+                        ],
+                    ],
+                ],
+            ],
+       ];
+    }
+
+    /**
+     * @test
+     * @dataProvider matchesReturnsExpectedRestFilesDataProvider
+     */
+    public function matchesReturnsExpectedRestFiles(array $configuration, string $phpCode, array $expected)
+    {
+        $parser = (new ParserFactory())->create(ParserFactory::ONLY_PHP7);
+        $statements = $parser->parse($phpCode);
+
+        $subject = new MethodArgumentRequiredStaticMatcher($configuration);
+
+        $traverser = new NodeTraverser();
+        $traverser->addVisitor($subject);
+        $traverser->traverse($statements);
+
+        $result = $subject->getMatches();
+        $this->assertEquals($expected[0]['restFiles'], $result[0]['restFiles']);
+    }
+}