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] Add generic type parameters for Eloquent\Builder in @param Tag to avoid PHPStan errors #52944

Conversation

kayw-geek
Copy link
Contributor

Currently, the @param tag for the Illuminate\Database\Eloquent\Builder class does not specify generic type parameters. As a result, PHPStan throws errors when implements the apply() method of Scope .

This PR adds the * generic type parameters to Illuminate\Database\Eloquent\Builder.

@ollieread
Copy link
Contributor

@kayw-geek I've just tested this locally and PHPStan complains about the usage of *.

@kayw-geek
Copy link
Contributor Author

@kayw-geek I've just tested this locally and PHPStan complains about the usage of *.

Can you provide a detailed error report or PHPStan playground? Theoretically, it should work.

@ollieread
Copy link
Contributor

@kayw-geek I've just tested this locally and PHPStan complains about the usage of *.

Can you provide a detailed error report or PHPStan playground? Theoretically, it should work.

Hmm, it's only in very specific situations that it breaks, and it happens when there are several layered generics, which, I think, is an implementation and PHPStan specific thing, not this. Ignore me, sorry.

@taylorotwell
Copy link
Member

I feel like this would apply to a huge number of other places in the framework? What PHPStan level?

@ollieread
Copy link
Contributor

I feel like this would apply to a huge number of other places in the framework? What PHPStan level?

There are many places in the framework that could be updated with proper generics for PHPStan (I always use max), so I think you're right, this would be a bigger task.

@kayw-geek I use larastan which has stubs to cover this sort of thing.

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.

3 participants