Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the Eloquent relation-method parser where self::class / static::class / parent::class in relation factory arguments were being captured as literal keywords (e.g. 'self') and then incorrectly substituted at consumer call sites, producing invalid relation generic types and downstream false positives.
Changes:
- Thread the declaring class FQCN and its immediate parent FQCN through
RelationMethodParsersoself/static/parentclass-const fetches can be resolved to real FQCNs during parsing. - Add unit coverage for
resolveClassConstFetch()keyword handling and a PHPT regression test covering multiple relation factories and the->using(self::class)chain. - Add disk-backed model fixtures required for autoload-based model registration during type tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Handlers/Eloquent/RelationMethodParser.php | Resolves self/static/parent in ClassConstFetch by threading declaring/parent FQCN context through the parser pipeline. |
| tests/Unit/Handlers/Eloquent/RelationMethodParserTest.php | Adds data-provider unit tests validating resolveClassConstFetch() behavior for self/static/parent and non-special names. |
| tests/Type/tests/Relation/Issue879SelfStaticParentClassTest.phpt | Adds a PHPT regression test covering keyword resolution across multiple relation factories and ->using(self::class). |
| tests/Application/app/Models/Animal.php | Adds an autoloadable fixture model to exercise self::class and static::class relation factory arguments. |
| tests/Application/app/Models/Comment.php | Adds an autoloadable self-referential fixture model used by the PHPT regression. |
| tests/Application/app/Models/Dog.php | Adds an autoloadable subclass fixture to exercise parent::class resolution (including named-arg through relations). |
| tests/Application/app/Models/SelfReferentialPivot.php | Adds an autoloadable pivot fixture to exercise ->using(self::class) via the firstClassStringArg path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Relation factories called with self::class / static::class / parent::class were leaking the literal keyword as TRelatedModel. PhpParser leaves these keywords unresolved (they're context-sensitive), so the parser stored the string 'self' / 'static' / 'parent' as the related-model template parameter. At the call site, Psalm then substituted self with the enclosing class, producing nonsensical "expects MergeTagsAction" errors on save() / associate(). Thread the declaring class FQCN (and, for parent::class, a pre-resolved parent FQCN) through parse → doParse → parseMethodBody → findRelationCallInExpr → extractClassStringArg / firstClassStringArg → resolveClassConstFetch. Substitute self/static to the declaring class and parent to the parent class. static::class is conservatively resolved to the declaring class — strictly better than leaking 'static' but not late-static-binding-correct (the parser cache is keyed on declaring class only). Documented in the resolveClassConstFetch docblock and pinned by a regression test so a future fix cannot drift silently.
47eabea to
a3ff5e0
Compare
Collaborator
Author
|
/benchmark |
Contributor
Benchmark Results
3 run(s), 1 warmup. Measured by hyperfine. Status: 🟢 PASS |
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.
Issue to Solve
Relation factories called with
self::class/static::class/parent::classwere leaking the literal keyword asTRelatedModel. PhpParser leaves these keywords unresolved (they're context-sensitive), so the parser stored the string'self'/'static'/'parent'as the related-model template parameter. At the call site, Psalm then substitutedselfwith the enclosing class, producing nonsensical errors likeexpects MergeTagsActionon->save()/->associate().Affects every relation factory that takes a class-string (
hasMany,hasOne,belongsTo,belongsToMany,morphMany,morphOne,morphToMany,morphedByMany,hasOneThrough,hasManyThrough) plus->using(self::class)onBelongsToMany/MorphToMany.Regression introduced in
fb49d583(#760). Before that commit, relation accessors collapsed toRel<Model, Model>and the keyword never reached the template slot.Related
Closes #879.
Solution Description
Thread the declaring class FQCN (and a pre-resolved parent FQCN for
parent::class) through the parser pipeline (parse->doParse->parseMethodBody->findRelationCallInExpr->extractClassStringArg/firstClassStringArg->resolveClassConstFetch). InresolveClassConstFetch, gate onPhpParser\Node\Name::isSpecialClassName()and substitute:self::class-> declaring classstatic::class-> declaring class (conservative; the parser cache is keyed on declaring class only, so late-static-binding-correct resolution is out of scope; documented in the docblock and pinned by a regression test)parent::class-> parent class FQCN (ornullwhen no parent, matching the dynamic-arg path so the upstream handler defers)The parent FQCN is resolved once in
doParsevia$codebase->classlike_storage_provider->get($className)->parent_class, withInvalidArgumentExceptioncaught and a debug message logged (matching the surrounding fail-soft style).Tests
PHPT regression at
tests/Type/tests/Relation/Issue879SelfStaticParentClassTest.phptcovering:HasMany::save()afterhasMany(self::class),BelongsTo::associate()afterbelongsTo(self::class))static::classresolves to the declaring class (pinned conservative behavior)parent::classfrom a subclass resolves to the parent FQCNhasManyThrough(Mechanic::class, self::class)(intermediate at positionalIndex=1)hasManyThrough(related: Mechanic::class, through: parent::class)(named-arg branch)->using(self::class)on aBelongsToMany(firstClassStringArgpath)Plus DataProvider unit tests for
resolveClassConstFetchcoveringself, case-insensitiveSELF,static,parent(with and without parent), and a regularFoo::class.Disk fixtures (
tests/Application/app/Models/WorkOrderNote.php,Tool.php,PowerTool.php,PartReplacement.php) live in the existing car repair shop domain and are required becauseModelRegistrationHandler::class_exists()skips models not loadable by the autoloader.WorkOrderNotecovers theself::classself-referential pattern (threaded service notes);Tool/PowerTool extends Toolcoversparent::class;PartReplacement extends Pivotcovers->using(self::class).Checklist
tests/Type/and unit test intests/Unit/)