Skip to content

Commit eb913d8

Browse files
committed
[Config] Only registered rules count as active rules
findByContract() returns every instance the container has cached that implements the contract. A rule that another service takes as a constructor dependency is built and cached like any other object, so it comes back as an active rule even though no config ever registered it. Five services take an autowired "@PARAM RectorInterface[]": RectorNodeTraverser, ConfigInitializer, OnlyRuleResolver, DeprecatedRulesReporter and ComposerBasedCommand. Entropy resolves an array parameter through findByContract() (Container::resolve(), ParameterTypesResolver), so this is not a test-only concern -- bin/rector applies the rule: bin/rector process <dir> --dry-run --clear-cache --config <that config> - return 'marker'; + return 'rewritten by an unregistered rule'; Applied rules: * UnregisteredMarkerRector AbstractRectorTestCase::setUp() reads the same list for refreshPhpRectors(). Narrowing in RectorConfig::findByContract() covers all of them, because they all go through it. Filtering in the test case alone would have left bin/rector as it is and made the test stricter than the product. The registered set is already tracked: rule() records into $registeredRectorClasses, and every registration path funnels through rule() -- rules(), ruleWithConfiguration(), and ruleWithConfigurationComposerVersionBound(), which returns before rule() when the package constraint is not satisfied. The filter can therefore only remove rules nobody registered; it cannot activate one the composer-package filter excluded. warmUpInstanceServices() builds every registered class of the contract first, so the filtered result is exactly the registered set, whatever the container happened to build earlier. Contracts that are not rule contracts are returned untouched, and so is a wider one: findByContract(NodeVisitor::class) still sees unregistered rules. That keeps registerDecoratingNodeVisitor() working, which registers through singleton() rather than rule(). One behaviour change reaches users. A rule class registered through RectorConfigBuilder::registerService() goes to singleton() directly, past rule(), and is no longer active. That method carries no RectorInterface assertion, so a config out there may rely on it. Before 2.6.4 it behaved the same way, because rule() set the tag and singleton() did not. The emptiness assertion in AbstractRectorTestCase now asks through NodeVisitor instead of RectorInterface. Read through the rule contract it could no longer fail: resetRuleConfigurations() empties the registered list two lines earlier, so the answer would be "empty" without looking at what forgetRectorsRules() actually left behind. The wider contract still sees a surviving rule instance, which is what the assertion is there to catch -- a stale instance is handed back by Container::make() before the afterResolving() callback runs, so the next test class registering the same rule would run it with the previous class's configuration. Before 2.6.4 this decision was made by tagged(RectorInterface::class), an explicit tag list filled by rule(). Registration decided what was active; since the container swap, existence does. This restores the former behaviour. The test goes to tests/Config/RectorConfigTest.php, at the seam the fix sits on: a rule is registered, its constructor dependency is another rule, and only the registered one comes back as active. A fixture test under tests/Issues would have gone through AbstractRectorTestCase and stayed green if the filter moved back into the test case. Found while fixing ssch/typo3-rector for 2.6.4, where ValidateAttributeDecorator takes StringClassNameToClassConstantRector in its constructor: the Extbase test resolved two active rules against one registered. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_019wx8ueHuqUidp5yybDQ2CN Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
1 parent c1c61c4 commit eb913d8

5 files changed

Lines changed: 133 additions & 2 deletions

File tree

‎src/Config/RectorConfig.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,38 @@ public function forgetByContract(string $contract): void
568568
}
569569
}
570570

