From eed9414de112438726149140c79c46270c24db83 Mon Sep 17 00:00:00 2001 From: Robert Egginton Date: Mon, 11 Mar 2019 13:56:03 +0000 Subject: [PATCH 1/4] New rule to warn if proxies or interceptors explicitly requested in constructors --- .../ConstructorProxyInterceptorSniff.php | 267 ++++++++++++++++++ .../ConstructorProxyInterceptorUnitTest.1.inc | 54 ++++ .../ConstructorProxyInterceptorUnitTest.2.inc | 54 ++++ .../ConstructorProxyInterceptorUnitTest.php | 48 ++++ Magento/ruleset.xml | 4 + 5 files changed, 427 insertions(+) create mode 100644 Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php create mode 100644 Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc create mode 100644 Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.2.inc create mode 100644 Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.php diff --git a/Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php b/Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php new file mode 100644 index 00000000..37e62450 --- /dev/null +++ b/Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php @@ -0,0 +1,267 @@ +getTokens(); + if ($tokens[$stackPtr]['code'] == T_USE) { + $this->processUse($phpcsFile, $stackPtr, $tokens); + } elseif ($tokens[$stackPtr]['code'] == T_FUNCTION) { + $this->processFunction($phpcsFile, $stackPtr, $tokens); + } + } + + /** + * Store plugin/proxy class names for use in matching constructor + * + * @param File $phpcsFile + * @param int $stackPtr + * @param array $tokens + */ + private function processUse( + File $phpcsFile, + $stackPtr, + $tokens + ) { + // Find end of use statement and position of AS alias if exists + $endPos = $phpcsFile->findNext(T_SEMICOLON, $stackPtr); + $asPos = $phpcsFile->findNext(T_AS, $stackPtr, $endPos); + // Find whether this use statement includes any of the warning words + $includesWarnWord = + $this->includesWarnWordsInSTRINGs( + $phpcsFile, + $stackPtr, + min($asPos, $endPos), + $tokens, + $lastWord + ); + if (! $includesWarnWord) { + return; + } + // If there is an alias then store this explicit alias for matching in constructor + if ($asPos) { + $aliasNamePos = $asPos + 2; + $this->aliases[] = strtolower($tokens[$aliasNamePos]['content']); + } + // Always store last word as alias for checking in constructor + $this->aliases[] = $lastWord; + } + + /** + * If constructor, check for proxy/plugin names and warn + * + * @param File $phpcsFile + * @param int $stackPtr + * @param array $tokens + */ + private function processFunction( + File $phpcsFile, + $stackPtr, + $tokens + ) { + // Find start and end of constructor signature based on brackets + if (! $this->getConstructorPosition($phpcsFile, $stackPtr, $tokens, $openParenth, $closeParenth)) { + return; + } + $positionInConstrSig = $openParenth; + $skipTillAfterVariable = false; + do { + // Find next part of namespace (string) or variable name + $positionInConstrSig = $phpcsFile->findNext( + [T_STRING, T_VARIABLE], + $positionInConstrSig + 1, + $closeParenth + ); + $currentTokenIsString = $tokens[$positionInConstrSig]['code'] == T_STRING; + // If we've already found a match, continue till after variable + if ($skipTillAfterVariable) { + if (!$currentTokenIsString) { + $skipTillAfterVariable = false; + } + continue; + } + // If match in namespace or variable then add warning + $name = strtolower($tokens[$positionInConstrSig]['content']); + $namesToWarn = $this->mergedNamesToWarn($currentTokenIsString); + if ($this->containsWord($namesToWarn, $name)) { + $phpcsFile->addWarning($this->warningMessage, $positionInConstrSig, $this->warningCode, [$name]); + if ($currentTokenIsString) { + $skipTillAfterVariable = true; + } + } + } while ($positionInConstrSig !== false && $positionInConstrSig < $closeParenth); + } + + /** + * Sets start and end of constructor signature or returns false + * + * @param File $phpcsFile + * @param int $stackPtr + * @param array $tokens + * @param int $openParenth + * @param int $closeParenth + * + * @return bool Whether a constructor + */ + private function getConstructorPosition( + File $phpcsFile, + $stackPtr, + array $tokens, + &$openParenth, + &$closeParenth + ) { + $methodNamePos = $phpcsFile->findNext(T_STRING, $stackPtr - 1); + if ($methodNamePos === false) { + return false; + } + // There is a method name + if ($tokens[$methodNamePos]['content'] != self::CONSTRUCT_METHOD_NAME) { + return false; + } + + // KNOWN: There is a constructor, so get position + + $openParenth = $phpcsFile->findNext(T_OPEN_PARENTHESIS, $methodNamePos); + $closeParenth = $phpcsFile->findNext(T_CLOSE_PARENTHESIS, $openParenth); + if ($openParenth === false || $closeParenth === false) { + return false; + } + + return true; + } + + /** + * Whether $name is contained in any of $haystacks + * + * @param array $haystacks + * @param string $name + * + * @return bool + */ + private function containsWord($haystacks, $name) + { + $matchWarnWord = false; + foreach ($haystacks as $warn) { + if (strpos($name, $warn) !== false) { + $matchWarnWord = true; + break; + } + } + + return $matchWarnWord; + } + + /** + * Whether warn words are included in STRING tokens in the given range + * + * Populates $lastWord in set to get usable name from namespace + * + * @param File $phpcsFile + * @param int $startPosition + * @param int $endPosition + * @param array $tokens + * @param string|null $lastWord + * + * @return bool + */ + private function includesWarnWordsInSTRINGs( + File $phpcsFile, + $startPosition, + $endPosition, + $tokens, + &$lastWord + ) { + $includesWarnWord = false; + $currentPosition = $startPosition; + do { + $currentPosition = $phpcsFile->findNext(T_STRING, $currentPosition + 1, $endPosition); + if ($currentPosition !== false) { + $word = strtolower($tokens[$currentPosition]['content']); + if ($this->containsWord($this->mergedNamesToWarn(false), $word)) { + $includesWarnWord = true; + } + $lastWord = $word; + } + } while ($currentPosition !== false && $currentPosition < $endPosition); + + return $includesWarnWord; + } + + /** + * Get array of names that if matched should raise warning. + * + * Includes aliases if required + * + * @param bool $includeAliases + * + * @return array + */ + private function mergedNamesToWarn($includeAliases = false) + { + $namesToWarn = $this->warnNames; + if ($includeAliases) { + $namesToWarn = array_merge($namesToWarn, $this->aliases); + } + + return $namesToWarn; + } +} diff --git a/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc b/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc new file mode 100644 index 00000000..aff15821 --- /dev/null +++ b/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc @@ -0,0 +1,54 @@ + 1, + 18 => 1, + 36 => 2, + 43 => 1 + ]; + } elseif ($testFile === 'ConstructorProxyInterceptorUnitTest.2.inc') { + return [ + 17 => 1, + 18 => 1, + 36 => 2, + 43 => 1 + ]; + } + return []; + } +} diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index 6c659ec2..885ec327 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -113,6 +113,10 @@ 8 warning + + 8 + warning + 8 warning From e0bfc48c919b0e64286decf3cfde28dc25dd9410 Mon Sep 17 00:00:00 2001 From: Robert Egginton Date: Fri, 29 Mar 2019 15:21:04 +0000 Subject: [PATCH 2/4] Only match constructor parameters with class names that match proxy or interceptor exactly --- ...f.php => DiscouragedDependenciesSniff.php} | 82 +++++++++++-------- .../ConstructorProxyInterceptorUnitTest.1.inc | 54 ------------ .../DiscouragedDependenciesUnitTest.1.inc | 55 +++++++++++++ ... => DiscouragedDependenciesUnitTest.2.inc} | 17 ++-- ...hp => DiscouragedDependenciesUnitTest.php} | 37 ++++----- Magento/ruleset.xml | 8 +- 6 files changed, 134 insertions(+), 119 deletions(-) rename Magento/Sniffs/Classes/{ConstructorProxyInterceptorSniff.php => DiscouragedDependenciesSniff.php} (78%) delete mode 100644 Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc create mode 100644 Magento/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc rename Magento/Tests/Classes/{ConstructorProxyInterceptorUnitTest.2.inc => DiscouragedDependenciesUnitTest.2.inc} (55%) rename Magento/Tests/Classes/{ConstructorProxyInterceptorUnitTest.php => DiscouragedDependenciesUnitTest.php} (50%) diff --git a/Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php b/Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php similarity index 78% rename from Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php rename to Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php index 37e62450..002ea770 100644 --- a/Magento/Sniffs/Classes/ConstructorProxyInterceptorSniff.php +++ b/Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php @@ -14,7 +14,7 @@ * * Search is in variable names and namespaces, including indirect namespaces from use statements */ -class ConstructorProxyInterceptorSniff implements Sniff +class DiscouragedDependenciesSniff implements Sniff { const CONSTRUCT_METHOD_NAME = '__construct'; @@ -33,17 +33,25 @@ class ConstructorProxyInterceptorSniff implements Sniff protected $warningCode = 'ConstructorProxyInterceptor'; /** - * @var array Aliases of proxies or plugins from use statements + * Aliases of proxies or plugins from use statements + * + * @var string[] */ private $aliases = []; /** - * @var array Terms to search for in variables and namespaces + * The current file - used for clearing USE aliases when file changes + * + * @var null|string */ - private $warnNames = [ - 'proxy', - 'plugin' - ]; + private $currentFile = null; + + /** + * Terms to search for in variables and namespaces + * + * @var string[] + */ + public $incorrectClassNames = ['proxy','interceptor']; /** * @inheritDoc @@ -61,8 +69,17 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - // Match use statements and constructor (latter matches T_FUNCTION to find constructors + // Clear down aliases when file under test changes + $currentFileName = $phpcsFile->getFilename(); + if ($this->currentFile != $currentFileName) { + // Clear aliases + $this->aliases = []; + $this->currentFile = $currentFileName; + } + + // Match use statements and constructor (latter matches T_FUNCTION to find constructors) $tokens = $phpcsFile->getTokens(); + if ($tokens[$stackPtr]['code'] == T_USE) { $this->processUse($phpcsFile, $stackPtr, $tokens); } elseif ($tokens[$stackPtr]['code'] == T_FUNCTION) { @@ -123,7 +140,7 @@ private function processFunction( return; } $positionInConstrSig = $openParenth; - $skipTillAfterVariable = false; + $lastName = null; do { // Find next part of namespace (string) or variable name $positionInConstrSig = $phpcsFile->findNext( @@ -131,23 +148,28 @@ private function processFunction( $positionInConstrSig + 1, $closeParenth ); + $currentTokenIsString = $tokens[$positionInConstrSig]['code'] == T_STRING; - // If we've already found a match, continue till after variable - if ($skipTillAfterVariable) { - if (!$currentTokenIsString) { - $skipTillAfterVariable = false; - } - continue; - } - // If match in namespace or variable then add warning - $name = strtolower($tokens[$positionInConstrSig]['content']); - $namesToWarn = $this->mergedNamesToWarn($currentTokenIsString); - if ($this->containsWord($namesToWarn, $name)) { - $phpcsFile->addWarning($this->warningMessage, $positionInConstrSig, $this->warningCode, [$name]); - if ($currentTokenIsString) { - $skipTillAfterVariable = true; + + if ($currentTokenIsString) { + // Remember string in case this is last before variable + $lastName = strtolower($tokens[$positionInConstrSig]['content']); + } else { + // If this is a variable, check last word for matches as was end of classname/alias + if ($lastName !== null) { + $namesToWarn = $this->mergedNamesToWarn(true); + if ($this->containsWord($namesToWarn, $lastName)) { + $phpcsFile->addError( + $this->warningMessage, + $positionInConstrSig, + $this->warningCode, + [$lastName] + ); + } + $lastName = null; } } + } while ($positionInConstrSig !== false && $positionInConstrSig < $closeParenth); } @@ -190,7 +212,7 @@ private function getConstructorPosition( } /** - * Whether $name is contained in any of $haystacks + * Whether $name exactly matches any of $haystacks * * @param array $haystacks * @param string $name @@ -199,15 +221,7 @@ private function getConstructorPosition( */ private function containsWord($haystacks, $name) { - $matchWarnWord = false; - foreach ($haystacks as $warn) { - if (strpos($name, $warn) !== false) { - $matchWarnWord = true; - break; - } - } - - return $matchWarnWord; + return in_array($name, $haystacks); } /** @@ -257,7 +271,7 @@ private function includesWarnWordsInSTRINGs( */ private function mergedNamesToWarn($includeAliases = false) { - $namesToWarn = $this->warnNames; + $namesToWarn = $this->incorrectClassNames; if ($includeAliases) { $namesToWarn = array_merge($namesToWarn, $this->aliases); } diff --git a/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc b/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc deleted file mode 100644 index aff15821..00000000 --- a/Magento/Tests/Classes/ConstructorProxyInterceptorUnitTest.1.inc +++ /dev/null @@ -1,54 +0,0 @@ - 1, - 18 => 1, - 36 => 2, - 43 => 1 + 37 => 1, + 44 => 1 ]; - } elseif ($testFile === 'ConstructorProxyInterceptorUnitTest.2.inc') { + } elseif ($testFile === 'DiscouragedDependenciesUnitTest.2.inc') { return [ 17 => 1, - 18 => 1, - 36 => 2, - 43 => 1 + 37 => 1, + 44 => 1 ]; } + + return []; + } + + /** + * @inheritdoc + */ + public function getWarningList() + { return []; } } diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index 885ec327..da5d1f6f 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -27,6 +27,10 @@ 10 error + + 10 + error + 10 error @@ -113,10 +117,6 @@ 8 warning - - 8 - warning - 8 warning From 33e18732fc2b48413f58d70076c7129090ddc56c Mon Sep 17 00:00:00 2001 From: Robert Egginton Date: Fri, 29 Mar 2019 16:33:08 +0000 Subject: [PATCH 3/4] Moving from Magento to Magento 2 name space --- .../Sniffs/Classes/DiscouragedDependenciesSniff.php | 2 +- .../Tests/Classes/DiscouragedDependenciesUnitTest.1.inc | 0 .../Tests/Classes/DiscouragedDependenciesUnitTest.2.inc | 0 .../Tests/Classes/DiscouragedDependenciesUnitTest.php | 4 ++-- Magento2/ruleset.xml | 2 +- 5 files changed, 4 insertions(+), 4 deletions(-) rename {Magento => Magento2}/Sniffs/Classes/DiscouragedDependenciesSniff.php (99%) rename {Magento => Magento2}/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc (100%) rename {Magento => Magento2}/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc (100%) rename {Magento => Magento2}/Tests/Classes/DiscouragedDependenciesUnitTest.php (91%) diff --git a/Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php b/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php similarity index 99% rename from Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php rename to Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php index 002ea770..afaa8058 100644 --- a/Magento/Sniffs/Classes/DiscouragedDependenciesSniff.php +++ b/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php @@ -4,7 +4,7 @@ * See COPYING.txt for license details. */ -namespace Magento\Sniffs\Classes; +namespace Magento2\Sniffs\Classes; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc similarity index 100% rename from Magento/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc rename to Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc similarity index 100% rename from Magento/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc rename to Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.php b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php similarity index 91% rename from Magento/Tests/Classes/DiscouragedDependenciesUnitTest.php rename to Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php index a7967025..3357cd89 100644 --- a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.php +++ b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php @@ -4,12 +4,12 @@ * See COPYING.txt for license details. */ -namespace Magento\Tests\Classes; +namespace Magento2\Tests\Classes; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; /** - * Class DiscouragedDependenciesInterceptorUnitTest + * Class DiscouragedDependenciesUnitTest * * Tests for interceptors in constructors */ diff --git a/Magento2/ruleset.xml b/Magento2/ruleset.xml index e0d5967d..bae89f95 100644 --- a/Magento2/ruleset.xml +++ b/Magento2/ruleset.xml @@ -28,7 +28,7 @@ 10 error - + 10 error From 1d4c5d15cee1a92dd0c3e69eaf7a251bbca41ea7 Mon Sep 17 00:00:00 2001 From: Robert Egginton Date: Mon, 8 Apr 2019 12:20:05 +0100 Subject: [PATCH 4/4] Addressing minor concerns from review --- Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php | 1 - Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc | 3 ++- Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php | 1 - Magento2/ruleset.xml | 1 + 4 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php b/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php index afaa8058..ed106d4b 100644 --- a/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php +++ b/Magento2/Sniffs/Classes/DiscouragedDependenciesSniff.php @@ -3,7 +3,6 @@ * Copyright © Magento. All rights reserved. * See COPYING.txt for license details. */ - namespace Magento2\Sniffs\Classes; use PHP_CodeSniffer\Files\File; diff --git a/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc index 2428d777..b5a04d16 100644 --- a/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc +++ b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc @@ -52,4 +52,5 @@ class Foo4 public function __construct(SafeAlias $other) { } -} \ No newline at end of file +} + diff --git a/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php index 3357cd89..db4332ef 100644 --- a/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php +++ b/Magento2/Tests/Classes/DiscouragedDependenciesUnitTest.php @@ -3,7 +3,6 @@ * Copyright © Magento. All rights reserved. * See COPYING.txt for license details. */ - namespace Magento2\Tests\Classes; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; diff --git a/Magento2/ruleset.xml b/Magento2/ruleset.xml index a8468a0e..2f9e5f4b 100644 --- a/Magento2/ruleset.xml +++ b/Magento2/ruleset.xml @@ -31,6 +31,7 @@ 10 error + */Test/* 10