Merge pull request #120 from magento/MC-18816

MC-18816: Impelement an better check for ThrowCatchSniff

Author: lenaorobei

Diff for: Magento2/Sniffs/Exceptions/ThrowCatchSniff.php

@@ -3,6 +3,7 @@
  * Copyright © Magento. All rights reserved.
  * See COPYING.txt for license details.
  */
+
 namespace Magento2\Sniffs\Exceptions;
 
 use function array_slice;
@@ -33,7 +34,7 @@ class ThrowCatchSniff implements Sniff
      */
     public function register()
     {
-        return [T_FUNCTION, T_CLOSURE];
+        return [T_TRY];
     }
 
     /**
@@ -42,53 +43,78 @@ public function register()
     public function process(File $phpcsFile, $stackPtr)
     {
         $tokens = $phpcsFile->getTokens();
-        if (!isset($tokens[$stackPtr]['scope_closer'])) {
-            // Probably an interface method no check
-            return;
-        }
+        $endOfStatement = $phpcsFile->findEndOfStatement($stackPtr);
 
-        $closeBrace = $tokens[$stackPtr]['scope_closer'];
-        $throwTags = [];
-        $catchTags = [];
+        $throwClassNames = [];
+        $searchForNextThrow = $stackPtr;
 
-        for ($i = $stackPtr; $i < $closeBrace; $i++) {
-            $token = $tokens[$i];
-            if ($token['code'] === T_CATCH) {
-                $catchTags[] = $token;
-            }
-            if ($token['code'] === T_THROW) {
-                $throwTags[] = $i;
+        //  search for all throws
+        do {
+            $throwTag = $phpcsFile->findNext(T_THROW, $searchForNextThrow, $endOfStatement);
+
+            if ($throwTag === false) {
+                break;
             }
-        }
 
-        if (count($catchTags) === 0 || count($throwTags) === 0) {
-            // No catch or throw found no check
-            return;
+            $throwClassNames[] = $this->getFullClassName($tokens, $throwTag + 1);
+
+            $searchForNextThrow = $throwTag + 1;
+        } while ($throwTag !== false);
+
+        if (empty($throwClassNames)) {
+            return; // is not relevant not throw in try found.
         }
 
         $catchClassNames = [];
-        $throwClassNames = [];
-
-        // find all relevant classes in catch
-        foreach ($catchTags as $catchTag) {
-            $start = $catchTag['parenthesis_opener'];
-            $end = $catchTag['parenthesis_closer'];
 
-            $match = $phpcsFile->findNext(T_STRING, $start, $end);
-            $catchClassNames[$match] = $tokens[$match]['content'];
-        }
+        // TRY statements need to check until the end of all CATCH statements.
+        do {
+            $nextToken = $phpcsFile->findNext(T_WHITESPACE, ($endOfStatement + 1), null, true);
+            if ($tokens[$nextToken]['code'] === T_CATCH) {
+                $endOfStatement = $tokens[$nextToken]['scope_closer'];
+                $catchClassNames[$nextToken] = $this->getFullClassName($tokens, $nextToken + 1);
+            } else {
+                break;
+            }
+        } while (isset($tokens[$nextToken]['scope_closer']) === true);
 
-        // find all relevant classes in throws
-        foreach ($throwTags as $throwTag) {
-            $match = $phpcsFile->findNext(T_STRING, $throwTag);
-            $throwClassNames[] = $tokens[$match]['content'];
+        if (empty($catchClassNames)) {
+            return; // is not relevant no catch found
         }
 
         $throwClassNames = array_flip($throwClassNames);
         foreach ($catchClassNames as $match => $catchClassName) {
-            if (array_key_exists($catchClassName, $throwClassNames)) {
+            if (isset($throwClassNames[$catchClassName])) {
                 $phpcsFile->addWarning($this->warningMessage, $match, $this->warningCode);
             }
         }
     }
+
+    /**
+     * Get the full class name with namespace.
+     *
+     * @param array $tokens
+     * @param int $startPos
+     * @return string
+     */
+    private function getFullClassName(array $tokens, $startPos)
+    {
+        $fullName = "";
+        $endOfClassName = [T_SEMICOLON => 0, T_CLOSE_PARENTHESIS => 0];
+
+        $tokenCount = count($tokens);
+        for ($i = $startPos; $i <= $tokenCount; $i++) {
+            $type = $tokens[$i]['code'];
+
+            if ($type === T_STRING || $type === T_NS_SEPARATOR) {
+                $fullName .= $tokens[$i]['content'];
+            }
+
+            if (array_key_exists($type, $endOfClassName)) {
+                break; // line end each
+            }
+        }
+
+        return $fullName;
+    }
 }

Diff for: Magento2/Tests/Exceptions/ThrowCatchUnitTest.1.inc

@@ -0,0 +1,18 @@
+<?php
+
+// This file contains all false positive cases that were found with previous implementation
+// Check  Exceptions MUST NOT handled in same function should fin nothing there
+
+/**
+ * @throws \Magento\Framework\Exception\LocalizedException
+ */
+function creatDir()
+{
+    try {
+        // call internal code that throw an Exception
+        throw new \Magento\Framework\Exception\LocalizedException('Nice user friendly message');
+    }
+    catch (\Magento\Framework\Exception\FileSystemException $e) {
+        throw new \Magento\Framework\Exception\LocalizedException('Nice user friendly message');
+    }
+}

Diff for: Magento2/Tests/Exceptions/ThrowCatchUnitTest.inc renamed to Magento2/Tests/Exceptions/ThrowCatchUnitTest.2.inc

@@ -3,6 +3,7 @@
  * Copyright © Magento. All rights reserved.
  * See COPYING.txt for license details.
  */
+
 namespace Magento2\Tests\Exceptions;
 
 use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
@@ -23,8 +24,13 @@ protected function getErrorList()
     /**
      * @inheritdoc
      */
-    protected function getWarningList()
+    protected function getWarningList($testFile = '')
     {
+
+        if ($testFile === 'ThrowCatchUnitTest.1.inc') {
+            return [];
+        }
+
         return [
             41 => 1,
             120 => 1,