Skip to content

Commit c31f099

Browse files
engcom-Charlieengcom-Charlie
committed
Merge remote-tracking branch '33918/add-phpmd-rule-unusedformalparameter-with-plugin-ignore' into comm_jul
2 parents d51ccda + dcfc489 commit c31f099

File tree

7 files changed

+316
-4
lines changed

7 files changed

+316
-4
lines changed

‎app/code/Magento/Catalog/Plugin/CategoryAuthorization.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public function __construct(Authorization $authorization)
3838
* @param CategoryInterface $category
3939
* @throws LocalizedException
4040
* @return array
41-
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
4241
*/
4342
public function beforeSave(CategoryRepositoryInterface $subject, CategoryInterface $category): array
4443
{

‎app/code/Magento/Catalog/Plugin/ProductAuthorization.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ public function __construct(Authorization $authorization)
3939
* @param bool $saveOptions
4040
* @throws LocalizedException
4141
* @return array
42-
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
4342
*/
4443
public function beforeSave(
4544
ProductRepositoryInterface $subject,

‎app/code/Magento/Catalog/Plugin/RemoveImagesFromGalleryAfterRemovingProduct.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ public function __construct(Gallery $galleryResource, ReadHandler $mediaGalleryR
4545
* @param callable $proceed
4646
* @param ProductInterface $product
4747
* @return bool
48-
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
4948
*/
5049
public function aroundDelete(
5150
ProductRepositoryInterface $subject,

‎dev/tests/static/framework/Magento/CodeMessDetector/Rule/UnusedCode/UnusedFormalParameter.php

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\CodeMessDetector\Rule\UnusedCode;
9+
10+
use PHPMD\AbstractNode;
11+
use PHPMD\Node\ClassNode;
12+
use PHPMD\Node\MethodNode;
13+
use PHPMD\Rule\UnusedFormalParameter as PhpmdUnusedFormalParameter;
14+
15+
class UnusedFormalParameter extends PhpmdUnusedFormalParameter
16+
{
17+
/**
18+
* This method collects all local variables in the body of the currently
19+
* analyzed method or function and removes those parameters that are
20+
* referenced by one of the collected variables.
21+
*
22+
* @param AbstractNode $node
23+
* @return void
24+
*/
25+
protected function removeUsedParameters(AbstractNode $node)
26+
{
27+
parent::removeUsedParameters($node);
28+
$this->removeVariablesUsedInPlugins($node);
29+
}
30+
31+
/**
32+
* Remove required method variables used in plugins from given node
33+
*
34+
* @param AbstractNode $node
35+
*/
36+
private function removeVariablesUsedInPlugins(AbstractNode $node)
37+
{
38+
if (!$node instanceof MethodNode) {
39+
return;
40+
}
41+
42+
/** @var ClassNode $classNode */
43+
$classNode = $node->getParentType();
44+
if (!$this->isPluginClass($classNode->getNamespaceName())) {
45+
return;
46+
}
47+
48+
/**
49+
* Around and After plugins has 2 required params $subject and $proceed or $result
50+
* that should be ignored
51+
*/
52+
foreach (['around', 'after'] as $pluginMethodPrefix) {
53+
if ($this->isFunctionNameStartingWith($node, $pluginMethodPrefix)) {
54+
$this->removeVariablesByCount(2);
55+
56+
break;
57+
}
58+
}
59+
60+
/**
61+
* Before plugins has 1 required params $subject
62+
* that should be ignored
63+
*/
64+
if ($this->isFunctionNameStartingWith($node, 'before')) {
65+
$this->removeVariablesByCount(1);
66+
}
67+
}
68+
69+
/**
70+
* Check if the first part of function fully qualified name is equal to $name
71+
*
72+
* Methods getImage and getName are equal. getImage used prior to usage in phpmd source
73+
*
74+
* @param MethodNode $node
75+
* @param string $name
76+
* @return boolean
77+
*/
78+
private function isFunctionNameStartingWith(MethodNode $node, string $name): bool
79+
{
80+
return (0 === strpos($node->getImage(), $name));
81+
}
82+
83+
/**
84+
* Remove first $countOfRemovingVariables from given node
85+
*
86+
* @param int $countOfRemovingVariables
87+
*/
88+
private function removeVariablesByCount(int $countOfRemovingVariables)
89+
{
90+
array_splice($this->nodes, 0, $countOfRemovingVariables);
91+
}
92+
93+
/**
94+
* Check if namespace contain "Plugin". Case-sensitive ignored
95+
*
96+
* @param string $class
97+
* @return bool
98+
*/
99+
private function isPluginClass(string $class): bool
100+
{
101+
return (stripos($class, 'plugin') !== false);
102+
}
103+
}

‎dev/tests/static/framework/Magento/CodeMessDetector/Test/Unit/Rule/UnusedCode/UnusedFormalParameterTest.php

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\CodeMessDetector\Test\Unit\Rule\UnusedCode;
9+
10+
use Magento\CodeMessDetector\Rule\UnusedCode\UnusedFormalParameter;
11+
use PHPMD\Node\ASTNode;
12+
use PHPMD\Node\MethodNode;
13+
use PHPMD\Report;
14+
use PHPUnit\Framework\MockObject\MockObject as MockObject;
15+
use PHPUnit\Framework\TestCase;
16+
17+
/**
18+
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
19+
*/
20+
class UnusedFormalParameterTest extends TestCase
21+
{
22+
private const FAKE_PLUGIN_NAMESPACE = 'Magento\CodeMessDetector\Test\UnusedCode\Plugin';
23+
private const FAKE_NAMESPACE = 'Magento\CodeMessDetector\Test\UnusedCode';
24+
25+
/**
26+
*
27+
* @dataProvider getCases
28+
*/
29+
public function testApply($methodName, $methodParams, $namespace, $expectViolation)
30+
{
31+
$node = $this->createMethodNodeMock($methodName, $methodParams, $namespace);
32+
$rule = new UnusedFormalParameter();
33+
$this->expectsRuleViolation($rule, $expectViolation);
34+
$rule->apply($node);
35+
}
36+
37+
/**
38+
* Prepare method node mock
39+
*
40+
* @param $methodName
41+
* @param $methodParams
42+
* @param $namespace
43+
* @return MethodNode|MockObject
44+
*/
45+
private function createMethodNodeMock($methodName, $methodParams, $namespace)
46+
{
47+
$methodNode = $this->createConfiguredMock(
48+
MethodNode::class,
49+
[
50+
'getName' => $methodName,
51+
'getImage' => $methodName,
52+
'isAbstract' => false,
53+
'isDeclaration' => true
54+
]
55+
);
56+
57+
$variableDeclarators = [];
58+
foreach ($methodParams as $methodParam) {
59+
$variableDeclarator = $this->createASTNodeMock();
60+
$variableDeclarator->method('getImage')
61+
->willReturn($methodParam);
62+
63+
$variableDeclarators[] = $variableDeclarator;
64+
}
65+
$parametersMock = $this->createASTNodeMock();
66+
$parametersMock->expects($this->once())
67+
->method('findChildrenOfType')
68+
->with('VariableDeclarator')
69+
->willReturn($variableDeclarators);
70+
71+
/**
72+
* Declare mock result for findChildrenOfType
73+
* with Dummy for removeCompoundVariables and removeVariablesUsedByFuncGetArgs
74+
*/
75+
$methodNode->expects($this->atLeastOnce())
76+
->method('findChildrenOfType')
77+
->withConsecutive(['FormalParameters'], ['CompoundVariable'], ['FunctionPostfix'])
78+
->willReturnOnConsecutiveCalls([$parametersMock], [], []);
79+
80+
// Dummy result for removeRegularVariables
81+
$methodNode->expects($this->once())
82+
->method('findChildrenOfTypeVariable')
83+
->willReturn([]);
84+
85+
$classNode = $this->createASTNodeMock();
86+
$classNode->expects($this->once())
87+
->method('getNamespaceName')
88+
->willReturn($namespace);
89+
$methodNode->expects($this->once())
90+
->method('getParentType')
91+
->willReturn($classNode);
92+
93+
return $methodNode;
94+
}
95+
96+
/**
97+
* Create ASTNode mock
98+
*
99+
* @return ASTNode|MockObject
100+
*/
101+
private function createASTNodeMock()
102+
{
103+
return $this->createMock(ASTNode::class);
104+
}
105+
106+
/**
107+
* @param UnusedFormalParameter $rule
108+
* @param bool $expects
109+
*/
110+
private function expectsRuleViolation(UnusedFormalParameter $rule, bool $expects)
111+
{
112+
/** @var Report|MockObject $reportMock */
113+
$reportMock = $this->createMock(Report::class);
114+
if ($expects) {
115+
$violationExpectation = $this->atLeastOnce();
116+
} else {
117+
$violationExpectation = $this->never();
118+
}
119+
$reportMock->expects($violationExpectation)
120+
->method('addRuleViolation');
121+
$rule->setReport($reportMock);
122+
}
123+
124+
/**
125+
* @return array
126+
*/
127+
public function getCases(): array
128+
{
129+
return [
130+
// Plugin methods
131+
[
132+
'beforePluginMethod',
133+
[
134+
'subject'
135+
],
136+
self::FAKE_PLUGIN_NAMESPACE,
137+
false
138+
],
139+
[
140+
'aroundPluginMethod',
141+
[
142+
'subject',
143+
'proceed'
144+
],
145+
self::FAKE_PLUGIN_NAMESPACE,
146+
false
147+
],
148+
[
149+
'aroundPluginMethod',
150+
[
151+
'subject',
152+
'result'
153+
],
154+
self::FAKE_PLUGIN_NAMESPACE,
155+
false
156+
],
157+
// Plugin method that contain unused parameter
158+
[
159+
'someMethod',
160+
[
161+
'unusedParameter'
162+
],
163+
self::FAKE_PLUGIN_NAMESPACE,
164+
true
165+
],
166+
// Non plugin method
167+
[
168+
'someMethod',
169+
[
170+
'subject',
171+
'result'
172+
],
173+
self::FAKE_NAMESPACE,
174+
true
175+
]
176+
];
177+
}
178+
}

‎dev/tests/static/framework/Magento/CodeMessDetector/resources/rulesets/unusedcode.xml

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?xml version="1.0"?>
2+
<!--
3+
/**
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
-->
8+
<ruleset name="Unused Code Rules"
9+
xmlns="http://pmd.sf.net/ruleset/1.0.0"
10+
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
11+
xsi:schemaLocation="http://pmd.sf.net/ruleset/1.0.0 http://pmd.sf.net/ruleset_xml_schema.xsd"
12+
xsi:noNamespaceSchemaLocation="http://pmd.sf.net/ruleset_xml_schema.xsd">
13+
14+
<rule name="UnusedFormalParameter"
15+
message="Avoid unused parameters such as '{0}'."
16+
class="Magento\CodeMessDetector\Rule\UnusedCode\UnusedFormalParameter">
17+
<description><![CDATA[Avoid passing parameters to methods or constructors and then not using those parameters except on plugins]]></description>
18+
<example>
19+
<![CDATA[
20+
class Foo
21+
{
22+
private function bar($howdy)
23+
{
24+
// $howdy is not used
25+
}
26+
}
27+
]]>
28+
</example>
29+
</rule>
30+
31+
</ruleset>

‎dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
</rule>
2727

2828
<!-- Unused code rules -->
29-
<rule ref="rulesets/unusedcode.xml" />
29+
<rule ref="rulesets/unusedcode.xml">
30+
<exclude name="UnusedFormalParameter"/>
31+
</rule>
3032

3133
<!-- Code design rules -->
3234
<rule ref="rulesets/design.xml/NumberOfChildren" />
@@ -45,5 +47,6 @@
4547
<!-- Magento Specific Rules -->
4648
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/AllPurposeAction" />
4749
<rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/CookieAndSessionMisuse" />
50+
<rule ref="Magento/CodeMessDetector/resources/rulesets/unusedcode.xml/UnusedFormalParameter" />
4851

4952
</ruleset>

0 commit comments

Comments
 (0)