Skip to content

Commit 7ea44f6

Browse files
committed
Fix duplicate violations when reusing that() for multiple should() calls
This fixes issue #303 where reusing the result of that() for multiple should() calls would cause duplicate violations due to shared mutable state in the RuleBuilder. The fix makes should() and andThat() clone the RuleBuilder before adding constraints/specs, ensuring each rule branch gets an isolated copy of the builder state. Added RuleBuilder::__clone() to deep clone Specs and Constraints objects.
1 parent efbe06b commit 7ea44f6

File tree

3 files changed

+113
-4
lines changed

3 files changed

+113
-4
lines changed

src/Rules/AndThatShould.php

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,17 @@ public function __construct(RuleBuilder $expressionBuilder)
1919

2020
public function andThat(Expression $expression): AndThatShouldParser
2121
{
22-
$this->ruleBuilder->addThat($expression);
22+
$ruleBuilder = clone $this->ruleBuilder;
23+
$ruleBuilder->addThat($expression);
2324

24-
return $this;
25+
return new self($ruleBuilder);
2526
}
2627

2728
public function should(Expression $expression): BecauseParser
2829
{
29-
$this->ruleBuilder->addShould($expression);
30+
$ruleBuilder = clone $this->ruleBuilder;
31+
$ruleBuilder->addShould($expression);
3032

31-
return new Because($this->ruleBuilder);
33+
return new Because($ruleBuilder);
3234
}
3335
}

src/Rules/RuleBuilder.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,10 @@ public function setRunOnlyThis(): self
7575

7676
return $this;
7777
}
78+
79+
public function __clone()
80+
{
81+
$this->thats = clone $this->thats;
82+
$this->shoulds = clone $this->shoulds;
83+
}
7884
}
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Arkitect\Tests\Unit\Rules;
6+
7+
use Arkitect\Analyzer\ClassDescription;
8+
use Arkitect\Expression\ForClasses\Extend;
9+
use Arkitect\Expression\ForClasses\HaveNameMatching;
10+
use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces;
11+
use Arkitect\Rules\Rule;
12+
use Arkitect\Rules\Violations;
13+
use PHPUnit\Framework\TestCase;
14+
15+
class RuleBuilderTest extends TestCase
16+
{
17+
/**
18+
* @see https://github.com/phparkitect/arkitect/issues/303
19+
*/
20+
public function test_reusing_that_for_multiple_should_does_not_produce_duplicate_violations(): void
21+
{
22+
// Create a class that violates both rules
23+
$classDescription = ClassDescription::getBuilder('App\Rector\MyClass', 'src/Rector/MyClass.php')
24+
->build();
25+
26+
// Reuse the that() result for multiple should() calls - this is the pattern from issue #303
27+
$rectors = Rule::allClasses()
28+
->that(new ResideInOneOfTheseNamespaces('App\Rector'));
29+
30+
$rule1 = $rectors
31+
->should(new HaveNameMatching('*Rector'))
32+
->because('rector classes should have Rector suffix');
33+
34+
$rule2 = $rectors
35+
->should(new Extend('Rector\Core\Rector\AbstractRector'))
36+
->because('rector classes should extend AbstractRector');
37+
38+
// Check violations for rule1 - should only check HaveNameMatching
39+
$violations1 = new Violations();
40+
$rule1->check($classDescription, $violations1);
41+
42+
// Check violations for rule2 - should only check Extend
43+
$violations2 = new Violations();
44+
$rule2->check($classDescription, $violations2);
45+
46+
// Each rule should produce exactly 1 violation, not 2
47+
// rule1 should only report the name matching violation
48+
// rule2 should only report the extend violation
49+
self::assertCount(1, $violations1, 'Rule 1 should have exactly 1 violation (name matching)');
50+
self::assertCount(1, $violations2, 'Rule 2 should have exactly 1 violation (extend)');
51+
52+
// Verify the violation messages are for the correct constraints
53+
self::assertStringContainsString(
54+
'should have a name that matches',
55+
$violations1->get(0)->getError(),
56+
'Rule 1 violation should be about name matching'
57+
);
58+
self::assertStringContainsString(
59+
'should extend',
60+
$violations2->get(0)->getError(),
61+
'Rule 2 violation should be about extending'
62+
);
63+
}
64+
65+
/**
66+
* @see https://github.com/phparkitect/arkitect/issues/303
67+
*/
68+
public function test_reusing_that_for_multiple_and_that_does_not_share_state(): void
69+
{
70+
// Create a class that matches the base namespace
71+
$classDescription = ClassDescription::getBuilder('App\Service\MyService', 'src/Service/MyService.php')
72+
->setFinal(true)
73+
->build();
74+
75+
// Reuse the that() result for multiple andThat() calls
76+
$services = Rule::allClasses()
77+
->that(new ResideInOneOfTheseNamespaces('App\Service'));
78+
79+
$rule1 = $services
80+
->andThat(new ResideInOneOfTheseNamespaces('App\Service\Internal'))
81+
->should(new HaveNameMatching('*Service'))
82+
->because('internal services should have Service suffix');
83+
84+
$rule2 = $services
85+
->andThat(new ResideInOneOfTheseNamespaces('App\Service\External'))
86+
->should(new HaveNameMatching('*Client'))
87+
->because('external services should have Client suffix');
88+
89+
// The class is in App\Service but not in App\Service\Internal or App\Service\External
90+
// So neither rule should match and produce violations
91+
$violations1 = new Violations();
92+
$rule1->check($classDescription, $violations1);
93+
94+
$violations2 = new Violations();
95+
$rule2->check($classDescription, $violations2);
96+
97+
// Neither rule should produce violations because the class doesn't match andThat conditions
98+
self::assertCount(0, $violations1, 'Rule 1 should have no violations (class not in Internal namespace)');
99+
self::assertCount(0, $violations2, 'Rule 2 should have no violations (class not in External namespace)');
100+
}
101+
}

0 commit comments

Comments
 (0)