Skip to content

Commit 6f2428b

Browse files
lenaorobeilenaorobei
authored
Merge pull request #43 from magento/29-ThrowCatch
#29: [New Rule] Exceptions must not be handled in the same function
2 parents df5fc07 + a4bf3e2 commit 6f2428b

File tree

4 files changed

+296
-0
lines changed

4 files changed

+296
-0
lines changed

‎Magento/Sniffs/Exceptions/ThrowCatchSniff.php

+95
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
<?php
2+
/**
3+
* Copyright © Magento. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
7+
namespace Magento\Sniffs\Exceptions;
8+
9+
use function array_slice;
10+
use PHP_CodeSniffer\Sniffs\Sniff;
11+
use PHP_CodeSniffer\Files\File;
12+
13+
/**
14+
* Detects exceptions must not be handled in same function
15+
*/
16+
class ThrowCatchSniff implements Sniff
17+
{
18+
/**
19+
* String representation of warning.
20+
*
21+
* @var string
22+
*/
23+
protected $warningMessage = 'Exceptions must not be handled in the same function where they are thrown.';
24+
25+
/**
26+
* Warning violation code.
27+
*
28+
* @var string
29+
*/
30+
protected $warningCode = 'ThrowCatch';
31+
32+
/**
33+
* @inheritDoc
34+
*/
35+
public function register()
36+
{
37+
return [T_FUNCTION, T_CLOSURE];
38+
}
39+
40+
/**
41+
* @inheritDoc
42+
*/
43+
public function process(File $phpcsFile, $stackPtr)
44+
{
45+
$tokens = $phpcsFile->getTokens();
46+
if (!isset($tokens[$stackPtr]['scope_closer'])) {
47+
// Probably an interface method no check
48+
return;
49+
}
50+
51+
$closeBrace = $tokens[$stackPtr]['scope_closer'];
52+
$throwTags = [];
53+
$catchTags = [];
54+
55+
for ($i = $stackPtr; $i < $closeBrace; $i++) {
56+
$token = $tokens[$i];
57+
if ($token['code'] === T_CATCH) {
58+
$catchTags[] = $token;
59+
}
60+
if ($token['code'] === T_THROW) {
61+
$throwTags[] = $i;
62+
}
63+
}
64+
65+
if (count($catchTags) === 0 || count($throwTags) === 0) {
66+
// No catch or throw found no check
67+
return;
68+
}
69+
70+
$catchClassNames = [];
71+
$throwClassNames = [];
72+
73+
// find all relevant classes in catch
74+
foreach ($catchTags as $catchTag) {
75+
$start = $catchTag['parenthesis_opener'];
76+
$end = $catchTag['parenthesis_closer'];
77+
78+
$match = $phpcsFile->findNext(T_STRING, $start, $end);
79+
$catchClassNames[$match] = $tokens[$match]['content'];
80+
}
81+
82+
// find all relevant classes in throws
83+
foreach ($throwTags as $throwTag) {
84+
$match = $phpcsFile->findNext(T_STRING, $throwTag);
85+
$throwClassNames[] = $tokens[$match]['content'];
86+
}
87+
88+
$throwClassNames = array_flip($throwClassNames);
89+
foreach ($catchClassNames as $match => $catchClassName) {
90+
if (array_key_exists($catchClassName, $throwClassNames)) {
91+
$phpcsFile->addWarning($this->warningMessage, $match, $this->warningCode);
92+
}
93+
}
94+
}
95+
}

‎Magento/Tests/Exceptions/ThrowCatchUnitTest.inc

