From 56ef04a4afffaff60fbd6420825e18e6f3beab5d Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Thu, 28 Feb 2019 00:03:29 +0100 Subject: [PATCH 1/3] #29: [New Rule] Exceptions must not be handled in the same function where they are thrown --- Magento/Sniffs/Functions/ThrowCatchSniff.php | 95 +++++++++++ .../Tests/Functions/ThrowCatchUnitTest.inc | 160 ++++++++++++++++++ .../Tests/Functions/ThrowCatchUnitTest.php | 37 ++++ Magento/ruleset.xml | 3 + 4 files changed, 295 insertions(+) create mode 100644 Magento/Sniffs/Functions/ThrowCatchSniff.php create mode 100644 Magento/Tests/Functions/ThrowCatchUnitTest.inc create mode 100644 Magento/Tests/Functions/ThrowCatchUnitTest.php diff --git a/Magento/Sniffs/Functions/ThrowCatchSniff.php b/Magento/Sniffs/Functions/ThrowCatchSniff.php new file mode 100644 index 00000000..7057e86d --- /dev/null +++ b/Magento/Sniffs/Functions/ThrowCatchSniff.php @@ -0,0 +1,95 @@ +getTokens(); + if (!isset($tokens[$stackPtr]['scope_closer'])) { + // Probably an interface method no check + return; + } + + $closeBrace = $tokens[$stackPtr]['scope_closer']; + $throwTags = []; + $catchTags = []; + + for ($i = $stackPtr; $i < $closeBrace; $i++) { + $token = $tokens[$i]; + if ($token['code'] === T_CATCH) { + $catchTags[] = $token; + } + if ($token['code'] === T_THROW) { + $throwTags[] = $i; + } + } + + if (count($catchTags) === 0 || count($throwTags) === 0) { + // No catch or throw found no check + return; + } + + $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']; + } + + // find all relevant classes in throws + foreach ($throwTags as $throwTag) { + $match = $phpcsFile->findNext(T_STRING, $throwTag); + $throwClassNames[] = $tokens[$match]['content']; + } + + $throwClassNames = array_flip($throwClassNames); + foreach ($catchClassNames as $match => $catchClassName) { + if (array_key_exists($catchClassName, $throwClassNames)) { + $phpcsFile->addWarning($this->warningMessage, $match, $this->warningCode); + } + } + } +} \ No newline at end of file diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.inc b/Magento/Tests/Functions/ThrowCatchUnitTest.inc new file mode 100644 index 00000000..c1c28b1f --- /dev/null +++ b/Magento/Tests/Functions/ThrowCatchUnitTest.inc @@ -0,0 +1,160 @@ +calculationService = $calculationService; + } + + /** + * @param array $data + * @return array|mixed + */ + private function vaildateFunction(array $data) + { + try { + if (!isset($data['formData'])) { + throw new CustomFooException('Bar is not set.'); + } + return $data['formData']; + } catch (CustomFooException $exception) { + // Rule find: Exceptions MUST NOT handled in same function + // handle $exception code + } + + return []; + } + + /** + * @param array $data + * @return int + */ + public function complexNotRelevantToCheck(array $data) + { + $result = $this->vaildateFunction($data); + $previous = 1; + + foreach ($result as $number) { + if ($number === 0) { + throw new LogicException('Cant not deviated by null'); + } + + $previous /= $number; + + // more complex code + + if ($previous === 0) { + throw new ArithmeticError('Calculation error'); + } + + // more complex code + + $result = $this->calculationService->call($previous); + + if ($result === false) { + throw new BadFunctionCallException('Cant not reach calculationService'); + } + } + + return (int)$previous; + } + + /** + * @param array $data + * @return int + * @throws BadFunctionCallException + */ + public function complexDivisionFunction(array $data) + { + try { + try { + + $result = $this->vaildateFunction($data); + $previous = 1; + + foreach ($result as $number) { + if ($number === 0) { + throw new LogicException('Cant not deviated by null'); + } + + $previous /= $number; + + // more complex code + + if ($previous === 0) { + throw new ArithmeticError('Calculation error'); + } + + // more complex code + + $result = $this->calculationService->call($previous); + + if ($result === false) { + throw new BadFunctionCallException('Cant not reach calculationService'); + } + } + + return (int)$previous; + + } catch (LogicException $logicException) { + // Rule find: Exceptions MUST NOT handled in same function + // handle $exception code + } catch (CustomFooException $exception) { + // handle $exception code + } + } catch (ArithmeticError $arithmeticError) { + // Rule find: Exceptions MUST NOT handled in same function + // handle $exception code + } + + return 0; + } +} + + +// bad logic function in .phtml + +function formatFunction(array $data) +{ + try { + if (!isset($data['formData'])) { + throw new CustomFooException('Bar is not set.'); + } + return $data['formData']; + } catch (CustomFooException $exception) { + // Rule find: Exceptions MUST NOT handled in same function + // handle $exception code + } + + return []; +} + +$d = function ($data) { + try { + throw new CustomFooException('Bar is not set.'); + } catch (CustomFooException $exception) { + // Rule find: Exceptions MUST NOT handled in same function + // handle $exception code + } +}; \ No newline at end of file diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.php b/Magento/Tests/Functions/ThrowCatchUnitTest.php new file mode 100644 index 00000000..84dfe71e --- /dev/null +++ b/Magento/Tests/Functions/ThrowCatchUnitTest.php @@ -0,0 +1,37 @@ + 1, + 120 => 1, + 126 => 1, + 145 => 1, + 156 => 1 + ]; + } +} \ No newline at end of file diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index ce101e33..a4a70458 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -72,6 +72,9 @@ 8 + + 8 + From 4da03239735efe8d366b8abf52021ccf71654a0b Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Thu, 28 Feb 2019 11:25:22 +0100 Subject: [PATCH 2/3] 29: [New Rule] Exceptions handling --- Magento/Sniffs/Functions/ThrowCatchSniff.php | 2 +- Magento/Tests/Functions/ThrowCatchUnitTest.inc | 2 +- Magento/Tests/Functions/ThrowCatchUnitTest.php | 2 +- Magento/ruleset.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Magento/Sniffs/Functions/ThrowCatchSniff.php b/Magento/Sniffs/Functions/ThrowCatchSniff.php index 7057e86d..69b841db 100644 --- a/Magento/Sniffs/Functions/ThrowCatchSniff.php +++ b/Magento/Sniffs/Functions/ThrowCatchSniff.php @@ -92,4 +92,4 @@ public function process(File $phpcsFile, $stackPtr) } } } -} \ No newline at end of file +} diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.inc b/Magento/Tests/Functions/ThrowCatchUnitTest.inc index c1c28b1f..2c6e81d0 100644 --- a/Magento/Tests/Functions/ThrowCatchUnitTest.inc +++ b/Magento/Tests/Functions/ThrowCatchUnitTest.inc @@ -157,4 +157,4 @@ $d = function ($data) { // Rule find: Exceptions MUST NOT handled in same function // handle $exception code } -}; \ No newline at end of file +}; diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.php b/Magento/Tests/Functions/ThrowCatchUnitTest.php index 84dfe71e..30c7dc81 100644 --- a/Magento/Tests/Functions/ThrowCatchUnitTest.php +++ b/Magento/Tests/Functions/ThrowCatchUnitTest.php @@ -34,4 +34,4 @@ protected function getWarningList() 156 => 1 ]; } -} \ No newline at end of file +} diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index ad4a6f9b..383f748e 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -72,7 +72,7 @@ 8 - + 8 From a4bf3e294fd09c0ae53ada00571b85d8d90719bc Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Sun, 3 Mar 2019 13:59:26 +0100 Subject: [PATCH 3/3] Review fixes --- Magento/Sniffs/{Functions => Exceptions}/ThrowCatchSniff.php | 2 +- .../Tests/{Functions => Exceptions}/ThrowCatchUnitTest.inc | 0 .../Tests/{Functions => Exceptions}/ThrowCatchUnitTest.php | 2 +- Magento/ruleset.xml | 4 ++++ 4 files changed, 6 insertions(+), 2 deletions(-) rename Magento/Sniffs/{Functions => Exceptions}/ThrowCatchSniff.php (98%) rename Magento/Tests/{Functions => Exceptions}/ThrowCatchUnitTest.inc (100%) rename Magento/Tests/{Functions => Exceptions}/ThrowCatchUnitTest.php (94%) diff --git a/Magento/Sniffs/Functions/ThrowCatchSniff.php b/Magento/Sniffs/Exceptions/ThrowCatchSniff.php similarity index 98% rename from Magento/Sniffs/Functions/ThrowCatchSniff.php rename to Magento/Sniffs/Exceptions/ThrowCatchSniff.php index 69b841db..b9ebf0d4 100644 --- a/Magento/Sniffs/Functions/ThrowCatchSniff.php +++ b/Magento/Sniffs/Exceptions/ThrowCatchSniff.php @@ -4,7 +4,7 @@ * See COPYING.txt for license details. */ -namespace Magento\Sniffs\Functions; +namespace Magento\Sniffs\Exceptions; use function array_slice; use PHP_CodeSniffer\Sniffs\Sniff; diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.inc b/Magento/Tests/Exceptions/ThrowCatchUnitTest.inc similarity index 100% rename from Magento/Tests/Functions/ThrowCatchUnitTest.inc rename to Magento/Tests/Exceptions/ThrowCatchUnitTest.inc diff --git a/Magento/Tests/Functions/ThrowCatchUnitTest.php b/Magento/Tests/Exceptions/ThrowCatchUnitTest.php similarity index 94% rename from Magento/Tests/Functions/ThrowCatchUnitTest.php rename to Magento/Tests/Exceptions/ThrowCatchUnitTest.php index 30c7dc81..07ff18ea 100644 --- a/Magento/Tests/Functions/ThrowCatchUnitTest.php +++ b/Magento/Tests/Exceptions/ThrowCatchUnitTest.php @@ -4,7 +4,7 @@ * See COPYING.txt for license details. */ -namespace Magento\Tests\Functions; +namespace Magento\Tests\Exceptions; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index 40336962..2ce4fff1 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -142,6 +142,10 @@ 8 warning + + 8 + warning +