Skip to content

Commit 7994eb0

Browse files
pdohogne-magentopdohogne-magento
committed
Code review changes
1 parent d546194 commit 7994eb0

File tree

9 files changed

+150
-39
lines changed

9 files changed

+150
-39
lines changed

‎src/Analyzer/AbstractCodeAnalyzer.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ protected function getFileName($context, $nodeName, $isBefore = true)
244244
* @param string $context
245245
* @param string $fileBefore
246246
* @param string $fileAfter
247-
* @return array
247+
* @return AbstractCodeAnalyzer[]
248248
*/
249249
protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
250250
{

‎src/Analyzer/ClassAnalyzer.php

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
108108
$classAfter = $afterNameMap[$key];
109109

110110
if ($classBefore !== $classAfter) {
111-
/** @var AnalyzerInterface[] $analyzers */
112111
$analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter);
113112

114113
foreach ($analyzers as $analyzer) {
@@ -125,7 +124,7 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
125124
* @param string $context
126125
* @param string $fileBefore
127126
* @param string $fileAfter
128-
* @return array
127+
* @return AbstractCodeAnalyzer[]
129128
*/
130129
protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
131130
{
@@ -140,7 +139,6 @@ protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
140139
];
141140
}
142141

143-
144142
/**
145143
* Get the class node registry
146144
*

‎src/Analyzer/InterfaceAnalyzer.php

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
118118
$interfaceAfter = $afterNameMap[$key];
119119

120120
if ($interfaceBefore !== $interfaceAfter) {
121-
/** @var AnalyzerInterface[] $analyzers */
122121
$analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter);
123122

124123
foreach ($analyzers as $analyzer) {
@@ -135,7 +134,7 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
135134
* @param string $context
136135
* @param string $fileBefore
137136
* @param string $fileAfter
138-
* @return array
137+
* @return AbstractCodeAnalyzer[]
139138
*/
140139
protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
141140
{
@@ -146,6 +145,4 @@ protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
146145
new InterfaceExtendsAnalyzer($context, $fileBefore, $fileAfter)
147146
];
148147
}
149-
150-
151148
}

