Skip to content

Commit c9485d2

Browse files
zakdmazakdma
authored
Merge pull request #25 from magento-tsg/MC-36160
[Arrows] MC-36160: SVC false-positive: used a protected property from parents class
2 parents 3372735 + 4b9f74c commit c9485d2

File tree

23 files changed

+459
-6
lines changed

23 files changed

+459
-6
lines changed

‎src/Analyzer/ClassAnalyzer.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
130130
{
131131
return [
132132
new ClassMethodAnalyzer($context, $fileBefore, $fileAfter, $this->dependencyGraph),
133-
new PropertyAnalyzer($context, $fileBefore, $fileAfter),
133+
new PropertyAnalyzer($context, $fileBefore, $fileAfter, $this->dependencyGraph),
134134
new ClassConstantAnalyzer($context, $fileBefore, $fileAfter),
135135
new ClassMethodExceptionAnalyzer($context, $fileBefore, $fileAfter),
136136
new ClassImplementsAnalyzer($context, $fileBefore, $fileAfter),

‎src/Analyzer/PropertyAnalyzer.php‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99

1010
namespace Magento\SemanticVersionChecker\Analyzer;
1111

12+
use Magento\SemanticVersionChecker\ClassHierarchy\Entity;
1213
use Magento\SemanticVersionChecker\Comparator\Visibility;
1314
use Magento\SemanticVersionChecker\Operation\PropertyMoved;
15+
use Magento\SemanticVersionChecker\Operation\PropertyOverwriteAdded;
1416
use Magento\SemanticVersionChecker\Operation\Visibility\PropertyDecreased as VisibilityPropertyDecreased;
1517
use Magento\SemanticVersionChecker\Operation\Visibility\PropertyIncreased as VisibilityPropertyIncreased;
1618
use PhpParser\Node;
@@ -62,9 +64,53 @@ protected function getNodeClass()
6264
*/
6365
protected function reportAddedNode($report, $fileAfter, $classAfter, $propertyAfter)
6466
{
67+
if ($this->dependencyGraph !== null) {
68+
$class = $this->dependencyGraph->findEntityByName((string)$classAfter->namespacedName);
69+
if ($class !== null) {
70+
$propertyOverwritten = $this->searchPropertyExistsRecursive(
71+
$class,
72+
$propertyAfter->props[0]->name->toString()
73+
);
74+
if ($propertyOverwritten) {
75+
$report->add(
76+
$this->context,
77+
new PropertyOverwriteAdded($this->context, $fileAfter, $classAfter, $propertyAfter)
78+
);
79+
80+
return;
81+
}
82+
}
83+
}
84+
6585
$report->add($this->context, new PropertyAdded($this->context, $fileAfter, $classAfter, $propertyAfter));
6686
}
6787

88+
/**
89+
* Check if there is such property in class inheritance chain.
90+
*
91+
* @param Entity $class
92+
* @param string $propertyName
93+
* @return boolean
94+
*/
95+
private function searchPropertyExistsRecursive($class, $propertyName)
96+
{
97+
/** @var Entity $entity */
98+
foreach ($class->getExtends() as $entity) {
99+
$properties = $entity->getPropertyList();
100+
// checks if the property is already exiting in parent class
101+
if (isset($properties[$propertyName])) {
102+
return true;
103+
}
104+
105+
$result = $this->searchPropertyExistsRecursive($entity, $propertyName);
106+
if ($result) {
107+
return true;
108+
}
109+
}
110+
111+
return false;
112+
}
113+
68114
/**
69115
* Create and report a PropertyRemoved operation
70116
*

‎src/ClassHierarchy/DependencyInspectionVisitor.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use PhpParser\Node\Stmt\ClassLike;
1616
use PhpParser\Node\Stmt\ClassMethod;
1717
use PhpParser\Node\Stmt\Interface_ as InterfaceNode;
18-
use PhpParser\Node\Stmt\PropertyProperty;
18+
use PhpParser\Node\Stmt\Property;
1919
use PhpParser\Node\Stmt\Trait_ as TraitNode;
2020
use PhpParser\Node\Stmt\TraitUse;
2121
use PhpParser\NodeTraverser;
@@ -86,7 +86,7 @@ public function enterNode(Node $node)
8686
$this->currentClassLike->addUses($traitEntity);
8787
}
8888
return NodeTraverser::DONT_TRAVERSE_CHILDREN;
89-
case $node instanceof PropertyProperty:
89+
case $node instanceof Property:
9090
$this->currentClassLike->addProperty($node);
9191
return NodeTraverser::DONT_TRAVERSE_CHILDREN;
9292
default:

‎src/ClassHierarchy/Entity.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -387,11 +387,11 @@ public function addMethod(ClassMethod $method): void
387387
}
388388

389389
/**
390-
* @param PropertyProperty $property
390+
* @param Property $property
391391
*/
392-
public function addProperty(PropertyProperty $property): void
392+
public function addProperty(Property $property): void
393393
{
394-
$this->propertyList[$property->name] = $property;
394+
$this->propertyList[$property->props[0]->name->toString()] = $property;
395395
}
396396

397397
/**

‎src/Operation/PropertyOverwriteAdded.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
/**
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace Magento\SemanticVersionChecker\Operation;
11+
12+
use PHPSemVerChecker\Operation\PropertyOperationUnary;
13+
use PHPSemVerChecker\SemanticVersioning\Level;
14+
15+
class PropertyOverwriteAdded extends PropertyOperationUnary
16+
{
17+
/**
18+
* @var array
19+
*/
20+
protected $code = [
21+
'class' => ['M019', 'M020', 'M026'],
22+
];
23+
24+
/**
25+
* @var string
26+
*/
27+
protected $reason = 'Property overwrite has been added.';
28+
29+
/**
30+
* @var array
31+
*/
32+
private $mapping = [
33+
'M019' => Level::PATCH,
34+
'M020' => Level::PATCH,
35+
'M026' => Level::PATCH,
36+
];
37+
38+
/**
39+
* Returns level of error.
40+
*
41+
* @return mixed
42+
*/
43+
public function getLevel()
44+
{
45+
return $this->mapping[$this->getCode()];
46+
}
47+
}

‎tests/Unit/Console/Command/CompareSourceCommandApiClassesTest.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,24 @@ public function changesDataProvider()
514514
],
515515
'Patch change is detected.'
516516
],
517+
'protected-property-overwrite' => [
518+
$pathToFixtures . '/protected-property-overwrite/source-code-before',
519+
$pathToFixtures . '/protected-property-overwrite/source-code-after',
520+
[
521+
'Class (PATCH)',
522+
'Test\Vcs\TestClass::$_cacheTag | [protected] Property overwrite has been added. | M020'
523+
],
524+
'Patch change is detected.'
525+
],
526+
'public-property-overwrite' => [
527+
$pathToFixtures . '/public-property-overwrite/source-code-before',
528+
$pathToFixtures . '/public-property-overwrite/source-code-after',
529+
[
530+
'Class (PATCH)',
531+
'Test\Vcs\TestClass::$_cacheTag | [public] Property overwrite has been added. | M019'
532+
],
533+
'Patch change is detected.'
534+
],
517535
];
518536
}
519537
}

