[Config] Only registered rules count as active rules - #8402
Closed
CybotTM wants to merge 1 commit into
Closed
Conversation
CybotTM
force-pushed
the
fix/testcase-active-rules-from-registration
branch
from
August 29, 2026 10:32
f9f4256 to
35a32fa
Compare
CybotTM
changed the title
CybotTM
force-pushed
the
fix/testcase-active-rules-from-registration
branch
4 times, most recently
from
August 29, 2026 12:51
eb913d8 to
78d0943
Compare
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 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 <info@sebastianmendel.de>
CybotTM
force-pushed
the
fix/testcase-active-rules-from-registration
branch
from
August 29, 2026 13:00
78d0943 to
21f378c
Compare
CybotTM
marked this pull request as ready for review
August 29, 2026 13:21
Member
|
Tl;Dr; Not accepting AI slop here. Description way too long to read. |
Author
|
Hi @TomasVotruba,
Thanks for the feedback, why do you not ask for improvement instead of just closing?
Sorry, if it made you think this PR was drafted "with little human effort, care, or meaning" I have shortened the description for your convenience. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
findByContract()returns every instance the container cached, so a rule that another service takes as a constructor dependency counts as active although no config registered it.RectorNodeTraverser(autowired from@param RectorInterface[]) andAbstractRectorTestCaseboth read that list, so it hitsbin/rector, not just tests:Before 2.6.4 the same decision was
tagged(RectorInterface::class), filled byrule()— registration decided what was active, not existence.Fix:
RectorConfig::findByContract()intersects with$registeredRectorClasseswhen the contract is a rule contract. Other and wider contracts are untouched.Test in
tests/Config/RectorConfigTest. Found in ssch/typo3-rector, whereValidateAttributeDecoratortakesStringClassNameToClassConstantRectorin its constructor.Assisted by claude-code:claude-fable-5