‎src/Analyzer/TraitAnalyzer.php

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,6 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
115115
if ($traitBefore !== $traitAfter) {
116116
$fileBefore = $this->getFileName($registryBefore, $traitName);
117117
$fileAfter = $this->getFileName($registryAfter, $traitName);
118-
119-
/** @var AbstractCodeAnalyzer[] $analyzers */
120118
$analyzers = $this->getContentAnalyzers(static::CONTEXT, $fileBefore, $fileAfter);
121119

122120
foreach ($analyzers as $analyzer) {
@@ -133,12 +131,10 @@ protected function reportChanged($report, $registryBefore, $registryAfter, $toVe
133131
* @param string $context
134132
* @param string $fileBefore
135133
* @param string $fileAfter
136-
* @return array
134+
* @return AbstractCodeAnalyzer[]
137135
*/
138136
protected function getContentAnalyzers($context, $fileBefore, $fileAfter)
139137
{
140138
return [new ClassMethodAnalyzer($context, $fileBefore, $fileAfter)];
141139
}
142-
143-
144140
}

‎src/DbSchemaReport.php

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,28 +11,18 @@
1111

1212
class DbSchemaReport extends ReportAlias
1313
{
14-
public static $contexts = [
15-
'class',
16-
'function',
17-
'interface',
18-
'trait',
19-
'database',
20-
'di',
21-
'layout',
22-
'system',
23-
'xsd',
24-
'less'
25-
];
26-
2714
/**
2815
* Report constructor.
2916
*/
3017
public function __construct()
3118
{
3219
$levels = array_fill_keys(Level::asList(), []);
3320
parent::__construct();
34-
foreach (static::$contexts as $context) {
35-
$this->differences[$context] = $levels;
36-
}
21+
$this->differences['database'] = $levels;
22+
$this->differences['di'] = $levels;
23+
$this->differences['layout'] = $levels;
24+
$this->differences['system'] = $levels;
25+
$this->differences['xsd'] = $levels;
26+
$this->differences['less'] = $levels;
3727
}
3828
}

‎src/MergedReport.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<?php
2+
3+
/**
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
8+
namespace Magento\SemanticVersionChecker;
9+
10+
use PHPSemVerChecker\Report\Report;
11+
12+
class MergedReport extends Report
13+
{
14+
/**
15+
* Merges with the given report including any non-standard contexts
16+
*
17+
* @param Report $report
18+
* @return $this
19+
*/
20+
public function merge(Report $report)
21+
{
22+
foreach ($report->differences as $context => $levels) {
23+
if (!key_exists($context, $this->differences)) {
24+
$this->differences[$context] = $levels;
25+
} else {
26+
foreach ($levels as $level => $differences) {
27+
$this->differences[$context][$level] = array_merge($this->differences[$context][$level], $differences);
28+
}
29+
}
30+
}
31+
32+
return $this;
33+
}
34+
}

‎src/ReportBuilder.php

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ class ReportBuilder
4444

4545
/**
4646
* Define analyzer factory list for the different report types.
47-
* @var array
47+
* @var string[]
4848
*/
49-
private $analyzerList = [
49+
private $analyzerFactoryClasses = [
5050
ReportTypes::API => AnalyzerFactory::class,
5151
ReportTypes::ALL => NonApiAnalyzerFactory::class,
5252
ReportTypes::DB_SCHEMA => DbSchemaAnalyzerFactory::class,
@@ -121,11 +121,11 @@ protected function makeVersionReport()
121121
/**
122122
* Get the mapping of report type -> analyzer factory
123123
*
124-
* @return array
124+
* @return string[]
125125
*/
126-
protected function getAnalyzerFactories()
126+
protected function getAnalyzerFactoryClasses()
127127
{
128-
return $this->analyzerList;
128+
return $this->analyzerFactoryClasses;
129129
}
130130

131131
/**
@@ -171,7 +171,7 @@ protected function buildReport()
171171
/**
172172
* @var AnalyzerFactoryInterface $factory
173173
*/
174-
foreach ($this->getAnalyzerFactories() as $reportType => $factory) {
174+
foreach ($this->getAnalyzerFactoryClasses() as $reportType => $factory) {
175175
/** @var AnalyzerInterface $analyzer */
176176
$analyzer = (new $factory())->create($dependencyMap);
177177
$tmpReport = $analyzer->analyze(

‎src/Reporter/BreakingChangeTableReporter.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
// @codingStandardsIgnoreFile
88
namespace Magento\SemanticVersionChecker\Reporter;
99

10-
use Magento\SemanticVersionChecker\DbSchemaReport;
1110
use PHPSemVerChecker\Report\Report;
1211
use PHPSemVerChecker\SemanticVersioning\Level;
1312
use Symfony\Component\Console\Output\OutputInterface;
@@ -52,14 +51,15 @@ public function __construct($apiChangeReport, $apiMembershipReport, $targetFile)
5251
*/
5352
public function output(OutputInterface $output)
5453
{
55-
$contexts = DbSchemaReport::$contexts;
54+
$reportContexts = array_keys($this->report->getDifferences());
55+
$membershipContexts = array_keys($this->membershipReport->getDifferences());
5656

57-
foreach ($contexts as $context) {
57+
foreach ($reportContexts as $context) {
5858
$header = static::formatSectionHeader($this->targetFile, $context, 'breaking-change');
5959
$this->outputChangeReport($output, $this->report, $context, $header);
6060
}
6161

62-
foreach ($contexts as $context) {
62+
foreach ($membershipContexts as $context) {
6363
$header = static::formatSectionHeader($this->targetFile, $context, 'api-membership');
6464
$this->outputChangeReport($output, $this->membershipReport, $context, $header);
6565
}

‎tests/Unit/MergedReportTest.php

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
<?php
2+
3+
/**
4+
* Copyright © Magento, Inc. All rights reserved.
5+
* See COPYING.txt for license details.
6+
*/
7+
8+
namespace Magento\SemanticVersionChecker\Test\Unit;
9+
10+
use Magento\SemanticVersionChecker\InjectableReport;
11+
use Magento\SemanticVersionChecker\MergedReport;
12+
use Magento\SemanticVersionChecker\Operation\ClassConstantRemoved;
13+
use Magento\SemanticVersionChecker\Operation\ClassConstructorOptionalParameterAdded;
14+
use Magento\SemanticVersionChecker\Operation\ClassExtendsAdded;
15+
use Magento\SemanticVersionChecker\Operation\ClassImplementsAdded;
16+
use Magento\SemanticVersionChecker\Operation\ClassMethodOptionalParameterAdded;
17+
use Magento\SemanticVersionChecker\Operation\ClassTraitAdded;
18+
use Magento\SemanticVersionChecker\Operation\DropForeignKey;
19+
use Magento\SemanticVersionChecker\Operation\SystemXml\FieldAdded;
20+
use PHPSemVerChecker\Report\Report;
21+
use PHPSemVerChecker\SemanticVersioning\Level;
22+
use PHPUnit\Framework\MockObject\MockObject;
23+
use PHPUnit\Framework\TestCase;
24+
25+
class MergedReportTest extends TestCase
26+
{
27+
/**
28+
* Verify that reports are properly merged including non-standard contexts
29+
*
30+
* @return void
31+
*/
32+
public function testMergedChanges()
33+
{
34+
$diffsWithDatabase = [];
35+
$diffsWithDatabase['class'][Level::PATCH] = [$this->createMock(ClassConstructorOptionalParameterAdded::class)];
36+
$diffsWithDatabase['class'][Level::MINOR] = [
37+
$this->createMock(ClassExtendsAdded::class),
38+
$this->createMock(ClassMethodOptionalParameterAdded::class)
39+
];
40+
$diffsWithDatabase['database'][Level::MAJOR] = [$this->createMock(DropForeignKey::class)];
41+
$databaseReport = new InjectableReport($diffsWithDatabase);
42+
43+
$diffsWithXml = [];
44+
$diffsWithXml['class'][Level::MAJOR] = [$this->createMock(ClassConstantRemoved::class)];
45+
$diffsWithXml['class'][Level::MINOR] = [$this->createMock(ClassImplementsAdded::class)];
46+
$diffsWithXml['xml'][Level::MINOR] = [$this->createMock(FieldAdded::class)];
47+
$xmlReport = new InjectableReport($diffsWithXml);
48+
49+
/** @var MockObject|ClassTraitAdded $preMergeOperation */
50+
$preMergeOperation = $this->createMock(ClassTraitAdded::class);
51+
$preMergeOperation->expects($this->any())->method('getLevel')->willReturn(Level::MINOR);
52+
53+
$mergedReport = new MergedReport();
54+
$mergedReport->addClass($preMergeOperation);
55+
$mergedReport->merge($databaseReport);
56+
$mergedReport->merge($xmlReport);
57+
58+
$mergedDiffs = $mergedReport->getDifferences();
59+
$this->assertTrue(key_exists('database', $mergedDiffs));
60+
$this->assertTrue(key_exists('xml', $mergedDiffs));
61+
$this->assertTrue($this->hasAllDifferences($mergedReport, $databaseReport));
62+
$this->assertTrue($this->hasAllDifferences($mergedReport, $xmlReport));
63+
$this->assertTrue(array_search($preMergeOperation, $mergedDiffs['class'][Level::MINOR]) !== false);
64+
}
65+
66+
/**
67+
* Checks if a merged report contains all differences in another report
68+
69+
* @param Report $mergedReport
70+
* @param Report $inputReport
71+
* @return bool
72+
*/
73+
private function hasAllDifferences($mergedReport, $inputReport)
74+
{
75+
$mergedDiffs = $mergedReport->getDifferences();
76+
$inputDiffs = $inputReport->getDifferences();
77+
foreach ($inputDiffs as $context => $levels) {
78+
if (!key_exists($context, $mergedDiffs)) {
79+
return false;
80+
}
81+
$mergedLevels = $mergedDiffs[$context];
82+
foreach ($levels as $level => $operations) {
83+
if (!key_exists($level, $mergedLevels)) {
84+
return false;
85+
}
86+
$mergedOperations = $mergedLevels[$level];
87+
foreach ($operations as $operation) {
88+
if (array_search($operation, $mergedOperations) === false) {
89+
return false;
90+
}
91+
}
92+
}
93+
}
94+
return true;
95+
}
96+
}

0 commit comments

Comments
 (0)