search

staudenmeir / belongs-to-through

Laravel Eloquent BelongsToThrough relationships
MIT License
1.15k stars 88 forks source link

Relation generics and PHPStan lvl 9 #101

Closed SanderMuller closed 1 month ago

SanderMuller commented 1 month ago

Similar to the work on https://github.com/staudenmeir/laravel-adjacency-lis I've introduced generic support for the BelongsToThrough relation, and increasing PHPStan to the max level was quite easy!

staudenmeir commented 1 month ago

Awesome, thanks!

staudenmeir commented 1 month ago

@SanderMuller FYI: PHPStan released v1.12.1 yesterday and that suddenly reports the return type getRelationExistenceQuery() as invalid (it's not caused by my changes in 949c7d9): https://github.com/staudenmeir/belongs-to-through/actions/runs/10697087338/job/29653550927

Not sure if that's a bug or PHPStan just hadn't been able to properly analyze the Eloquent and Query Builder construct until now.

I fixed it with a @var comment for now: https://github.com/staudenmeir/belongs-to-through/pull/101/commits/7a9f8b4e9d1c467e4f1a4c2ac1bf2a67781a6b95

SanderMuller commented 1 month ago

@SanderMuller FYI: PHPStan released v1.12.1 yesterday and that suddenly reports the return type getRelationExistenceQuery() as invalid (it's not caused by my changes in 949c7d9): https://github.com/staudenmeir/belongs-to-through/actions/runs/10697087338/job/29653550927

Not sure if that's a bug or PHPStan just hadn't been able to properly analyze the Eloquent and Query Builder construct until now.

I fixed it with a @var comment for now: 7a9f8b4

Weird $query->select() seems to already trigger a "downgrade" of the inferred type to Illuminate/Database/Query/Builder instead of retaining \Illuminate\Database\Eloquent\Builder<\Illuminate\Database\Eloquent\Model> despite the select() method returning $this, which should AFAIK keep its caller scope. I first thought maybe the Laravel Framework made a change, but it indeed seems to be related to the latest PHPStan update, though I'm not sure why. There's quite some changes in https://github.com/phpstan/phpstan-src/compare/1.12.0...1.12.1 😅 maybe we should accept your fix for now

staudenmeir commented 1 month ago

There's quite some changes in https://github.com/phpstan/phpstan-src/compare/1.12.0...1.12.1

Yeah... My best guess would be that it's related to one of the commits about mixins.

SanderMuller commented 1 month ago

There's quite some changes in phpstan/phpstan-src@1.12.0...1.12.1

Yeah... My best guess would be that it's related to one of the commits about mixins.

I'm actually getting the same PHPStan errors on my other projects. This seems to be the related issue https://github.com/phpstan/phpstan/issues/11624 and https://github.com/larastan/larastan/issues/2032, mixins are indeed mentioned there