‎tests/Unit/Console/Command/CompareSourceCommandNonApiClassesTest.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,24 @@ public function changesDataProvider()
242242
],
243243
'Patch change is detected.'
244244
],
245+
'protected-property-overwrite' => [
246+
$pathToFixtures . '/protected-property-overwrite/source-code-before',
247+
$pathToFixtures . '/protected-property-overwrite/source-code-after',
248+
[
249+
'Class (PATCH)',
250+
'Test\Vcs\TestClass::$_cacheTag | [protected] Property overwrite has been added. | M020'
251+
],
252+
'Patch change is detected.'
253+
],
254+
'public-property-overwrite' => [
255+
$pathToFixtures . '/public-property-overwrite/source-code-before',
256+
$pathToFixtures . '/public-property-overwrite/source-code-after',
257+
[
258+
'Class (PATCH)',
259+
'Test\Vcs\TestClass::$_cacheTag | [public] Property overwrite has been added. | M019'
260+
],
261+
'Patch change is detected.'
262+
],
245263
];
246264
}
247265
}

‎tests/Unit/Console/Command/CompareSourceCommandTest/_files/api-class/protected-property-overwrite/source-code-after/TestClass.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<?php
2+
/**
3+
*
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
namespace Test\Vcs;
8+
9+
/**
10+
* Class TestClass
11+
* @api
12+
*/
13+
class TestClass extends TestClassParent
14+
{
15+
/**
16+
* @var array
17+
*/
18+
protected $_cacheTag = [
19+
'test',
20+
];
21+
}

‎tests/Unit/Console/Command/CompareSourceCommandTest/_files/api-class/protected-property-overwrite/source-code-after/TestClassParent.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
/**
3+
*
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
namespace Test\Vcs;
8+
9+
/**
10+
* Class TestClassParent
11+
* @api
12+
*/
13+
class TestClassParent
14+
{
15+
/**
16+
* Model cache tag for clear cache in after save and after delete
17+
*
18+
* When you use true - all cache will be clean
19+
*
20+
* @var string|array|bool
21+
*/
22+
protected $_cacheTag = false;
23+
}

‎tests/Unit/Console/Command/CompareSourceCommandTest/_files/api-class/protected-property-overwrite/source-code-before/TestClass.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php
2+
/**
3+
*
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
namespace Test\Vcs;
8+
9+
/**
10+
* Class TestClass
11+
* @api
12+
*/
13+
class TestClass extends TestClassParent
14+
{
15+
16+
}

0 commit comments

Comments
 (0)