refactor(rules): migrate deprecated isSubclassOf(string) → isSubclassOfClass (queue #112) - #56
Conversation
…OfClass (queue #112) isSubclassOf(string) is @deprecated in PHPStan 2.2.2 and removed in 3.x — a latent break for every consumer's static analysis. Resolve the configured/known base FQCN via ReflectionProvider->hasClass()/getClass() and call isSubclassOfClass(ClassReflection), preserving the unknown-base-class no-op that keeps non-Laravel consumers unaffected. Pinned by a base-class-absent fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017kMpRQSwwg8zM5kYzbNArQ
Goosterhof
left a comment
There was a problem hiding this comment.
Verdict: clean. Verified independently, not just read.
This is a textbook deprecation migration and I went looking for the classic failure modes of a string → ClassReflection API swap — none present.
What I checked, not just what the PR claims:
-
Byte-for-byte equivalence claim — verified against the actual vendored PHPStan source, not taken on faith. Extracted
vendor/phpstan/phpstan/phpstan.pharand readClassReflection::isSubclassOf()directly:public function isSubclassOf(string $className): bool { if (!$this->reflectionProvider->hasClass($className)) { return false; } return $this->isSubclassOfClass($this->reflectionProvider->getClass($className)); }
This matches the PR body's claimed body exactly, and all three migrated call sites (
EnforceAuditModelProtectionsRule,EnforceFormRequestToDtoRule,EnforceResourceDataValidatorOptInRule) reproduce thishasClass-guard-then-isSubclassOfClassshape 1:1. No signature-mismatch risk (no raw string passed where aClassReflectionis required), no inverted subclass direction, no dropped existence guard. -
Completeness —
grep -rn -- "isSubclassOf(" src/(excludingisSubclassOfClass) returns zero live call sites; the only remaining string hits are docblock prose. All 3 sites from the queue-#112 scope are migrated, plus the audit-rule's fixed-class site the queue didn't explicitly name — correctly swept in the same pass rather than left half-migrated. -
Ran the suite myself, not trusting the PR's reported numbers:
composer test→ 176/176 passed (256 assertions),composer phpstan→[OK] No errors, deprecation warning gone. Matches the PR body's claimed counts exactly. -
DI wiring —
ReflectionProvideris constructor-injected in all three rules with noextension.neonargument added, andextension.neonconfirms noarguments:block for any of the three rules — correct, since PHPStan/Nette autowiresReflectionProvideras a container service.testRuleResolvesFromExtensionNeonAndFires(which pulls each rule out of the real container) is in the green 176, which is the actual proof the autowiring works, not just a claim. -
No silent enforcement weakening — each rule gained a new no-op test (base-class-absent / not-a-subclass) rather than a fixture change on an existing positive-case test, so I don't have to take "behavior unchanged" on faith for the tests that already existed; they're literally untouched except for the new constructor arg being threaded through
ruleOverridecalls.
Dashboard note (not a finding): check (8.4) / check (8.5) were still pending on GitHub Actions at review time — both are required contexts on main's branch protection. Worth a green-check confirmation before merge; my own local run of the same commands (composer test + composer phpstan) was clean, so I don't expect a surprise, but I haven't watched the Action itself complete.
No blockers, no majors, no minors. This is the correct, complete, test-pinned form of this migration — approve-quality work, posting as COMMENT only because GitHub blocks self-approval on your own PR.
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
There was a problem hiding this comment.
Clean at 14715d7 — confirm the-general. Grep: zero live isSubclassOf( call sites, only isSubclassOfClass in src/Rules/*.php (stray hits are comments/test names). Each rule DIs ReflectionProvider, guards hasClass() before isSubclassOfClass(getClass(...)) — reproduces deprecated form's no-op, pinned by 3 base-class-absent tests. Static review only here (no vendor in worktree); the-general already ran composer test 176/176 + phpstan clean. No gated findings.
What
ClassReflection::isSubclassOf(string)is@deprecatedin the vendored PHPStan (2.2+) and removed in PHPStan 3 — a latent break for every consumer territory's static analysis the day this package targets 3.x. Migrated all 3 string-form call sites toisSubclassOfClass(ClassReflection):EnforceFormRequestToDtoRule(formRequestBaseClass)EnforceResourceDataValidatorOptInRule(resourceDataBaseClass)EnforceAuditModelProtectionsRule(Model::class— the queue named only the first two; the WR-0195 audit rule's fixed-class site is the third, swept in the same pass)Each rule now injects
ReflectionProviderand resolves its base FQCN viahasClass()/getClass()before callingisSubclassOfClass().The load-bearing no-op is preserved exactly
The deprecated string form's own body is
if (!hasClass($fqcn)) return false; return isSubclassOfClass(getClass($fqcn));(confirmed by reading the vendored PHPStan phar). The migration is a 1:1 inline of that, so the "consumers analysing non-Laravel trees are unaffected" guarantee — a tree with no configured base class never fires the rule — is reproduced byte-for-byte. Pinned by 3 new base-class-absent no-op tests.Verification
composer test176/176 (256 assertions, +3 new) ·composer phpstan[OK] No errors(deprecation gone) · Pint clean · grep proof: every->isSubclassOf(is now->isSubclassOfClass((remaining hits are docblock prose).extension.neonchange needed — Nette autowiresReflectionProvider; existing container-resolution tests prove wiring. First rule here to inject a PHPStan service (standard PHPStan-core pattern) — worth a review glance.[Unreleased] → ### Changed(PATCH; pairs with the pending WR-0438 release cut — not cut here).🤖 General-dispatched Armorer (warpath).