Skip to content
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

[11.x] Fix incorrect PHPDoc for Builder relation #52754

Conversation

kayw-geek
Copy link
Contributor

@kayw-geek kayw-geek commented Sep 12, 2024

In PHPStan, the parameters in a Closure are non-covariant by default. So, when you define a Closure with a parameter type-hinted as a subclass of Relation, PHPStan will throw the following error:

expects 
array<(Closure(Illuminate\Database\Eloquent\Relations\Relation<*, *, *>): mixed)|string>|string, 
array<string, Closure(Illuminate\Database\Eloquent\Relations\HasOne): void} given

This issue may be caused by #52729

@kayw-geek kayw-geek force-pushed the bugfix/fix-incorrect-phpdoc-for-relations branch from 2808290 to f460a7f Compare September 12, 2024 08:51
@kayw-geek kayw-geek changed the title Fix incorrect PHPDoc for Builder relation [11.x] Fix incorrect PHPDoc for Builder relation Sep 12, 2024
@kayw-geek kayw-geek force-pushed the bugfix/fix-incorrect-phpdoc-for-relations branch from f460a7f to c3d9e0f Compare September 12, 2024 09:00
@staudenmeir
Copy link
Contributor

/cc @calebdw

@Plytas
Copy link
Contributor

Plytas commented Sep 12, 2024

Also, I'm pretty sure it doesn't work with nested eager loading as the value in the array can be another array.

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

@kayw-geek,

This will not work---I believe call-site variance only works for generics (e.g., Collection<int, covariant Foo>)

Perhaps this is something @ondrejmirtes might consider supporting?

In the meantime you can simply change your closure to specify the Relation instead of HasOne:

- $query->with(['foo' => fn (HasOne $q) => $q]);
+ $query->with(['foo' => fn (Relation $q) => $q]);

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

@Plytas,

Also, I'm pretty sure it doesn't work with nested eager loading as the value in the array can be another array.

Recursive array types are a bit painful, I'll add array to the definition and then open a PHPStan issue to see if it would be possible to get an array-rec type

@kayw-geek
Copy link
Contributor Author

PHPStan playground is not support, but PHPStan program is supported.

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

PHPStan playground is not support, but PHPStan program is supported.

Sorry, you're mistaken here---the playground is the phpstan program. The only reason why you're not seeing errors is that the framework phpstan level is too low,, This results in a phpstan parse error and it now knows nothing about the parameter type

@kayw-geek
Copy link
Contributor Author

@calebdw
I want to more accurate native type hint in my project.

@Plytas
Copy link
Contributor

Plytas commented Sep 12, 2024

@calebdw I think array will be fine for now.

Regarding using Relation as type hint:

In the meantime you can simply change your closure to specify the Relation instead of HasOne:

- $query->with(['foo' => fn (HasOne $q) => $q]);
+ $query->with(['foo' => fn (Relation $q) => $q]);

There's a slight issue with it if you need to perform other operations on the inner query like ->select() or ->where(). PHPstan will complain that those methods are not available on Relation.

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

@Plytas, the Relation forwards those method to the mixed in Builder

@Plytas
Copy link
Contributor

Plytas commented Sep 12, 2024

It does on runtime, but I wan't able to get PHPstan happy with it.

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

It does on runtime, but I wan't able to get PHPstan happy with it.

Then there's a bug somewhere, but it does work: https://phpstan.org/r/a17bc6f0-ca28-4dd5-9c66-fbebf28db054

@Plytas
Copy link
Contributor

Plytas commented Sep 12, 2024

Thanks for going extra length to confirm it. I'll try to debug why it's not working on my end.

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

Might be an issue with Larastan

@Plytas
Copy link
Contributor

Plytas commented Sep 12, 2024

@calebdw just an FYI. I believe you're right about it being Larastan issue. It's missing the @mixin annotation.

https://github.com/larastan/larastan/blob/2.x/stubs/common/Relation.stub

@calebdw
Copy link
Contributor

calebdw commented Sep 12, 2024

Larastan has ClassReflectionExtensions that handle the mixin magic---please raise an issue over there and I'll look into it

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.

5 participants