Use static instead of $this in relation factory return templates#917
Open
alies-dev wants to merge 4 commits into
Open
Use static instead of $this in relation factory return templates#917alies-dev wants to merge 4 commits into
static instead of $this in relation factory return templates#917alies-dev wants to merge 4 commits into
Conversation
…mplates Replace `@return Relation<TRelated, $this>` with `@return Relation<TRelated, static>` in every HasRelationships factory method (hasOne, hasOneThrough, morphOne, belongsTo, morphTo, hasMany, hasManyThrough, morphMany, belongsToMany, morphToMany, morphedByMany). Empirically, Psalm 7 substitutes `static` with the late-static-bound class while leaving `$this` in template position unsubstituted. That collapsed the intermediate of chained calls inside relation methods (e.g. `$this->belongsTo(X::class)->withoutGlobalScopes()`) to `mixed` and surfaced as MixedMethodCall. `static` and `$this` are semantically equivalent at the TDeclaringModel slot, so the divergence from Laravel source is safe. Adds a trait-level docblock pinning the divergence so future stub-sync passes preserve it, and a regression test that asserts both the intermediate factory type and the chained return type for the structurally distinct relation shapes (2-template, 3-template through, 4-template pivot, no-TRelatedModel morphTo). Fixes #913
alies-dev
changed the title
static instead of $this in relation factory return templates
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Psalm 7 template-substitution regression affecting fluent chaining on Eloquent relation factory calls inside model relation methods (e.g. belongsTo(...)->withoutGlobalScopes()), where the intermediate relation type previously collapsed to mixed and triggered MixedMethodCall.
Changes:
- Updated
HasRelationshipsrelation factory stubs to usestatic(instead of$this) in theTDeclaringModelgeneric slot so Psalm 7 correctly substitutes the concrete model type. - Added a PHPT regression test that asserts the intermediate inferred relation types (via
@psalm-check-type-exact) and verifies fluent chaining works across multiple relation template shapes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
stubs/common/Database/Eloquent/Concerns/HasRelationships.phpstub |
Switches TDeclaringModel return template arguments from $this to static across all relation factories; adds a trait-level note documenting the intentional divergence from Laravel source. |
tests/Type/tests/Relation/RelationChainInsideRelationMethodTest.phpt |
Adds a regression test covering intermediate relation typing and chained builder-mixin calls for several structurally distinct relation types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
\$this->belongsTo(X::class)->withoutGlobalScopes()(and similar chained calls on relation factories inside Model methods) raisedMixedMethodCallbecause the intermediate factory call collapsed tomixed. The root cause sits in the stub:@return Relation<TRelated, \$this>inHasRelationshipsleft the TDeclaringModel slot unsubstituted under Psalm 7. Swapping\$thisforstaticsubstitutes correctly and produces the right concrete generic type.The receiver of any relation factory is always the model itself, so
staticand\$thisare semantically equivalent at the TDeclaringModel slot. This is a stub divergence from Laravel source documented inline in the trait.Applies to all 11 factory methods in
HasRelationships:hasOne,hasOneThrough,morphOne,belongsTo,morphTo,hasMany,hasManyThrough,morphMany,belongsToMany,morphToMany,morphedByMany.Notes for reviewers
$this->belongsTo(...)->withoutGlobalScopes()chain in relation methods #913 for context).@psalm-check-type-exact, since a future regression that re-collapses the intermediate tomixedwould still satisfy a declared return type) and the full chained-return shape for the four structurally distinct relation shapes:belongsTo,hasOne,hasMany)hasManyThrough)belongsToMany)morphTo)morphOne,morphMany,hasOneThrough,morphToMany,morphedByMany) share the same template shapes andstatic-substitution path with covered methods.ModelRelationReturnTypeHandlerwas introduced specifically to work around the same\$this-in-template Psalm limitation for user-defined relation methods. Now that the factory stubs work directly, that handler might have some redundant logic to simplify.Fixes #913