Only match constructor parameters with class names that match proxy or interceptor exactly

Robert Egginton · Robert Egginton · commit e0bfc48c919b · 2019-03-29T15:21:04.000Z
diff --git a/Magento/Sniffs/Classes/DiscouragedDependenciesSniff.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,31 +140,36 @@ private function processFunction(
             return;
         }
         $positionInConstrSig = $openParenth;
-        $skipTillAfterVariable = false;
+        $lastName = null;
         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;
+
+            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
diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc b/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.1.inc
@@ -0,0 +1,55 @@
+<?php
+
+namespace Vendor\Module\Anyname;
+
+use \Vendor\Module\AnyName\Interceptor as InterAlias;
+use \Exception as SafeAlias;
+
+class Interceptor {}
+
+/**
+ * // Rule find: constructor use of interceptor class
+ */
+class Foo
+{
+    public function __construct(
+        $first,
+        \Vendor\Module\AnyName\Interceptor $anything,
+        $interceptorOnlyInName,
+        InterAliasMorethan $fine, // should not match
+        $another = []
+    )
+    {
+
+    }
+    public function notAConstruct(
+        \Vendor\Module\Anyname\Interceptor $anything
+    )
+    {
+
+    }
+}
+
+/**
+ * // Rule find: constructor use of interceptor class with alias
+ */
+class Foo2 {
+    public function __construct(InterAlias $anything, $aInterceptorInName) {}
+}
+
+/**
+ * // Rule find: constructor use of interceptor class with use statement
+ */
+class Foo3 {
+    public function __construct(Interceptor $anything) {}
+}
+
+/**
+ * // Rule find: This is fine
+ */
+class Foo4
+{
+    public function __construct(SafeAlias $other) {
+
+    }
+}
diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc b/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.2.inc
@@ -1,11 +1,11 @@
 <?php
 
-namespace Vendor\Module\Proxy;
+namespace Vendor\Module\Anyname;
 
-use \Vendor\Module\Proxy\AnyName as ProxAlias;
+use \Vendor\Module\AnyName\Proxy as ProxAlias;
 use \Exception as SafeAlias;
 
-class AnyName {}
+class Proxy {}
 
 /**
  * // Rule find: constructor use of proxy class
@@ -14,15 +14,16 @@ class Foo
 {
     public function __construct(
             $first,
-            \Vendor\Module\Proxy\AnyName $anything,
-            $proxy,
+            \Vendor\Module\AnyName\Proxy $anything,
+            $proxyOnlyInName,
+            ProxAliasNope $notAProblem, // should not match
             $another = []
     )
     {
 
     }
     public function notAConstruct(
-        \Vendor\Module\Proxy\AnyName $anything
+        \Vendor\Module\Anyname\Proxy $anything
     )
     {
 
@@ -33,14 +34,14 @@ class Foo
  * // Rule find: constructor use of proxy class with alias
  */
 class Foo2 {
-    public function __construct(ProxAlias $anything, $aProxy) {}
+    public function __construct(ProxAlias $anything, $aProxyInName) {}
 }
 
 /**
  * // Rule find: constructor use of proxy class with use statement
  */
 class Foo3 {
-    public function __construct(AnyName $anything) {}
+    public function __construct(Proxy $anything, InterAlias $oldAliasInName) {}
 }
 
 /**
diff --git a/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.php b/Magento/Tests/Classes/DiscouragedDependenciesUnitTest.php
@@ -9,40 +9,39 @@
 use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
 
 /**
- * Class ConstructorProxyInterceptorUnitTest
+ * Class DiscouragedDependenciesInterceptorUnitTest
  *
  * Tests for interceptors in constructors
  */
-class ConstructorProxyInterceptorUnitTest extends AbstractSniffUnitTest
+class DiscouragedDependenciesUnitTest extends AbstractSniffUnitTest
 {
     /**
      * @inheritdoc
      */
-    public function getErrorList()
+    public function getErrorList($testFile = '')
     {
-        return [];
-    }
-
-    /**
-     * @inheritdoc
-     */
-    public function getWarningList($testFile = '')
-    {
-        if ($testFile === 'ConstructorProxyInterceptorUnitTest.1.inc') {
+        if ($testFile === 'DiscouragedDependenciesUnitTest.1.inc') {
             return [
                 17 => 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
@@ -27,6 +27,10 @@
         <severity>10</severity>
         <type>error</type>
     </rule>
+    <rule ref="Magento.Classes.DiscouragedDependencies">
+        <severity>10</severity>
+        <type>error</type>
+    </rule>
     <rule ref="Magento.Legacy.MageEntity">
         <severity>10</severity>
         <type>error</type>
@@ -113,10 +117,6 @@
         <severity>8</severity>
         <type>warning</type>
     </rule>
-    <rule ref="Magento.Classes.ConstructorProxyInterceptor">
-        <severity>8</severity>
-        <type>warning</type>
-    </rule>
     <rule ref="Magento.Classes.ObjectInstantiation">
         <severity>8</severity>
         <type>warning</type>

Original file line number	Diff line number	Diff line change
`@@ -1,11 +1,11 @@`
`1`	`1`	`<?php`
`2`	`2`
`3`		`-namespace Vendor\Module\Proxy;`
	`3`	`+namespace Vendor\Module\Anyname;`
`4`	`4`
`5`		`-use \Vendor\Module\Proxy\AnyName as ProxAlias;`
	`5`	`+use \Vendor\Module\AnyName\Proxy as ProxAlias;`
`6`	`6`	`use \Exception as SafeAlias;`
`7`	`7`
`8`		`-class AnyName {}`
	`8`	`+class Proxy {}`
`9`	`9`
`10`	`10`	`/**`
`11`	`11`	`* // Rule find: constructor use of proxy class`
`@@ -14,15 +14,16 @@ class Foo`
`14`	`14`	`{`
`15`	`15`	`public function __construct(`
`16`	`16`	`$first,`
`17`		`- \Vendor\Module\Proxy\AnyName $anything,`
`18`		`- $proxy,`
	`17`	`+ \Vendor\Module\AnyName\Proxy $anything,`
	`18`	`+ $proxyOnlyInName,`
	`19`	`+ ProxAliasNope $notAProblem, // should not match`
`19`	`20`	`$another = []`
`20`	`21`	`)`
`21`	`22`	`{`
`22`	`23`
`23`	`24`	`}`
`24`	`25`	`public function notAConstruct(`
`25`		`- \Vendor\Module\Proxy\AnyName $anything`
	`26`	`+ \Vendor\Module\Anyname\Proxy $anything`
`26`	`27`	`)`
`27`	`28`	`{`
`28`	`29`
`@@ -33,14 +34,14 @@ class Foo`
`33`	`34`	`* // Rule find: constructor use of proxy class with alias`
`34`	`35`	`*/`
`35`	`36`	`class Foo2 {`
`36`		`- public function __construct(ProxAlias $anything, $aProxy) {}`
	`37`	`+ public function __construct(ProxAlias $anything, $aProxyInName) {}`
`37`	`38`	`}`
`38`	`39`
`39`	`40`	`/**`
`40`	`41`	`* // Rule find: constructor use of proxy class with use statement`
`41`	`42`	`*/`
`42`	`43`	`class Foo3 {`
`43`		`- public function __construct(AnyName $anything) {}`
	`44`	`+ public function __construct(Proxy $anything, InterAlias $oldAliasInName) {}`
`44`	`45`	`}`
`45`	`46`
`46`	`47`	`/**`