Repository navigation
Conversation
Pass 1 of the question "can a rule carry a bounded composer constraint (>=8.5.0 <9.0.0) instead of the open-ended >=8.5.0 PR #419 gives it". Change records first, as the primary source: rule -> CR (scraped from the rule class and from the config line registering it, since the generic configurable rules carry the CR beside each configuration entry) -> change_record.change_to_branch for the declared target -> change_record_symbol role='from' -> core_symbol.removal_in for the version the old API actually went away in. Of 154 rules, 121 cite an indexed change record, 76 have a removal version and 39 of those are code-observed rather than merely scheduled. D8 rules resolve to removal in 9.0.0, D9 to 10.0.0, D10 to 11.0.0, D11 to 12.0.0 or 13.0.0 -- all observed at or below 11.0.0, almost all scheduled above it. The records are not reliable on their own, which pass 2 has to check against the tree. Six are wrong or unreadable: four give a removal version equal to the deprecation version, 3349345 removes inside a major (10.2 -> 10.5.0), 3461934 gives one minor of grace (11.0 -> 11.1.0), and 3513877 has a typo in its own branch field. So an upper bound must come from removal_in, never from arithmetic on the deprecation version. For the D8/D9 bounding case specifically: 15 of 43 rules can be bounded from this evidence today, all on observed removals; 27 have no removal version (26 cite no CR at all, mostly the 9.1 AssertLegacyTrait block); and FunctionToFirstArgMethodRector spans three minors so it can never take one class-level bound.
Change records first, then the code, because the records are not reliable on their own. Pass 2 went at the symbols directly: one row per configuration entry rather than per rule (234 entries, since a configurable rule carries one deprecation per value object), reading the deprecated symbol out of the value object's constructor argument instead of out of prose, then resolving each against core_symbol.removal_in / removal_kind, and falling back to repos/drupal-core for what the catalog does not carry. 228 of the 234 entries resolve to a core symbol. The 6 that do not are not core symbols: three name PHPUnit's getMock(), three name Symfony CMF RouteObjectInterface constants (core decoupled from CMF in 9.1.0, 37893880799). Coverage went from 76 rules with a removal version and 39 code-observed, to 100 and 48. Drupal 8 is 16/18 and Drupal 9 23/25, almost entirely observed. For the bounding question: 38 of 43 D8/D9 rules can now be bounded, every one on an observed removal, up from 15 after pass 1. The 9.1 AssertLegacyTrait block -- 12 rules citing no change record at all -- resolved in one lookup, all removed in 10.0.0. The headline correction no change record would have given us: RequestTimeConstRector sits in the 8.3 set, but REQUEST_TIME was not removed until 11.0.0. A bound computed as "deprecation major + 1" would have given it <9.0.0 and silently disabled the rule for every D9 and D10 user. With 3349345 (10.2 -> 10.5.0) and 3461934 (11.0 -> 11.1.0), that is three proofs that an upper bound must come from removal_in, never from arithmetic. Aggregating entries per rule also settles where a bound belongs: FunctionToServiceRector's 97 entries span removals from 9.0.0 to 13.0.0, as do FunctionToStaticRector's and ConstantToClassConstantRector's. No class-level constraint can be right for them; their bounds belong per configuration entry. Stopping for Drupal 11: 42 of its rules still have no removal version, and of the 51 that do only 8 are observed -- the rest are scheduled for 12.0.0 or 13.0.0, and the evidence above says not to bound on a promise.
One report covering the whole review: what the PR does, what was resolved and fixed, what stays open, and the evidence behind each so it does not have to be gathered again. Resolved: the bundled Symfony/PHPUnit/Twig sets had to go and did (e05e5f9, pushed); the global ComposerPackageConstraintFilter turned out not to break the look-ahead workflow, because a rule targets the version its deprecation lands in -- one or two majors before removal -- and contrib is analysed inside a site, so the library-floor path never fires. Open: whether D8/D9 rules stay, with the coverage, maintenance and ecosystem numbers behind it, plus the accidental regression that composer-based.php re-enables all 43 of them; and bounded constraints, with the verified mechanism, the three rules any implementation must follow, and the escape-hatch catch. No code changes.
Core states each deprecation in a machine-readable form that cites the change record that documents it: ModuleHandler::loadAllIncludes() is deprecated in drupal:11.3.0 and is removed from drupal:13.0.0. ... See https://www.drupal.org/node/3536432 There are 1,651 such strings in core, 912 carrying a node link, covering 217 distinct change records. Every rule already has its change record nids, so rule -> nid -> core's promise is a join rather than research. It resolved 17 of the 54 rules that still had no upper bound, each with a file:line citation, at no cost. Coverage: 100 -> 117 of 154 rules with a removal version. The new ones land as scheduled rather than observed -- Drupal 12 does not exist yet, so for an 11.x deprecation there is nothing to observe, only core's stated intention.
…t core Seven Haiku agents took the 27 rules the change-record join could not reach, four rules each, each told to read the rule and its test fixtures, find the deprecated symbol in core, and return the notice copied verbatim with a file and line. A script then re-read every citation and string-matched it. 18 of 21 bounded claims verified. 3 were fabricated: a cited file that does not exist, an evidence_text absent from the line given, and a version not present in the quoted text. Requiring a verbatim citation is what made those visible -- the conclusions of two of the three were actually right, so trusting the answers rather than the evidence would have looked fine and been unreproducible. The three rejects and two not_found results were resolved by hand from core's git history: ReplaceRequestTimeConstantRector 11.0.0 2f44d215e86 (#3442766) MigrateSqlGetMigrationPluginManagerRector 11.0.0 166f3a39e46 (#3439369) RemoveTwigNodeTransTagArgumentRector 11.1.0 cec21638d09 (#3477374) RenameStopProceduralHookScanRector 11.2.0 308ad151024 (#3495943) ReplaceLocaleTranslationPathConfigRector 13.0.0 locale.schema.yml:45 Two lessons for a repeat run. The working tree only shows what is still there, so a pre-11.0 removal needs git log -S plus git tag --contains, not grep -- all three not_found results had that one cause. And where a rule spans several notices with different removals, take the latest; bounding on the earliest silently disables the rule while the deprecation is still live. Coverage: 117 -> 139 of 154 rules with a removal version, 52 code-observed. The remaining 15 are documented as rules that should not get a drupal/core upper bound at all -- 5 are PHPUnit-bound (4 of which PR #419 gives a drupal/core constraint, a defect worth its own report), 3 are infrastructure, 5 are behaviour changes with nothing removed, 1 has no stated removal, and 1 needs a human look.
You were right that "nothing there" was too quick. Checking pass 4's leftovers against the symbol catalog, core's git history and the change records on drupal.org turned two into real bounds and corrected the package on a third. FromUriRector was a misidentified symbol, not a missing one. Pass 2 looked up Url::fromUri, found it undeprecated, and filed the rule as a behaviour change. The rule actually matches Url::fromUri(file_create_url($uri)), so its deprecated symbol is file_create_url() -- removed 10.0.0, observed. It has no fixtures, so fixture-based identification could not reach it either. FunctionalTestDefaultThemePropertyRector does have a removal: deprecated 8.8.0 (CR 3083055), BC layer removed in 9.0.0 (305c401f90e, #3110874), and core now throws an exception citing that very change record. AddSymfonyConstraintValidatorTypeDeclarationsRector tracks Symfony 8.0's ConstraintValidatorInterface, per its own @see -- a sixth wrong-package rule, five of which PR #419 gives a drupal/core constraint. The other 13 are each chased to a citation rather than left open: - \Drupal::classResolver is added_in 8.3.x with status [] -- not deprecated at all, which settles ViewsConfigUpdaterClassResolverToServiceRector. - $modules became protected static in 8.3.0 (b33af7a964e, #2814035). - state_cache: core cites CR 3177901, not the two the rule cites; "In Drupal 11+, settings 'state_cache' is removed and permanently turned on" is the rule's lower bound, and Settings.php still flags it with no deadline. - ShouldCallParentMethodsRector is not a deprecation rule at all -- it is test hygiene registered in the 9.0 and 10.0 deprecation sets. - getMock removed in PHPUnit 6; getName renamed to name in PHPUnit 10; annotations removed in PHPUnit 12. Also: Classy was removed in 10.1.0, not 10.0.0 -- one more trap for anyone computing an upper bound by arithmetic. Coverage: 139 -> 141 of 154, 54 code-observed. Also replaced the stale "what is left" section, which still quoted pass-2 numbers.
An invariant check — Drupal removes deprecated API only at a major boundary,
so removal must be X.0.0 in a strictly later major than the deprecation —
flagged 11 of 134 matrix rows as impossible.
Seven had captured the deprecation version in the removal column, three of
them a version earlier than the set the rule ships in. All seven came through
the change-record path, none through git archaeology. Re-read from core and
corrected, with file:line citations.
The remaining four are genuine: two are real 11.0.0 removals whose sets are
keyed on removal rather than deprecation, and two are not deprecation removals
at all (an upstream Twig constructor change, and the rename of an attribute
introduced one minor earlier during the OOP-hooks work).
So no rule bounds on a mid-major removal, and the upper bound is always a
clean <{major+1}.0.0 with no special case. Documents the check for CI.
Adds "Deprecated in" beside "Removed in", so each row carries both ends of the bound. Derived, never hand-typed, from four sources in descending trust: notices read from core by hand, the verified pass-4 output, the core-notice evidence in bounds.json, and the change-record join taking the earliest deprecation across a rule's change records. 93 of 154 rows filled. The gap is structural, not random: D11 is 85/98 while D8 is 4/22 and D9 2/27, because those notices were deleted from core years ago and no route that reads an 11.x checkout can see them. Comparing the derived version against the set each rule ships in found 6 disagreements out of 93. Five are coverage gaps in the same direction -- the rule is withheld from sites that already have the deprecation, including RemoveInstallSchemaSystemSequencesRector (set 11.4, deprecated 10.2.0) and ReplaceDialogClassOptionRector (set 11.3, deprecated 10.3.0). One fires early. Also shifts the documented invariant check to the new column position and corrects a bad note from the previous commit: the matrix holds 154 rows and matches the coverage table; the earlier 153 came from a stale line range.
The composer-based set currently accepts only an open-ended `>=X.Y.Z`, so a rule stays active forever once the deprecation lands. This allows the closed form `>=11.3.0 <13.0.0` as well, which is what lets a rule switch itself off once it can no longer apply. The upper bound must be `<N.0.0`: Drupal removes deprecated API only on a major boundary, so any other shape is a mistake rather than a preference. The rule also compares the two majors and rejects an upper bound that is not later than the lower one -- an invariant that would have caught seven removal versions in the version matrix that had silently captured the deprecation version instead. Both checks live in one shared validator, so a constraint declared on a rule class and one declared in the set are held to the same shape. The open-ended form is still accepted while the set is being bounded; it can be rejected once every constraint carries a span.
The six generic rules -- FunctionToService, FunctionToStatic, MethodToMethodWithCheck, ConstantToClassConstant, ClassConstantToClassConstant and FunctionCallRemoval -- are each registered many times, once per deprecation version, and together hold 168 configuration entries. Every group stated only a lower bound, so a Drupal 8 entry still ran against Drupal 11. Each group now closes at the major after the latest removal among its own entries. Where a group's entries span two removals, the later one wins: the rule must stay alive while any of its targets is still live. How the 168 entries were resolved: - 119 by reading the deprecation notice directly above the declaration in core, scoped to the declaring file, with global functions distinguished from same-named methods by declaration column; - 49 by git archaeology, since those symbols are gone from an 11.x tree. Every archaeology result was re-verified mechanically rather than trusted: each cited commit was confirmed to contain a deletion line for the symbol, and `git tag --contains` was confirmed to make the claimed version the first release containing that commit. 30 of 30 git claims and 15 of 15 tree claims verified, with no fabrications. Two groups keep an open lower bound. One holds the Symfony CMF RouteObjectInterface constants, which were never core symbols -- core added its own interface in 9.1.0 and the rewrite target still exists, so there is nothing to bound against. The other is the remaining residue. Also widens the package check from a hard-coded `drupal/core` to a short allowlist, and accepts an upper-bound-only constraint for a package whose API predates the rule. A `drupal/core` bound must still state a lower bound. This unblocks the five PHPUnit rules and one Symfony rule that currently carry a `drupal/core` constraint that is not true of them.
Closes the upper bound on 63 more registrations, taking the set from 44 to 107 bounded of 124. Each inherits its rule's removal version from the version matrix and retires one major later. The 17 that stay open are the rules with no removal version to bind to, plus the four whose constraint names drupal/core but whose deprecation belongs to PHPUnit or Symfony; those need repackaging first. Class-level constraints in src/ are deliberately NOT bounded here -- see the next commit message for why they cannot be, yet.
Widening the PHPStan package check to allow phpunit/phpunit and symfony/validator let six rules name the package they actually track, but it contradicts the rest of this PR. The bundled PHPUnit, Symfony and Twig sets were removed on the grounds that those deprecations are not ours to track, and the README now sends users to ->withComposerBased(phpunit: true, ...). An allowlist re-adopts that responsibility one layer down and commits drupal-rector to following PHPUnit's release cycle. It would also entrench a duplicate: rector-phpunit already ships its own GetMockRector doing the same rewrite, in its own composer-based set, so a user following our README gets it twice. The upper-bound-only form goes with it, since it existed only to express "phpunit/phpunit <6.0.0". A constraint is once again required to state the drupal/core version the deprecation was introduced in. Kept: the closed-range form, the major-ordering check, and the shared validator behind both PHPStan rules. The six wrong-package rules keep the bound #419 gave them -- untrue of them, now documented in the matrix along with why the allowlist is the wrong repair.
Rule tests already stand in a fake drupal/core version, because this package does not require drupal/core and every rule bonded through ComposerPackageConstraintInterface would otherwise be filtered out. That stub pinned one global version, 12.0.0, chosen as "the highest any rule is bonded to, so every rule is active". That holds only while every constraint is open-ended. Once a rule states a closed range no single version can activate them all: a rule for an API removed in Drupal 10 caps at <11.0.0, while one deprecated in 11.4 only starts at >=11.4.0. The two are mutually exclusive, so a global pin silently switches off whichever half it does not match -- the rule stops transforming and its own fixture test fails. Each tests/src/Drupal<major> namespace now gets its own pin, high within that major, so every rule in the set is active. Tests outside those namespaces keep the existing fallback. No change to the 143 test classes or the 136 rule configs; the test case derives the file from its own class name. This commit is inert on its own -- verified at 700 tests with no failures before any rule was bounded. One thing it gives up: a rule test now asserts the transformation but no longer exercises activation, since the pin is chosen to satisfy the rule. Activation deserves its own test rather than being implicitly covered by every fixture.
Every rule that declares a ComposerPackageConstraint stated only the version its deprecation was introduced in, so it stayed active forever: a Drupal 8 rule still ran against a Drupal 11 codebase. Each now retires one major after the API was removed. Removal versions come from the version matrix, which reads them from core's own deprecation notices or, where the symbol is already gone from an 11.x tree, from the commit that deleted it plus the first release tag containing that commit. The bound is the major *after* removal, never the removal itself. Rector reads source, not runtime: a codebase can still contain a removed call in dead code or an unported module, and those are exactly the calls worth rewriting. Bounding at the removal version would switch the rule off at the moment the code became broken. Five constraints stay open. Three have no removal version in core to bind to (ProtectedStaticModulesPropertyRector, ShouldCallParentMethodsRector, RemoveStateCacheSettingRector). The other two are part of the six rules whose constraint names drupal/core but whose deprecation belongs to PHPUnit or Symfony; they need the right package before a bound means anything. Depends on the preceding commit: without the per-major test pin these bounds fail 35 of the project's own rule tests, because the shared stub reported a core newer than a D8/D9 rule's upper bound.
RenameClassRector and RenameStaticMethodRector are Rector's own classes, but the registrations, the deprecations and the change records are all Drupal's, so they bound like any other rule. Six of the seven now close: L505 Bytes::toInt >=9.1.0 <11.0.0 removed 10.0.0 L792 MatchingRouteNotFoundException >=11.1.0 <13.0.0 removed 12.0.0 L875 AliasWhitelist >=11.1.0 <13.0.0 removed 12.0.0 L1162 pgsql QueryFactory, migrate >=11.2.0 <13.0.0 removed 12.0.0 L1460 WorkspaceAssociation >=11.3.0 <13.0.0 removed 12.0.0 L2121 NodeViewController >=11.4.0 <14.0.0 removed 13.0.0 Four of those removal versions were stated in the surrounding code comments rather than read from core. All four were re-checked against core's own notices; three held, one did not. The one that did not is the block_content Access group, and the comment was wrong three ways. Drupal\block_content\Access\* was @internal and was MOVED to Drupal\Core\Access\* in 2d8018be0a8, first released in 11.2.0 -- not deprecated in 11.3.0 and removed in 12.0.0. The commit adds no class_alias and no @deprecated marker, so no deprecation cycle ever existed, which is how it landed mid-major without breaching policy. That makes it a genuine open-ended case, but for the opposite reason to the one recorded: code on the old path breaks from 11.2.0 and stays broken, so the rewrite keeps its value indefinitely. Its floor was also a minor too late, withholding the rule from 11.2 sites that already need it, so it moves >=11.3.0 -> >=11.2.0. The comment and its stale TODO are corrected. Set registrations now 113 of 124 bounded.
…noyances. Officle Drupal 9 support ended 3 years ago
bbrala
marked this pull request as ready for review
October 1, 2026 11:54
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
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.
Stacked on top of palantirnet#419 so the diff shows only what this adds. Draft while the data work lands.
Why
Every constraint in palantirnet#419 is open-ended
>=X.Y.Z, so a rule stays active forever once its deprecation lands. A D8 rule still runs on a D11 site. Closing the range is what lets a rule retire itself.What is here so far
PHPStan — the bound rules now accept the closed form
>=11.3.0 <13.0.0alongside the open one, and enforce two invariants:<N.0.0, because Drupal removes deprecated API only on a major boundary;Nmust be a later major than the lower bound's.Both checks live in one shared validator, so a constraint on a rule class and one in the set are held to the same shape. The open form is still accepted while the set is being bounded, so nothing breaks today.
docs/rector-rule-version-matrix.md— the evidence for every bound: each rule's deprecation and removal version, where the number came from, and how confident it is. 154 rules, 141 with a removal version, 54 verified against core's own source rather than a change record.The bound policy
>={deprecated_in} <{major(removal) + 1}.0.0The upper bound is one major past removal, not the removal itself. Rector reads source, not runtime: a codebase can still contain a removed call in dead code or an unported module, and those are exactly the calls worth rewriting. Bounding at the removal version switches the rule off at the moment the code becomes broken.
Two findings worth flagging
Seven removal versions in the matrix were wrong — they had captured the deprecation version, three of them a version earlier than the set the rule ships in. All seven came through the change-record path, none through git archaeology. Corrected against core with file:line citations. The new major-ordering check would have caught every one.
Six rules carry a
drupal/corebound but do not target core — five are PHPUnit rules and one tracks Symfony 8.0. The PHPStan rule currently hard-codesdrupal/coreas the only allowed package, so it enforces the wrong answer. Needs a small package allowlist.Still to come