From 21f378c8ac542d2f39418a6c129a62a5b5bddcc8 Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Sat, 29 Aug 2026 12:13:38 +0200 Subject: [PATCH] [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 --dry-run --clear-cache --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 RectorConfig::singleton() goes past rule() and is no longer active. Two public builder entry points lead there: registerService(), which asserts nothing, and registerDecoratingNodeVisitor(), whose Assert::isAOf(..., NodeVisitor::class) a rule satisfies because RectorInterface extends NodeVisitor. ssch/typo3-rector does exactly this, and it argues for the change. Its config/v13/typo3-130.php registers two rules through singleton() with the comment "The following rules exist only to render the diff for the documentation. The actual logic is in THIS file!". Measured against 2.6.4, loading that set yields 28 active rules against 26 registered -- the two extra are those documentation-only ones, which therefore run against user code today. Before 2.6.4 they did not, because singleton() set no tag. This restores what the comment says was intended. 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 just above, 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 --- src/Config/RectorConfig.php | 32 +++++++++++++++++ .../PHPUnit/AbstractRectorTestCase.php | 12 +++++-- tests/Config/RectorConfigTest.php | 20 +++++++++++ tests/Config/Source/DependencyOnlyRector.php | 35 ++++++++++++++++++ tests/Config/Source/RegisteringRector.php | 36 +++++++++++++++++++ 5 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 tests/Config/Source/DependencyOnlyRector.php create mode 100644 tests/Config/Source/RegisteringRector.php diff --git a/src/Config/RectorConfig.php b/src/Config/RectorConfig.php index ac3ed8bcd80..161d7fa726c 100644 --- a/src/Config/RectorConfig.php +++ b/src/Config/RectorConfig.php @@ -568,6 +568,38 @@ public function forgetByContract(string $contract): void } } + /** + * The container caches every instance it builds, so a rule that another service takes as a + * constructor dependency is cached like any other object. Returned unfiltered it would count + * as an active rule although no config ever registered it — and this is where the active set + * comes from on both paths: the node traverser is autowired from an "@param RectorInterface[]" + * parameter, and AbstractRectorTestCase reads the same list. + * + * The narrowing keys off the requested contract, so a wider one still sees every instance - + * findByContract(NodeVisitor::class) includes unregistered rules, since RectorInterface + * extends NodeVisitor. + * + * @template TType as object + * + * @param class-string $contractClass + * @return list + */ + public function findByContract(string $contractClass): array + { + $instances = parent::findByContract($contractClass); + + if (! is_a($contractClass, RectorInterface::class, true)) { + return $instances; + } + + return array_values( + array_filter( + $instances, + fn (object $instance): bool => isset($this->registeredRectorClasses[$instance::class]) + ) + ); + } + public function reportingRealPath(bool $absolute = true): void { SimpleParameterProvider::setParameter(Option::ABSOLUTE_FILE_PATH, $absolute); diff --git a/src/Testing/PHPUnit/AbstractRectorTestCase.php b/src/Testing/PHPUnit/AbstractRectorTestCase.php index f90461c745a..5c5eef5b464 100644 --- a/src/Testing/PHPUnit/AbstractRectorTestCase.php +++ b/src/Testing/PHPUnit/AbstractRectorTestCase.php @@ -7,6 +7,7 @@ use Iterator; use Nette\Utils\FileSystem; use Nette\Utils\Strings; +use PhpParser\NodeVisitor; use PHPUnit\Framework\ExpectationFailedException; use Rector\Application\ApplicationFileProcessor; use Rector\Autoloading\AdditionalAutoloader; @@ -100,8 +101,15 @@ protected function setUp(): void $this->forgetRectorsRules(); $rectorConfig->resetRuleConfigurations(); - // this has to be always empty, so we can add new rules with their configuration - $this->assertEmpty($rectorConfig->findByContract(RectorInterface::class)); + // this has to be always empty, so we can add new rules with their configuration. + // asked through the wider contract on purpose: the rule contract now reports only + // registered rules, and the registered list was just cleared, so it would answer + // "empty" without looking at what the forget above actually left behind + $leftOverRectors = array_filter( + $rectorConfig->findByContract(NodeVisitor::class), + static fn (NodeVisitor $nodeVisitor): bool => $nodeVisitor instanceof RectorInterface + ); + $this->assertSame([], $leftOverRectors); $this->bootFromConfigFiles([$configFile]); diff --git a/tests/Config/RectorConfigTest.php b/tests/Config/RectorConfigTest.php index 0f054548c7c..810aff16fa6 100644 --- a/tests/Config/RectorConfigTest.php +++ b/tests/Config/RectorConfigTest.php @@ -7,6 +7,7 @@ use Rector\Config\RectorConfig; use Rector\Configuration\Option; use Rector\Configuration\Parameter\SimpleParameterProvider; +use Rector\Contract\Rector\RectorInterface; use Rector\Renaming\Rector\MethodCall\RenameMethodRector; use Rector\Renaming\Rector\Name\RenameClassRector; use Rector\Renaming\Rector\PropertyFetch\RenamePropertyRector; @@ -14,6 +15,8 @@ use Rector\Renaming\ValueObject\RenameProperty; use Rector\Symfony\Set\SymfonySetList; use Rector\Testing\PHPUnit\AbstractLazyTestCase; +use Rector\Tests\Config\Source\DependencyOnlyRector; +use Rector\Tests\Config\Source\RegisteringRector; use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromReturnNewRector; final class RectorConfigTest extends AbstractLazyTestCase @@ -89,4 +92,21 @@ public function testRuleWithConfigurationComposerVersionBoundOnMissingPackage(): $registeredRectorRules = SimpleParameterProvider::provideArrayParameter(Option::REGISTERED_RECTOR_RULES); $this->assertNotContains(RenamePropertyRector::class, $registeredRectorRules); } + + public function testRuleTakenOnlyAsDependencyIsNotActive(): void + { + $rectorConfig = $this->getContainer(); + + $rectorConfig->rule(RegisteringRector::class); + + // building the registered rule also builds and caches its constructor dependency, + // which is a rule too - but one no config asked for + $activeRectorClasses = array_map( + static fn (object $rector): string => $rector::class, + $rectorConfig->findByContract(RectorInterface::class) + ); + + $this->assertContains(RegisteringRector::class, $activeRectorClasses); + $this->assertNotContains(DependencyOnlyRector::class, $activeRectorClasses); + } } diff --git a/tests/Config/Source/DependencyOnlyRector.php b/tests/Config/Source/DependencyOnlyRector.php new file mode 100644 index 00000000000..1ab97353c88 --- /dev/null +++ b/tests/Config/Source/DependencyOnlyRector.php @@ -0,0 +1,35 @@ +> + */ + public function getNodeTypes(): array + { + return [String_::class]; + } + + public function refactor(Node $node): ?Node + { + return null; + } +} diff --git a/tests/Config/Source/RegisteringRector.php b/tests/Config/Source/RegisteringRector.php new file mode 100644 index 00000000000..64e93579ec8 --- /dev/null +++ b/tests/Config/Source/RegisteringRector.php @@ -0,0 +1,36 @@ +> + */ + public function getNodeTypes(): array + { + return [String_::class]; + } + + public function refactor(Node $node): ?Node + { + return $this->dependencyOnlyRector->refactor($node); + } +}