-
Notifications
You must be signed in to change notification settings - Fork 11k
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] chore: update to PHPStan Level 1 #51956
Conversation
$this->addOneOfManySubQueryConstraints($subQuery, $groupBy, $columns, $aggregate); | ||
$this->addOneOfManySubQueryConstraints($subQuery, column: null, aggregate: $aggregate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a tricky one:
The method only accepts 3 arguments while 4 were being passed in:
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php
Lines 34 to 42 in 032f378
/** | |
* Add constraints for inner join subselect for one of many relationships. | |
* | |
* @param \Illuminate\Database\Eloquent\Builder $query | |
* @param string|null $column | |
* @param string|null $aggregate | |
* @return void | |
*/ | |
abstract public function addOneOfManySubQueryConstraints(Builder $query, $column = null, $aggregate = null); |
However, the only two implementations don't even use the last two arguments:
framework/src/Illuminate/Database/Eloquent/Relations/HasOne.php
Lines 88 to 91 in 032f378
public function addOneOfManySubQueryConstraints(Builder $query, $column = null, $aggregate = null) | |
{ | |
$query->addSelect($this->foreignKey); | |
} |
framework/src/Illuminate/Database/Eloquent/Relations/MorphOne.php
Lines 86 to 89 in 032f378
public function addOneOfManySubQueryConstraints(Builder $query, $column = null, $aggregate = null) | |
{ | |
$query->addSelect($this->foreignKey, $this->morphType); | |
} |
Additionally, the $groupBy
and $columns
variables that were being passed could be arrays whereas the $column
parameter can only be string|null
---given all this I just decided to pass null
here for the column
parameter.
However, I'm open to other ideas if there's something more meaningful we can pass.
3e55014
to
2ee3bbf
Compare
@calebdw don't forget to mark the PR ready for review if you need a new one. |
Will do @driesvints! |
Thanks! |
This has broken the actions and are facades appear to be all messed up. |
I'll take a look |
@taylorotwell, all of the tests passed when this was merged so what was the issue? |
It looks like there might be a bug in the laravel/facade-documenter parsing? For static forwarding, facades should use If all the facades properly use the |
@calebdw, I've fixed the documenter so that it won't bork the docblocks when it cannot document things properly. I don't feel we should use Most tooling I have seen interprets I know that PHPStan have a different interpretation of this that works with Facades, which is "when However, because of this difference in interpretation, language servers all work differently. If we were to use I don't think we should appease PHPStan while making the development experience worse for a lot of humans. On a personal note, I believe that PHPStan should have introduced a Here is a quick example of PHPStan showing everything is okay but it is not due to the weird |
There's an open PR for this: phpstan/phpstan#11259 |
Hello!
This PR cleans up some bugs and brings the framework to PHPStan Level 1.
For reference, here is all of the errors on 11.x:
Thanks!