+160
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
<?php
2+
3+
namespace Foo\Bar;
4+
5+
use ArithmeticError;
6+
use BadFunctionCallException;
7+
use Exception;
8+
use LogicException;
9+
10+
abstract class Calculator
11+
{
12+
abstract public function complexNotRelevantToCheck(array $data);
13+
}
14+
15+
class CustomFooException extends Exception {}
16+
17+
class Foo extends Calculator
18+
{
19+
20+
/**
21+
* @var CallulationService
22+
*/
23+
private $calculationService;
24+
25+
public function __construct(CallulationService $calculationService)
26+
{
27+
$this->calculationService = $calculationService;
28+
}
29+
30+
/**
31+
* @param array $data
32+
* @return array|mixed
33+
*/
34+
private function vaildateFunction(array $data)
35+
{
36+
try {
37+
if (!isset($data['formData'])) {
38+
throw new CustomFooException('Bar is not set.');
39+
}
40+
return $data['formData'];
41+
} catch (CustomFooException $exception) {
42+
// Rule find: Exceptions MUST NOT handled in same function
43+
// handle $exception code
44+
}
45+
46+
return [];
47+
}
48+
49+
/**
50+
* @param array $data
51+
* @return int
52+
*/
53+
public function complexNotRelevantToCheck(array $data)
54+
{
55+
$result = $this->vaildateFunction($data);
56+
$previous = 1;
57+
58+
foreach ($result as $number) {
59+
if ($number === 0) {
60+
throw new LogicException('Cant not deviated by null');
61+
}
62+
63+
$previous /= $number;
64+
65+
// more complex code
66+
67+
if ($previous === 0) {
68+
throw new ArithmeticError('Calculation error');
69+
}
70+
71+
// more complex code
72+
73+
$result = $this->calculationService->call($previous);
74+
75+
if ($result === false) {
76+
throw new BadFunctionCallException('Cant not reach calculationService');
77+
}
78+
}
79+
80+
return (int)$previous;
81+
}
82+
83+
/**
84+
* @param array $data
85+
* @return int
86+
* @throws BadFunctionCallException
87+
*/
88+
public function complexDivisionFunction(array $data)
89+
{
90+
try {
91+
try {
92+
93+
$result = $this->vaildateFunction($data);
94+
$previous = 1;
95+
96+
foreach ($result as $number) {
97+
if ($number === 0) {
98+
throw new LogicException('Cant not deviated by null');
99+
}
100+
101+
$previous /= $number;
102+
103+
// more complex code
104+
105+
if ($previous === 0) {
106+
throw new ArithmeticError('Calculation error');
107+
}
108+
109+
// more complex code
110+
111+
$result = $this->calculationService->call($previous);
112+
113+
if ($result === false) {
114+
throw new BadFunctionCallException('Cant not reach calculationService');
115+
}
116+
}
117+
118+
return (int)$previous;
119+
120+
} catch (LogicException $logicException) {
121+
// Rule find: Exceptions MUST NOT handled in same function
122+
// handle $exception code
123+
} catch (CustomFooException $exception) {
124+
// handle $exception code
125+
}
126+
} catch (ArithmeticError $arithmeticError) {
127+
// Rule find: Exceptions MUST NOT handled in same function
128+
// handle $exception code
129+
}
130+
131+
return 0;
132+
}
133+
}
134+
135+
136+
// bad logic function in .phtml
137+
138+
function formatFunction(array $data)
139+
{
140+
try {
141+
if (!isset($data['formData'])) {
142+
throw new CustomFooException('Bar is not set.');
143+
}
144+
return $data['formData'];
145+
} catch (CustomFooException $exception) {
146+
// Rule find: Exceptions MUST NOT handled in same function
147+
// handle $exception code
148+
}
149+
150+
return [];
151+
}
152+
153+
$d = function ($data) {
154+
try {
155+
throw new CustomFooException('Bar is not set.');
156+
} catch (CustomFooException $exception) {
157+
// Rule find: Exceptions MUST NOT handled in same function
158+
// handle $exception code
159+
}
160+
};

‎Magento/Tests/Exceptions/ThrowCatchUnitTest.php

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
/**
3+
* Copyright © Magento. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
7+
namespace Magento\Tests\Exceptions;
8+
9+
use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
10+
11+
/**
12+
* Class ThrowCatchUnitTest
13+
*/
14+
class ThrowCatchUnitTest extends AbstractSniffUnitTest
15+
{
16+
/**
17+
* @inheritdoc
18+
*/
19+
protected function getErrorList()
20+
{
21+
return [];
22+
}
23+
24+
/**
25+
* @inheritdoc
26+
*/
27+
protected function getWarningList()
28+
{
29+
return [
30+
41 => 1,
31+
120 => 1,
32+
126 => 1,
33+
145 => 1,
34+
156 => 1
35+
];
36+
}
37+
}

‎Magento/ruleset.xml

+4
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@
142142
<severity>8</severity>
143143
<type>warning</type>
144144
</rule>
145+
<rule ref="Magento.Exceptions.ThrowCatch">
146+
<severity>8</severity>
147+
<type>warning</type>
148+
</rule>
145149

146150
<!-- Severity 7 warnings: General code issues. -->
147151
<rule ref="Generic.Arrays.DisallowLongArraySyntax">

0 commit comments

Comments
 (0)