571+
/**
572+
* The container caches every instance it builds, so a rule that another service takes as a
573+
* constructor dependency is cached like any other object. Returned unfiltered it would count
574+
* as an active rule although no config ever registered it — and this is where the active set
575+
* comes from on both paths: the node traverser is autowired from an "@param RectorInterface[]"
576+
* parameter, and AbstractRectorTestCase reads the same list.
577+
*
578+
* The narrowing keys off the requested contract, so a wider one still sees every instance -
579+
* findByContract(NodeVisitor::class) includes unregistered rules, since RectorInterface
580+
* extends NodeVisitor.
581+
*
582+
* @template TType as object
583+
*
584+
* @param class-string<TType> $contractClass
585+
* @return list<TType>
586+
*/
587+
public function findByContract(string $contractClass): array
588+
{
589+
$instances = parent::findByContract($contractClass);
590+
591+
if (! is_a($contractClass, RectorInterface::class, true)) {
592+
return $instances;
593+
}
594+
595+
return array_values(
596+
array_filter(
597+
$instances,
598+
fn (object $instance): bool => isset($this->registeredRectorClasses[$instance::class])
599+
)
600+
);
601+
}
602+
571603
public function reportingRealPath(bool $absolute = true): void
572604
{
573605
SimpleParameterProvider::setParameter(Option::ABSOLUTE_FILE_PATH, $absolute);

‎src/Testing/PHPUnit/AbstractRectorTestCase.php‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Iterator;
88
use Nette\Utils\FileSystem;
99
use Nette\Utils\Strings;
10+
use PhpParser\NodeVisitor;
1011
use PHPUnit\Framework\ExpectationFailedException;
1112
use Rector\Application\ApplicationFileProcessor;
1213
use Rector\Autoloading\AdditionalAutoloader;
@@ -100,8 +101,15 @@ protected function setUp(): void
100101
$this->forgetRectorsRules();
101102
$rectorConfig->resetRuleConfigurations();
102103

103-
// this has to be always empty, so we can add new rules with their configuration
104-
$this->assertEmpty($rectorConfig->findByContract(RectorInterface::class));
104+
// this has to be always empty, so we can add new rules with their configuration.
105+
// asked through the wider contract on purpose: the rule contract now reports only
106+
// registered rules, and the registered list was just cleared, so it would answer
107+
// "empty" without looking at what the forget above actually left behind
108+
$leftOverRectors = array_filter(
109+
$rectorConfig->findByContract(NodeVisitor::class),
110+
static fn (NodeVisitor $nodeVisitor): bool => $nodeVisitor instanceof RectorInterface
111+
);
112+
$this->assertSame([], $leftOverRectors);
105113

106114
$this->bootFromConfigFiles([$configFile]);
107115

‎tests/Config/RectorConfigTest.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,16 @@
77
use Rector\Config\RectorConfig;
88
use Rector\Configuration\Option;
99
use Rector\Configuration\Parameter\SimpleParameterProvider;
10+
use Rector\Contract\Rector\RectorInterface;
1011
use Rector\Renaming\Rector\MethodCall\RenameMethodRector;
1112
use Rector\Renaming\Rector\Name\RenameClassRector;
1213
use Rector\Renaming\Rector\PropertyFetch\RenamePropertyRector;
1314
use Rector\Renaming\ValueObject\MethodCallRename;
1415
use Rector\Renaming\ValueObject\RenameProperty;
1516
use Rector\Symfony\Set\SymfonySetList;
1617
use Rector\Testing\PHPUnit\AbstractLazyTestCase;
18+
use Rector\Tests\Config\Source\DependencyOnlyRector;
19+
use Rector\Tests\Config\Source\RegisteringRector;
1720
use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromReturnNewRector;
1821

1922
final class RectorConfigTest extends AbstractLazyTestCase
@@ -89,4 +92,21 @@ public function testRuleWithConfigurationComposerVersionBoundOnMissingPackage():
8992
$registeredRectorRules = SimpleParameterProvider::provideArrayParameter(Option::REGISTERED_RECTOR_RULES);
9093
$this->assertNotContains(RenamePropertyRector::class, $registeredRectorRules);
9194
}
95+
96+
public function testRuleTakenOnlyAsDependencyIsNotActive(): void
97+
{
98+
$rectorConfig = $this->getContainer();
99+
100+
$rectorConfig->rule(RegisteringRector::class);
101+
102+
// building the registered rule also builds and caches its constructor dependency,
103+
// which is a rule too - but one no config asked for
104+
$activeRectorClasses = array_map(
105+
static fn (object $rector): string => $rector::class,
106+
$rectorConfig->findByContract(RectorInterface::class)
107+
);
108+
109+
$this->assertContains(RegisteringRector::class, $activeRectorClasses);
110+
$this->assertNotContains(DependencyOnlyRector::class, $activeRectorClasses);
111+
}
92112
}
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Tests\Config\Source;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Scalar\String_;
9+
use Rector\Rector\AbstractRector;
10+
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
11+
12+
/**
13+
* Never registered. It exists only as a constructor dependency of @see RegisteringRector,
14+
* which is enough for the container to build and cache it.
15+
*/
16+
final class DependencyOnlyRector extends AbstractRector
17+
{
18+
public function getRuleDefinition(): RuleDefinition
19+
{
20+
return new RuleDefinition('', []);
21+
}
22+
23+
/**
24+
* @return array<class-string<Node>>
25+
*/
26+
public function getNodeTypes(): array
27+
{
28+
return [String_::class];
29+
}
30+
31+
public function refactor(Node $node): ?Node
32+
{
33+
return null;
34+
}
35+
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Tests\Config\Source;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Scalar\String_;
9+
use Rector\Rector\AbstractRector;
10+
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
11+
12+
final class RegisteringRector extends AbstractRector
13+
{
14+
public function __construct(
15+
private readonly DependencyOnlyRector $dependencyOnlyRector
16+
) {
17+
}
18+
19+
public function getRuleDefinition(): RuleDefinition
20+
{
21+
return new RuleDefinition('', []);
22+
}
23+
24+
/**
25+
* @return array<class-string<Node>>
26+
*/
27+
public function getNodeTypes(): array
28+
{
29+
return [String_::class];
30+
}
31+
32+
public function refactor(Node $node): ?Node
33+
{
34+
return $this->dependencyOnlyRector->refactor($node);
35+
}
36+
}

0 commit comments

Comments
 (0)