Additional Analysers for Models/Query Builder #438
Conversation
peterfox
added
enhancement
…ers-further' into feature/analyse-models-builders-further
|
@calebdw I'll leave this PR here. I've added some relation analysis methods but I don't want to go overboard with creating lots of cool tooling without them being used more first. |
| $extendedPropertyReflection = $objectType->getInstanceProperty('model', $scope); | ||
| $modelType = $extendedPropertyReflection->getReadableType(); | ||
|
|
||
| if ($modelType->isSuperTypeOf(self::modelType())->no()) { |
There was a problem hiding this comment.
Your super type checks are still backwards (there's a lot of them that need to be updated)---the super type needs to be outside the function call not inside (e.g., $user->isSuperTypeOf($model) will fail because User is not a super type of Model; it should be $model->isSuperTypeOf($user))
| if ($modelType->isSuperTypeOf(self::modelType())->no()) { | |
| if (self::modelType()->isSuperTypeOf($modelType)->no()) { |
src/NodeAnalyzer/ModelAnalyzer.php
Outdated
| $extendedMethodReflection = $classReflection->getMethod($relationName, $scope); | ||
|
|
||
| foreach ($extendedMethodReflection->getVariants() as $extendedParametersAcceptor) { | ||
| if ($extendedParametersAcceptor->getReturnType()->isSuperTypeOf(self::relationType())->yes()) { |
There was a problem hiding this comment.
| if ($extendedParametersAcceptor->getReturnType()->isSuperTypeOf(self::relationType())->yes()) { | |
| if (self::relationType()->isSuperTypeOf($extendedParametersAcceptor->getReturnType())->yes()) { |
There was a problem hiding this comment.
Are you sure for this one? When I change it, the tests break for scenario it_can_determine_if_the_model_has_relationship_method at line tests/NodeAnalyzer/ModelAnalyzerTest.php:117
There was a problem hiding this comment.
Yes I'm sure, the problem is that the relationship needs the return type:
public function relationship(): HasManyWithout it, phpstan is resolving the return type as mixed and so your test is a false positive because you're asking if mixed is a supertype of Relation which is true
There was a problem hiding this comment.
Would PHPStan now infer the correct type purely from the fact it always returns a HasMany object anyways?
There was a problem hiding this comment.
Well you're right although I'm a little confused still because I assumed it would infer the type from the PHPDocs and the return of the method but I guess not.
There was a problem hiding this comment.
PHPStan doesn't descend into method and functions---it stops at the boundary. When inside a method phpstan checks that you return what you say you return, and when outside a method phpstan checks to make sure the arguments you pass are compatible
https://phpstan.org/user-guide/troubleshooting-types#what-phpstan-doesn%E2%80%99t-do
There was a problem hiding this comment.
Ah okay. That makes sense. Thank you for your patience with me.
There was a problem hiding this comment.
No worries! It's a common misconception, but it helps to better understand how phpstan works once you grasp it 😃
…driftingly/rector-laravel into feature/analyse-models-builders-further
…driftingly/rector-laravel into feature/analyse-models-builders-further
calebdw
left a comment
calebdw
left a comment
There was a problem hiding this comment.
Looks good! Just a couple of more comments
This adds:
Improves: