Skip to content

Commit 21eb1a5

Browse files
floriankraemerfloriankraemer
authored
Merge pull request #5 from Phauthentic/refactor
Refactoring the code, minor improvements
2 parents 178bd19 + 7e0be48 commit 21eb1a5

15 files changed

Lines changed: 897 additions & 328 deletions

‎src/Checker/BcChecker.php‎

Lines changed: 83 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,13 @@
1818

1919
use Phauthentic\BcCheck\Detector\DetectorRegistry;
2020
use Phauthentic\BcCheck\Diff\RenameDetector;
21+
use Phauthentic\BcCheck\Diff\RenameMap;
2122
use Phauthentic\BcCheck\Git\GitRepositoryInterface;
23+
use Phauthentic\BcCheck\Parser\AnalysisResult;
2224
use Phauthentic\BcCheck\Parser\CodebaseAnalyzerInterface;
2325
use Phauthentic\BcCheck\ValueObject\BcBreak;
2426
use Phauthentic\BcCheck\ValueObject\BcBreakType;
27+
use Phauthentic\BcCheck\ValueObject\ClassInfo;
2528

2629
final readonly class BcChecker implements BcCheckerInterface
2730
{
@@ -33,50 +36,108 @@ public function __construct(
3336
) {
3437
}
3538

36-
public function check(string $fromCommit, string $toCommit): array
39+
/**
40+
* Validates that both commit references are valid.
41+
*
42+
* @throws InvalidCommitException When either commit is invalid
43+
*/
44+
private function validateCommits(string $fromCommit, string $toCommit): void
3745
{
38-
// Validate commits
3946
if (!$this->git->isValidCommit($fromCommit)) {
4047
throw InvalidCommitException::invalidFromCommit($fromCommit);
4148
}
4249

4350
if (!$this->git->isValidCommit($toCommit)) {
4451
throw InvalidCommitException::invalidToCommit($toCommit);
4552
}
53+
}
4654

47-
// Detect renames from diff
55+
/**
56+
* Detects method and property renames between commits.
57+
*
58+
* @return array<string, RenameMap> Map of file paths to rename maps
59+
*/
60+
private function detectRenames(string $fromCommit, string $toCommit): array
61+
{
4862
$diff = $this->git->getDiff($fromCommit, $toCommit);
49-
$renamesByFile = $this->renameDetector->detect($diff);
5063

51-
// Analyze both versions with file mapping
52-
$beforeResult = $this->analyzer->analyzeAtCommitWithFileMap($fromCommit, 'source');
53-
$afterResult = $this->analyzer->analyzeAtCommitWithFileMap($toCommit, 'target');
64+
return $this->renameDetector->detect($diff);
65+
}
5466

67+
/**
68+
* Analyzes both source and target codebases at their respective commits.
69+
*
70+
* @return array{0: AnalysisResult, 1: AnalysisResult} Array containing [sourceAnalysis, targetAnalysis]
71+
*/
72+
private function analyzeCodebases(string $fromCommit, string $toCommit): array
73+
{
74+
$sourceAnalysis = $this->analyzer->analyzeAtCommitWithFileMap($fromCommit, 'source');
75+
$targetAnalysis = $this->analyzer->analyzeAtCommitWithFileMap($toCommit, 'target');
76+
77+
return [$sourceAnalysis, $targetAnalysis];
78+
}
79+
80+
/**
81+
* Detects classes that have been removed between source and target analyses.
82+
*
83+
* @return list<BcBreak>
84+
*/
85+
private function detectRemovedClasses(AnalysisResult $sourceAnalysis, AnalysisResult $targetAnalysis): array
86+
{
5587
$breaks = [];
5688

57-
// Check for removed classes
58-
foreach ($beforeResult->getClasses() as $className => $beforeClass) {
59-
if (!$afterResult->hasClass($className)) {
89+
foreach ($sourceAnalysis->getClasses() as $className => $sourceClass) {
90+
if (!$targetAnalysis->hasClass($className)) {
6091
$breaks[] = new BcBreak(
61-
message: sprintf('%s %s was removed', ucfirst($beforeClass->type->value), $className),
92+
message: sprintf('%s %s was removed', ucfirst($sourceClass->type->value), $className),
6293
className: $className,
6394
type: BcBreakType::ClassRemoved,
6495
);
65-
66-
continue;
6796
}
97+
}
6898

69-
// Get rename map for this class's file
70-
$filePath = $beforeResult->getFileForClass($className);
71-
$renameMap = $filePath !== null ? ($renamesByFile[$filePath] ?? null) : null;
99+
return $breaks;
100+
}
72101

73-
// Compare classes - we know afterClass exists because hasClass was true
74-
/** @var \Phauthentic\BcCheck\ValueObject\ClassInfo $afterClass */
75-
$afterClass = $afterResult->getClass($className);
76-
$detected = $this->registry->detectAll($beforeClass, $afterClass, $renameMap);
102+
/**
103+
* Compares two versions of a class and detects backward compatibility breaks.
104+
*
105+
* @param array<string, RenameMap> $renamesByFile Map of file paths to rename maps
106+
* @return list<BcBreak>
107+
*/
108+
private function compareClasses(ClassInfo $sourceClass, ClassInfo $targetClass, AnalysisResult $sourceAnalysis, array $renamesByFile): array
109+
{
110+
$renameMap = $this->getRenameMapForClass($sourceClass->name, $sourceAnalysis, $renamesByFile);
111+
112+
return $this->registry->detectAll($sourceClass, $targetClass, $renameMap);
113+
}
114+
115+
/**
116+
* Gets the rename map for a specific class based on its file path.
117+
*
118+
* @param array<string, RenameMap> $renamesByFile Map of file paths to rename maps
119+
*/
120+
private function getRenameMapForClass(string $className, AnalysisResult $sourceAnalysis, array $renamesByFile): ?RenameMap
121+
{
122+
$filePath = $sourceAnalysis->getFileForClass($className);
123+
124+
return $filePath !== null ? ($renamesByFile[$filePath] ?? null) : null;
125+
}
126+
127+
public function check(string $fromCommit, string $toCommit): array
128+
{
129+
$this->validateCommits($fromCommit, $toCommit);
130+
$renamesByFile = $this->detectRenames($fromCommit, $toCommit);
131+
[$sourceAnalysis, $targetAnalysis] = $this->analyzeCodebases($fromCommit, $toCommit);
132+
133+
$breaks = [];
134+
$breaks = array_merge($breaks, $this->detectRemovedClasses($sourceAnalysis, $targetAnalysis));
77135

78-
foreach ($detected as $break) {
79-
$breaks[] = $break;
136+
foreach ($sourceAnalysis->getClasses() as $className => $sourceClass) {
137+
if ($targetAnalysis->hasClass($className)) {
138+
$targetClass = $targetAnalysis->getClass($className);
139+
assert($targetClass !== null); // Guaranteed by hasClass() check above
140+
$breaks = array_merge($breaks, $this->compareClasses($sourceClass, $targetClass, $sourceAnalysis, $renamesByFile));
80141
}
81142
}
82143

‎src/Command/CheckCommand.php‎

Lines changed: 10 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,8 @@
2222
use Phauthentic\BcCheck\Config\ConfigurationException;
2323
use Phauthentic\BcCheck\Config\ConfigurationLoader;
2424
use Phauthentic\BcCheck\DependencyInjection\ContainerFactory;
25+
use Phauthentic\BcCheck\Factory\OutputFormatterFactory;
2526
use Phauthentic\BcCheck\Git\GitException;
26-
use Phauthentic\BcCheck\Output\CheckstyleOutputFormatter;
27-
use Phauthentic\BcCheck\Output\GithubActionsFormatter;
28-
use Phauthentic\BcCheck\Output\GitlabCodeQualityOutputFormatter;
29-
use Phauthentic\BcCheck\Output\JsonOutputFormatter;
30-
use Phauthentic\BcCheck\Output\JunitOutputFormatter;
31-
use Phauthentic\BcCheck\Output\MarkdownOutputFormatter;
32-
use Phauthentic\BcCheck\Output\OutputFormatterInterface;
33-
use Phauthentic\BcCheck\Output\SarifOutputFormatter;
34-
use Phauthentic\BcCheck\Output\TextOutputFormatter;
3527
use Symfony\Component\Console\Attribute\AsCommand;
3628
use Symfony\Component\Console\Command\Command;
3729
use Symfony\Component\Console\Input\InputArgument;
@@ -45,14 +37,11 @@
4537
)]
4638
final class CheckCommand extends Command
4739
{
48-
private const FORMAT_TEXT = 'text';
49-
private const FORMAT_JSON = 'json';
50-
private const FORMAT_MARKDOWN = 'markdown';
51-
private const FORMAT_GITHUB = 'github-actions';
52-
private const FORMAT_SARIF = 'sarif';
53-
private const FORMAT_CHECKSTYLE = 'checkstyle';
54-
private const FORMAT_JUNIT = 'junit';
55-
private const FORMAT_GITLAB = 'gitlab';
40+
public function __construct(
41+
private readonly OutputFormatterFactory $formatterFactory = new OutputFormatterFactory(),
42+
) {
43+
parent::__construct();
44+
}
5645

5746
protected function configure(): void
5847
{
@@ -83,17 +72,10 @@ protected function configure(): void
8372
'f',
8473
InputOption::VALUE_REQUIRED,
8574
sprintf(
86-
'Output format (%s, %s, %s, %s, %s, %s, %s, %s)',
87-
self::FORMAT_TEXT,
88-
self::FORMAT_JSON,
89-
self::FORMAT_MARKDOWN,
90-
self::FORMAT_GITHUB,
91-
self::FORMAT_SARIF,
92-
self::FORMAT_CHECKSTYLE,
93-
self::FORMAT_JUNIT,
94-
self::FORMAT_GITLAB,
75+
'Output format (%s)',
76+
implode(', ', $this->formatterFactory->getSupportedFormats()),
9577
),
96-
self::FORMAT_TEXT,
78+
OutputFormatterFactory::FORMAT_TEXT,
9779
)
9880
->addOption(
9981
'show-files',
@@ -140,7 +122,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
140122
}
141123

142124
// Get formatter
143-
$formatter = $this->getFormatter($format);
125+
$formatter = $this->formatterFactory->create($format);
144126

145127
// Build container with services
146128
try {
@@ -205,18 +187,4 @@ private function loadConfiguration(?string $configPath): Configuration
205187

206188
return $loader->createDefault();
207189
}
208-
209-
private function getFormatter(string $format): OutputFormatterInterface
210-
{
211-
return match ($format) {
212-
self::FORMAT_JSON => new JsonOutputFormatter(),
213-
self::FORMAT_MARKDOWN => new MarkdownOutputFormatter(),
214-
self::FORMAT_GITHUB => new GithubActionsFormatter(),
215-
self::FORMAT_SARIF => new SarifOutputFormatter(),
216-
self::FORMAT_CHECKSTYLE => new CheckstyleOutputFormatter(),
217-
self::FORMAT_JUNIT => new JunitOutputFormatter(),
218-
self::FORMAT_GITLAB => new GitlabCodeQualityOutputFormatter(),
219-
default => new TextOutputFormatter(),
220-
};
221-
}
222190
}

‎src/Config/Configuration.php‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,49 @@ public function getExternalDetectors(): array
5252
return $this->externalDetectors;
5353
}
5454

55+
/**
56+
* Determines if a fully qualified class name should be included in analysis.
57+
*/
5558
public function shouldInclude(string $fqcn): bool
5659
{
57-
// Check exclude patterns first
60+
if ($this->isExcluded($fqcn)) {
61+
return false;
62+
}
63+
64+
if (!$this->hasIncludePatterns()) {
65+
return true;
66+
}
67+
68+
return $this->isIncluded($fqcn);
69+
}
70+
71+
/**
72+
* Checks if the FQCN matches any exclude pattern.
73+
*/
74+
private function isExcluded(string $fqcn): bool
75+
{
5876
foreach ($this->excludePatterns as $pattern) {
5977
if ($this->matchesPattern($fqcn, $pattern)) {
60-
return false;
78+
return true;
6179
}
6280
}
6381

64-
// If no include patterns, include everything not excluded
65-
if ($this->includePatterns === []) {
66-
return true;
67-
}
82+
return false;
83+
}
84+
85+
/**
86+
* Checks if include patterns are configured.
87+
*/
88+
private function hasIncludePatterns(): bool
89+
{
90+
return $this->includePatterns !== [];
91+
}
6892

69-
// Check include patterns
93+
/**
94+
* Checks if the FQCN matches any include pattern.
95+
*/
96+
private function isIncluded(string $fqcn): bool
97+
{
7098
foreach ($this->includePatterns as $pattern) {
7199
if ($this->matchesPattern($fqcn, $pattern)) {
72100
return true;

‎src/Config/ConfigurationLoader.php‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,13 @@ public function load(string $path): Configuration
4949
*/
5050
public function createFromArray(array $data): Configuration
5151
{
52+
/** @var list<string> $includePatterns */
5253
$includePatterns = $this->extractStringList($data, 'include');
54+
/** @var list<string> $excludePatterns */
5355
$excludePatterns = $this->extractStringList($data, 'exclude');
56+
/** @var list<string> $sourceDirectories */
5457
$sourceDirectories = $this->extractStringList($data, 'source_directories');
58+
/** @var list<string> $externalDetectors */
5559
$externalDetectors = $this->extractStringList($data, 'external_detectors');
5660

5761
// Validate regex patterns
@@ -79,6 +83,27 @@ public function createDefault(): Configuration
7983
return new Configuration();
8084
}
8185

86+
/**
87+
* Validates that all items in an array are strings and returns them as a list.
88+
*
89+
* @param array<mixed> $items
90+
* @param string $context Description of the configuration key for error messages
91+
* @throws ConfigurationException When any item is not a string
92+
* @return list<string>
93+
*/
94+
private function validateStringItems(array $items, string $context): array
95+
{
96+
$result = [];
97+
foreach ($items as $item) {
98+
if (!is_string($item)) {
99+
throw ConfigurationException::itemsMustBeStrings($context);
100+
}
101+
$result[] = $item;
102+
}
103+
104+
return $result;
105+
}
106+
82107
/**
83108
* @param array<string, mixed> $data
84109
* @return list<string>
@@ -93,15 +118,7 @@ private function extractStringList(array $data, string $key): array
93118
throw ConfigurationException::keyMustBeArray($key);
94119
}
95120

96-
$result = [];
97-
foreach ($data[$key] as $item) {
98-
if (!is_string($item)) {
99-
throw ConfigurationException::itemsMustBeStrings($key);
100-
}
101-
$result[] = $item;
102-
}
103-
104-
return $result;
121+
return $this->validateStringItems($data[$key], $key);
105122
}
106123

107124
private function validatePattern(string $pattern, string $context): void

0 commit comments

Comments
 (0)