Skip to content

[12.x] fix: add TPivotModel default and define pivot property in {Belongs,Morph}ToMany #55086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Mar 19, 2025

Hello!

This cleans up the TPivotModel implementation in #55053 to use a default so users don't need to set the template if not needed. It also uses the template to define the pivot property on the resultant records.

Note that there is currently a PHPStan bug that prevents the TAccessor template from being used as an object key. As soon as that bug is fixed I will submit another PR.

Closes larastan/larastan#1774, closes #55078
CC: @browner12 , @AJenbo

Thanks!

@calebdw calebdw changed the title [12.xfix: add TPivotModel default and define pivot property in {Belongs,Mo… [12.x] fix: add TPivotModel default and define pivot property in {Belongs,Morph}ToMany Mar 19, 2025
@@ -20,9 +20,12 @@
/**
* @template TRelatedModel of \Illuminate\Database\Eloquent\Model
* @template TDeclaringModel of \Illuminate\Database\Eloquent\Model
* @template TPivotModel of \Illuminate\Database\Eloquent\Relations\Pivot
* @template TPivotModel of \Illuminate\Database\Eloquent\Relations\Pivot = \Illuminate\Database\Eloquent\Relations\Pivot
Copy link
Member

@crynobone crynobone Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are use cases where project using \Illuminate\Database\Eloquent\Relations\Concerns\AsPivot&\Illuminate\Database\Eloquent\Model instead of \Illuminate\Database\Eloquent\Relations\Pivot

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be necessary to have an interface for this

Copy link
Contributor Author

@calebdw calebdw Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\Illuminate\Database\Eloquent\Relations\Concerns\AsPivot&\Illuminate\Database\Eloquent\Model is not a correct type because you can't have a trait in a intersection type:

The user just needs to extend from Pivot/MorphPivot

@taylorotwell
Copy link
Member

@calebdw does this have any negative affect on PHP Storm and VS Code experience?

@calebdw
Copy link
Contributor Author

calebdw commented Mar 19, 2025

Not that I know of, this should improve the experience as the code now better understands that the pivot property exists

@taylorotwell taylorotwell merged commit 069e868 into laravel:12.x Mar 19, 2025
39 checks passed
@calebdw
Copy link
Contributor Author

calebdw commented Mar 19, 2025

Thank you sir!

@calebdw calebdw deleted the belongs_to_many branch March 19, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken BelongsToMany type definitions Undefined property pivot
4 participants