From a2d04e4729c26bf53f79d1e74b2af766d7ebc465 Mon Sep 17 00:00:00 2001 From: Dave Date: Sun, 12 Jul 2026 09:42:07 +0100 Subject: [PATCH] Assorted small clean ups - Rename FqcnRemover::removeRqcn() to removeFqcn() (typo). - Remove StringUtils::endsWith(): unused in src, subtly wrong for an empty needle, and PHP 8 provides str_ends_with. - Register 'remove' as a real alias of remove-baseline-results. The documented 'sarb remove' only worked via Symfony's unambiguous abbreviation matching, so adding any future remove-* command would have silently broken every user's CI. - list-static-analysis-tools printed copy/paste commands that fail: both were missing the mandatory baseline-file argument. - --clean-up now picks its random sample from the issues that matched the baseline, as its help text says, rather than from all the latest issues (which included the new ones just reported). - Set the console application name and version: sarb --version printed 'Console Tool'. - InvalidOutputFormatterException now extends SarbException (the error reporter assumes all exceptions do). - Exclude value objects and exceptions from the service container sweep, so a newly type hinted value object fails at container compile time rather than at runtime. - Add an architecture rule: Domain must not depend on Symfony (it is currently clean; this stops erosion). - Fix the PhpstanJsonResultsParser docblock which claimed PHPStan's general errors are ignored (SARB stops with an error). Co-authored-by: Dave Liddament --- config/services.yml | 5 ++ .../InvalidOutputFormatterException.php | 4 +- src/Domain/Pruner/PrunedResults.php | 17 ++++ src/Domain/Utils/FqcnRemover.php | 2 +- src/Domain/Utils/StringUtils.php | 10 --- .../Command/ListResultsParsesCommand.php | 4 +- .../RemoveBaseLineFromResultsCommand.php | 10 ++- .../internal/AddCommandCompilerPass.php | 11 +++ .../PhpstanJsonResultsParser.php | 5 +- tests/Integration/EndToEndTest.php | 3 +- tests/Unit/Core/Pruner/PrunedResultsTest.php | 88 +++++++++++++++++++ tests/Unit/Core/Utils/FqcnRemoverTest.php | 2 +- tests/Unit/Core/Utils/StringUtilsTest.php | 15 ---- tools/Phpat/LayerTest.php | 10 +++ 14 files changed, 150 insertions(+), 36 deletions(-) create mode 100644 tests/Unit/Core/Pruner/PrunedResultsTest.php diff --git a/config/services.yml b/config/services.yml index f02fe149..e926a4d0 100644 --- a/config/services.yml +++ b/config/services.yml @@ -6,3 +6,8 @@ services: DaveLiddament\StaticAnalysisResultsBaseliner\: resource: '../src/*' + # Value objects and exceptions are not services. Excluding them means a newly type hinted + # value object fails at container compile time rather than at runtime. + exclude: + - '../src/**/*Exception.php' + - '../src/Domain/Common/*' diff --git a/src/Domain/OutputFormatter/InvalidOutputFormatterException.php b/src/Domain/OutputFormatter/InvalidOutputFormatterException.php index 6ffdc118..09b196b9 100644 --- a/src/Domain/OutputFormatter/InvalidOutputFormatterException.php +++ b/src/Domain/OutputFormatter/InvalidOutputFormatterException.php @@ -2,6 +2,8 @@ namespace DaveLiddament\StaticAnalysisResultsBaseliner\Domain\OutputFormatter; -final class InvalidOutputFormatterException extends \Exception +use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\SarbException; + +final class InvalidOutputFormatterException extends SarbException { } diff --git a/src/Domain/Pruner/PrunedResults.php b/src/Domain/Pruner/PrunedResults.php index b7d5b453..7f11fae7 100644 --- a/src/Domain/Pruner/PrunedResults.php +++ b/src/Domain/Pruner/PrunedResults.php @@ -28,4 +28,21 @@ public function getInputAnalysisResults(): AnalysisResults { return $this->inputAnalysisResults; } + + /** + * Returns the input results that matched the baseline (i.e. input results minus the pruned ones). + */ + public function getBaseLinedResults(): AnalysisResults + { + $prunedAnalysisResults = $this->prunedResults->getAnalysisResults(); + + $baseLinedResults = []; + foreach ($this->inputAnalysisResults->getAnalysisResults() as $analysisResult) { + if (!in_array($analysisResult, $prunedAnalysisResults, true)) { + $baseLinedResults[] = $analysisResult; + } + } + + return new AnalysisResults($baseLinedResults); + } } diff --git a/src/Domain/Utils/FqcnRemover.php b/src/Domain/Utils/FqcnRemover.php index 232b3397..3c12b843 100644 --- a/src/Domain/Utils/FqcnRemover.php +++ b/src/Domain/Utils/FqcnRemover.php @@ -11,7 +11,7 @@ final class FqcnRemover /** * Removes anything that looks like a FQCN from the string. */ - public function removeRqcn(string $raw): string + public function removeFqcn(string $raw): string { $parts = explode(' ', $raw); $outputParts = []; diff --git a/src/Domain/Utils/StringUtils.php b/src/Domain/Utils/StringUtils.php index d4384036..c00db931 100644 --- a/src/Domain/Utils/StringUtils.php +++ b/src/Domain/Utils/StringUtils.php @@ -33,16 +33,6 @@ public static function removeFromStart(string $needle, string $haystack): string return substr($haystack, $length); } - /** - * Returns true if $haystack ends with $needle. - */ - public static function endsWith(string $needle, string $haystack): bool - { - $length = strlen($needle); - - return substr($haystack, -$length) === $needle; - } - /** * Returns true if line is empty (or contains only white space). */ diff --git a/src/Framework/Command/ListResultsParsesCommand.php b/src/Framework/Command/ListResultsParsesCommand.php index 4bdd4976..c8fbae64 100644 --- a/src/Framework/Command/ListResultsParsesCommand.php +++ b/src/Framework/Command/ListResultsParsesCommand.php @@ -49,7 +49,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln( sprintf( - ' Create baseline: %s | sarb create --input-format="%s"', + ' Create baseline: %s | sarb create --input-format="%s" ', $identifier->getToolCommand(), $identifier->getCode(), ), @@ -57,7 +57,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln( sprintf( - ' Remove baseline results: %s | sarb remove', + ' Remove baseline results: %s | sarb remove ', $identifier->getToolCommand(), ), ); diff --git a/src/Framework/Command/RemoveBaseLineFromResultsCommand.php b/src/Framework/Command/RemoveBaseLineFromResultsCommand.php index ab867ccd..1a707981 100644 --- a/src/Framework/Command/RemoveBaseLineFromResultsCommand.php +++ b/src/Framework/Command/RemoveBaseLineFromResultsCommand.php @@ -53,6 +53,11 @@ protected function configure(): void { $this->setDescription('Shows issues created since the baseline'); + // The documentation tells users to run "sarb remove". That currently works because + // Symfony matches unambiguous abbreviations, but a real alias means adding another + // remove-* command in the future can not silently break every user's CI. + $this->setAliases(['remove']); + $outputFormatters = $this->outputFormatterLookupService->getIdentifiers(); $this->addOption( self::OUTPUT_FORMAT, @@ -123,8 +128,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $returnCode = $outputAnalysisResults->hasNoIssues() ? 0 : 1; - if ($showRandomIssues && !$prunedResults->getInputAnalysisResults()->hasNoIssues()) { - $randomIssues = $this->randomResultsPicker->getRandomResultsToFix($prunedResults->getInputAnalysisResults()); + $baseLinedResults = $prunedResults->getBaseLinedResults(); + if ($showRandomIssues && !$baseLinedResults->hasNoIssues()) { + $randomIssues = $this->randomResultsPicker->getRandomResultsToFix($baseLinedResults); OutputWriter::writeToStdError( $output, diff --git a/src/Framework/Container/internal/AddCommandCompilerPass.php b/src/Framework/Container/internal/AddCommandCompilerPass.php index 117bfbba..158e30ee 100644 --- a/src/Framework/Container/internal/AddCommandCompilerPass.php +++ b/src/Framework/Container/internal/AddCommandCompilerPass.php @@ -10,6 +10,7 @@ namespace DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Container\internal; +use Composer\InstalledVersions; use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Container\Container; use Symfony\Component\Console\Application; use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; @@ -18,14 +19,24 @@ final class AddCommandCompilerPass implements CompilerPassInterface { + private const PACKAGE_NAME = 'dave-liddament/sarb'; + public function process(ContainerBuilder $container): void { $container->register(Application::class, Application::class); $definition = $container->getDefinition(Application::class); + $definition->setArguments(['SARB', $this->getVersion()]); $definition->setPublic(true); $taggedServices = $container->findTaggedServiceIds(Container::COMMAND_TAG); foreach (array_keys($taggedServices) as $id) { $definition->addMethodCall('add', [new Reference($id)]); } } + + private function getVersion(): string + { + return InstalledVersions::isInstalled(self::PACKAGE_NAME) + ? (InstalledVersions::getPrettyVersion(self::PACKAGE_NAME) ?? 'UNKNOWN') + : 'UNKNOWN'; + } } diff --git a/src/Plugins/ResultsParsers/PhpstanJsonResultsParser/PhpstanJsonResultsParser.php b/src/Plugins/ResultsParsers/PhpstanJsonResultsParser/PhpstanJsonResultsParser.php index e93f531c..b8c5e479 100644 --- a/src/Plugins/ResultsParsers/PhpstanJsonResultsParser/PhpstanJsonResultsParser.php +++ b/src/Plugins/ResultsParsers/PhpstanJsonResultsParser/PhpstanJsonResultsParser.php @@ -33,7 +33,8 @@ * Handles PHPStan's JSON output. * * NOTE: SARB only deals with errors that are attached to a particular line in a file. - * PHPStan can report general errors (not specific to file). These are ignored by SARB. + * PHPStan can report general errors (not specific to a file). If any of these are present SARB + * stops with an error, as they normally indicate the PHPStan run itself failed. */ final class PhpstanJsonResultsParser implements ResultsParser { @@ -121,7 +122,7 @@ private function convertAnalysisResultFromArray( } $rawType = ArrayUtils::getStringValue($analysisResultAsArray, self::TYPE); - $type = $this->fqcnRemover->removeRqcn($rawType); + $type = $this->fqcnRemover->removeFqcn($rawType); $location = Location::fromAbsoluteFileName( $absoluteFileName, diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 1561e59d..84480f4c 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -5,7 +5,6 @@ use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\ProjectRoot; use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Common\RelativeFileName; use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Utils\Path; -use DaveLiddament\StaticAnalysisResultsBaseliner\Domain\Utils\StringUtils; use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\CreateBaseLineCommand; use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\ListHistoryAnalysersCommand; use DaveLiddament\StaticAnalysisResultsBaseliner\Framework\Command\ListResultsParsesCommand; @@ -406,7 +405,7 @@ private function updatePathsInJsonFiles(ProjectRoot $projectRoot): void $files = scandir($directory); $this->assertNotFalse($files); foreach ($files as $file) { - if (StringUtils::endsWith('.json', $file)) { + if (str_ends_with($file, '.json')) { $fullPath = Path::makeAbsolute($file, $directory); $contents = file_get_contents($fullPath); $this->assertNotFalse($contents); diff --git a/tests/Unit/Core/Pruner/PrunedResultsTest.php b/tests/Unit/Core/Pruner/PrunedResultsTest.php new file mode 100644 index 00000000..df1767f8 --- /dev/null +++ b/tests/Unit/Core/Pruner/PrunedResultsTest.php @@ -0,0 +1,88 @@ +baseLinedResult = $this->buildAnalysisResult($projectRoot, '/FILE_1', 1, 'TYPE_1', Severity::error()); + $this->newResult = $this->buildAnalysisResult($projectRoot, '/FILE_2', 2, 'TYPE_2', Severity::error()); + } + + public function testGetters(): void + { + $baseLine = $this->createBaseLine(); + $prunedAnalysisResults = new AnalysisResults([$this->newResult]); + $inputAnalysisResults = new AnalysisResults([$this->baseLinedResult, $this->newResult]); + + $prunedResults = new PrunedResults($baseLine, $prunedAnalysisResults, $inputAnalysisResults); + + $this->assertSame($baseLine, $prunedResults->getBaseLine()); + $this->assertSame($prunedAnalysisResults, $prunedResults->getPrunedResults()); + $this->assertSame($inputAnalysisResults, $prunedResults->getInputAnalysisResults()); + } + + public function testBaseLinedResultsAreInputResultsMinusPrunedResults(): void + { + $prunedResults = new PrunedResults( + $this->createBaseLine(), + new AnalysisResults([$this->newResult]), + new AnalysisResults([$this->baseLinedResult, $this->newResult]), + ); + + $baseLinedResults = $prunedResults->getBaseLinedResults(); + + $this->assertSame([$this->baseLinedResult], $baseLinedResults->getAnalysisResults()); + } + + public function testNoBaseLinedResultsWhenAllInputResultsAreNew(): void + { + $prunedResults = new PrunedResults( + $this->createBaseLine(), + new AnalysisResults([$this->baseLinedResult, $this->newResult]), + new AnalysisResults([$this->baseLinedResult, $this->newResult]), + ); + + $this->assertTrue($prunedResults->getBaseLinedResults()->hasNoIssues()); + } + + private function createBaseLine(): BaseLine + { + $baseLineResultsBuilder = new BaseLineResultsBuilder(); + $baseLineResultsBuilder->add('FILE_1', 1, 'TYPE_1', Severity::error()); + + return new BaseLine( + new HistoryFactoryStub(), + $baseLineResultsBuilder->build(), + new SarbJsonResultsParser(), + new GitCommit('fae40b3d596780ffd746dbd2300d05dcfbd09033'), + ); + } +} diff --git a/tests/Unit/Core/Utils/FqcnRemoverTest.php b/tests/Unit/Core/Utils/FqcnRemoverTest.php index 02d3353a..56c0fffc 100644 --- a/tests/Unit/Core/Utils/FqcnRemoverTest.php +++ b/tests/Unit/Core/Utils/FqcnRemoverTest.php @@ -33,7 +33,7 @@ public static function dataProvider(): array public function testFqcnRemover(string $input, string $expected): void { $fqcnRemove = new FqcnRemover(); - $actual = $fqcnRemove->removeRqcn($input); + $actual = $fqcnRemove->removeFqcn($input); $this->assertEquals($expected, $actual); } } diff --git a/tests/Unit/Core/Utils/StringUtilsTest.php b/tests/Unit/Core/Utils/StringUtilsTest.php index e6f64752..533e2053 100644 --- a/tests/Unit/Core/Utils/StringUtilsTest.php +++ b/tests/Unit/Core/Utils/StringUtilsTest.php @@ -33,21 +33,6 @@ public function testRemoveFromStartExceptionWhenInvalidStart(): void StringUtils::removeFromStart('Bar', 'FooBar'); } - public function testDoesEndWith1(): void - { - $this->assertTrue(StringUtils::endsWith('e', 'abcde')); - } - - public function testDoesEndWith2(): void - { - $this->assertTrue(StringUtils::endsWith('de', 'abcde')); - } - - public function testDoesNotEndWith1(): void - { - $this->assertFalse(StringUtils::endsWith('d', 'abcde')); - } - public function testIsEmpty(): void { $this->assertTrue(StringUtils::isEmptyLine('')); diff --git a/tools/Phpat/LayerTest.php b/tools/Phpat/LayerTest.php index 669dba73..91b9cfe5 100644 --- a/tools/Phpat/LayerTest.php +++ b/tools/Phpat/LayerTest.php @@ -26,6 +26,16 @@ public function testDomainDoesNotDependOnOtherLayers(): Rule ; } + public function testDomainDoesNotDependOnSymfony(): Rule + { + return PHPat::rule() + ->classes(Selector::inNamespace(self::DOMAIN_NAMESPACE)) + ->shouldNotDependOn() + ->classes(Selector::inNamespace('Symfony')) + ->because('Domain code should not depend on the framework') + ; + } + public function testsPluginDoesNotDependOnFrameworkLayer(): Rule { return PHPat::rule()