diff --git a/src/Rules/Violations.php b/src/Rules/Violations.php index 4ef4e206..47fe5f08 100644 --- a/src/Rules/Violations.php +++ b/src/Rules/Violations.php @@ -98,7 +98,7 @@ public function remove(self $violations, bool $ignoreBaselineLinenumbers = false foreach ($baselineViolations as $baseIdx => $baselineViolation) { if ( $baselineViolation->getFqcn() === $violation->getFqcn() - && $baselineViolation->getError() === $violation->getError() + && self::extractViolationKey($baselineViolation->getError()) === self::extractViolationKey($violation->getError()) ) { unset($this->violations[$idx], $baselineViolations[$baseIdx]); continue 2; @@ -121,21 +121,43 @@ public function jsonSerialize(): array /** * Comparison method that respects all fields in the violation. + * + * Uses the stable violation key (the part before ', but ') for comparison, + * so that changes to rule configuration (e.g. allowed namespaces) do not + * invalidate existing baseline entries. */ public static function compareViolations(Violation $a, Violation $b): int { - return $a <=> $b; + return [ + $a->getFqcn(), + $a->getLine(), + $a->getFilePath(), + self::extractViolationKey($a->getError()), + ] <=> [ + $b->getFqcn(), + $b->getLine(), + $b->getFilePath(), + self::extractViolationKey($b->getError()), + ]; } /** - * Comparison method that only checks the namespace and error but ignores the line number. + * Extracts the stable violation-specific part from an error message. + * + * ViolationMessage produces two formats: + * - withDescription: "$violation, but $ruleDescription" → returns $violation + * - selfExplanatory: "$ruleDescription" (no ", but ") → returns the full string + * + * The rule description may include configuration-dependent values (like namespace lists) + * that change when the rule config is updated. The violation part is always stable. */ - public static function compareViolationsIgnoreLineNumber(Violation $a, Violation $b): int + private static function extractViolationKey(string $error): string { - if (($a->getFqcn() === $b->getFqcn()) && ($a->getError() === $b->getError())) { - return 0; + $pos = strpos($error, ', but '); + if (false !== $pos) { + return substr($error, 0, $pos); } - return self::compareViolations($a, $b); + return $error; } } diff --git a/tests/Unit/Rules/ViolationsTest.php b/tests/Unit/Rules/ViolationsTest.php index ca0299e8..b5eaf755 100644 --- a/tests/Unit/Rules/ViolationsTest.php +++ b/tests/Unit/Rules/ViolationsTest.php @@ -155,6 +155,88 @@ public function test_sort(): void ], $violationStore->toArray()); } + public function test_remove_violations_matches_when_rule_description_changes(): void + { + $violations = new Violations(); + $violations->add(new Violation( + 'App\Foo', + 'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain, App\Shared', + 10 + )); + + $baseline = new Violations(); + $baseline->add(new Violation( + 'App\Foo', + 'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain', + 10 + )); + + $violations->remove($baseline); + + self::assertCount(0, $violations); + } + + public function test_remove_violations_matches_when_rule_description_changes_ignore_linenumber(): void + { + $violations = new Violations(); + $violations->add(new Violation( + 'App\Foo', + 'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain, App\Shared', + 15 + )); + + $baseline = new Violations(); + $baseline->add(new Violation( + 'App\Foo', + 'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain', + 10 + )); + + $violations->remove($baseline, true); + + self::assertCount(0, $violations); + } + + public function test_remove_violations_does_not_match_different_dependency(): void + { + $violations = new Violations(); + $violations->add(new Violation( + 'App\Foo', + 'depends on App\Baz, but should depend only on classes in one of these namespaces: App\Domain', + 10 + )); + + $baseline = new Violations(); + $baseline->add(new Violation( + 'App\Foo', + 'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain', + 10 + )); + + $violations->remove($baseline); + + self::assertCount(1, $violations); + } + + public function test_remove_violations_still_works_for_self_explanatory_messages(): void + { + $violations = new Violations(); + $violations->add(new Violation( + 'App\Foo', + 'should be final because we want immutability' + )); + + $baseline = new Violations(); + $baseline->add(new Violation( + 'App\Foo', + 'should be final because we want immutability' + )); + + $violations->remove($baseline); + + self::assertCount(0, $violations); + } + public function test_remove_violations_from_violations_ignore_linenumber(): void { $violation1 = new Violation(