Skip to content

Commit 0558b4d

Browse files
committed
Fix baseline invalidation when allowed namespaces change (#377)
Extract the stable violation-specific part from error messages during baseline comparison, so that changes to rule configuration (e.g. adding or removing allowed namespaces) do not invalidate existing baseline entries. https://claude.ai/code/session_01DPotRHDJzXnmPGPoCVp8Rn
1 parent 415aa56 commit 0558b4d

File tree

2 files changed

+122
-3
lines changed

2 files changed

+122
-3
lines changed

src/Rules/Violations.php

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public function remove(self $violations, bool $ignoreBaselineLinenumbers = false
9898
foreach ($baselineViolations as $baseIdx => $baselineViolation) {
9999
if (
100100
$baselineViolation->getFqcn() === $violation->getFqcn()
101-
&& $baselineViolation->getError() === $violation->getError()
101+
&& self::extractViolationKey($baselineViolation->getError()) === self::extractViolationKey($violation->getError())
102102
) {
103103
unset($this->violations[$idx], $baselineViolations[$baseIdx]);
104104
continue 2;
@@ -121,21 +121,58 @@ public function jsonSerialize(): array
121121

122122
/**
123123
* Comparison method that respects all fields in the violation.
124+
*
125+
* Uses the stable violation key (the part before ', but ') for comparison,
126+
* so that changes to rule configuration (e.g. allowed namespaces) do not
127+
* invalidate existing baseline entries.
124128
*/
125129
public static function compareViolations(Violation $a, Violation $b): int
126130
{
127-
return $a <=> $b;
131+
return [
132+
$a->getFqcn(),
133+
$a->getLine(),
134+
$a->getFilePath(),
135+
self::extractViolationKey($a->getError()),
136+
] <=> [
137+
$b->getFqcn(),
138+
$b->getLine(),
139+
$b->getFilePath(),
140+
self::extractViolationKey($b->getError()),
141+
];
128142
}
129143

130144
/**
131145
* Comparison method that only checks the namespace and error but ignores the line number.
132146
*/
133147
public static function compareViolationsIgnoreLineNumber(Violation $a, Violation $b): int
134148
{
135-
if (($a->getFqcn() === $b->getFqcn()) && ($a->getError() === $b->getError())) {
149+
if (
150+
$a->getFqcn() === $b->getFqcn()
151+
&& self::extractViolationKey($a->getError()) === self::extractViolationKey($b->getError())
152+
) {
136153
return 0;
137154
}
138155

139156
return self::compareViolations($a, $b);
140157
}
158+
159+
/**
160+
* Extracts the stable violation-specific part from an error message.
161+
*
162+
* ViolationMessage produces two formats:
163+
* - withDescription: "$violation, but $ruleDescription" → returns $violation
164+
* - selfExplanatory: "$ruleDescription" (no ", but ") → returns the full string
165+
*
166+
* The rule description may include configuration-dependent values (like namespace lists)
167+
* that change when the rule config is updated. The violation part is always stable.
168+
*/
169+
private static function extractViolationKey(string $error): string
170+
{
171+
$pos = strpos($error, ', but ');
172+
if (false !== $pos) {
173+
return substr($error, 0, $pos);
174+
}
175+
176+
return $error;
177+
}
141178
}

tests/Unit/Rules/ViolationsTest.php

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,88 @@ public function test_sort(): void
155155
], $violationStore->toArray());
156156
}
157157

158+
public function test_remove_violations_matches_when_rule_description_changes(): void
159+
{
160+
$violations = new Violations();
161+
$violations->add(new Violation(
162+
'App\Foo',
163+
'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain, App\Shared',
164+
10
165+
));
166+
167+
$baseline = new Violations();
168+
$baseline->add(new Violation(
169+
'App\Foo',
170+
'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain',
171+
10
172+
));
173+
174+
$violations->remove($baseline);
175+
176+
self::assertCount(0, $violations);
177+
}
178+
179+
public function test_remove_violations_matches_when_rule_description_changes_ignore_linenumber(): void
180+
{
181+
$violations = new Violations();
182+
$violations->add(new Violation(
183+
'App\Foo',
184+
'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain, App\Shared',
185+
15
186+
));
187+
188+
$baseline = new Violations();
189+
$baseline->add(new Violation(
190+
'App\Foo',
191+
'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain',
192+
10
193+
));
194+
195+
$violations->remove($baseline, true);
196+
197+
self::assertCount(0, $violations);
198+
}
199+
200+
public function test_remove_violations_does_not_match_different_dependency(): void
201+
{
202+
$violations = new Violations();
203+
$violations->add(new Violation(
204+
'App\Foo',
205+
'depends on App\Baz, but should depend only on classes in one of these namespaces: App\Domain',
206+
10
207+
));
208+
209+
$baseline = new Violations();
210+
$baseline->add(new Violation(
211+
'App\Foo',
212+
'depends on App\Bar, but should depend only on classes in one of these namespaces: App\Domain',
213+
10
214+
));
215+
216+
$violations->remove($baseline);
217+
218+
self::assertCount(1, $violations);
219+
}
220+
221+
public function test_remove_violations_still_works_for_self_explanatory_messages(): void
222+
{
223+
$violations = new Violations();
224+
$violations->add(new Violation(
225+
'App\Foo',
226+
'should be final because we want immutability'
227+
));
228+
229+
$baseline = new Violations();
230+
$baseline->add(new Violation(
231+
'App\Foo',
232+
'should be final because we want immutability'
233+
));
234+
235+
$violations->remove($baseline);
236+
237+
self::assertCount(0, $violations);
238+
}
239+
158240
public function test_remove_violations_from_violations_ignore_linenumber(): void
159241
{
160242
$violation1 = new Violation(

0 commit comments

Comments
 (0)