Skip to content

Commit 49ad5fc

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 49ad5fc

File tree

3 files changed

+87
-4
lines changed

3 files changed

+87
-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
@@ -30,6 +30,12 @@ public function __construct()
3030
$this->runOnlyThis = false;
3131
}
3232

33+
public function __clone()
34+
{
35+
$this->thats = clone $this->thats;
36+
$this->shoulds = clone $this->shoulds;
37+
}
38+
3339
public function addThat(Expression $that): self
3440
{
3541
$this->thats->add($that);
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
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+
/**
16+
* @see https://github.com/phparkitect/arkitect/issues/303
17+
*/
18+
class RuleBuilderTest extends TestCase
19+
{
20+
public function test_reusing_that_for_multiple_should_produces_independent_rules(): void
21+
{
22+
$classViolatingBothRules = ClassDescription::getBuilder('App\Rector\MyClass', 'src/Rector/MyClass.php')
23+
->build();
24+
25+
$rectors = Rule::allClasses()
26+
->that(new ResideInOneOfTheseNamespaces('App\Rector'));
27+
28+
$nameMatchingRule = $rectors
29+
->should(new HaveNameMatching('*Rector'))
30+
->because('rector classes should have Rector suffix');
31+
32+
$extendsRule = $rectors
33+
->should(new Extend('Rector\Core\Rector\AbstractRector'))
34+
->because('rector classes should extend AbstractRector');
35+
36+
$nameMatchingViolations = new Violations();
37+
$nameMatchingRule->check($classViolatingBothRules, $nameMatchingViolations);
38+
39+
$extendsViolations = new Violations();
40+
$extendsRule->check($classViolatingBothRules, $extendsViolations);
41+
42+
self::assertCount(1, $nameMatchingViolations);
43+
self::assertCount(1, $extendsViolations);
44+
self::assertStringContainsString('should have a name that matches', $nameMatchingViolations->get(0)->getError());
45+
self::assertStringContainsString('should extend', $extendsViolations->get(0)->getError());
46+
}
47+
48+
public function test_reusing_that_for_multiple_and_that_produces_independent_rules(): void
49+
{
50+
$classInBaseNamespaceOnly = ClassDescription::getBuilder('App\Service\MyService', 'src/Service/MyService.php')
51+
->build();
52+
53+
$services = Rule::allClasses()
54+
->that(new ResideInOneOfTheseNamespaces('App\Service'));
55+
56+
$internalRule = $services
57+
->andThat(new ResideInOneOfTheseNamespaces('App\Service\Internal'))
58+
->should(new HaveNameMatching('*Service'))
59+
->because('internal services should have Service suffix');
60+
61+
$externalRule = $services
62+
->andThat(new ResideInOneOfTheseNamespaces('App\Service\External'))
63+
->should(new HaveNameMatching('*Client'))
64+
->because('external services should have Client suffix');
65+
66+
$internalViolations = new Violations();
67+
$internalRule->check($classInBaseNamespaceOnly, $internalViolations);
68+
69+
$externalViolations = new Violations();
70+
$externalRule->check($classInBaseNamespaceOnly, $externalViolations);
71+
72+
self::assertCount(0, $internalViolations);
73+
self::assertCount(0, $externalViolations);
74+
}
75+
}

0 commit comments

Comments
 (0)