Skip to content

feat: update PHPStan lvl to 5 #91

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

SMillerDev
Copy link
Contributor

@SMillerDev SMillerDev commented Mar 5, 2025

This updates the PHPStan check to lvl 5. There are still some unresolved issues and the method types are based on the redis docs so might not be complete.

@SMillerDev SMillerDev force-pushed the feat/general/phpstan_4 branch from 7dfe886 to 9ce7944 Compare March 5, 2025 11:04
@SMillerDev SMillerDev changed the title feat: update PHPStan lvl to 4 feat: update PHPStan lvl to 5 Mar 5, 2025
Copy link
Collaborator

@pprkut pprkut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a couple of type changes you can have a look at #92. Or wait until that one is in. It should make a couple things easier.

@SMillerDev SMillerDev force-pushed the feat/general/phpstan_4 branch from 2fa54aa to 546e5a0 Compare March 17, 2025 11:36
@SMillerDev SMillerDev force-pushed the feat/general/phpstan_4 branch from 546e5a0 to b195deb Compare March 17, 2025 11:38
@SMillerDev
Copy link
Contributor Author

Updated based on the latest state of the develop branch.

@SMillerDev SMillerDev requested a review from pprkut March 17, 2025 11:38
@pprkut
Copy link
Collaborator

pprkut commented Mar 17, 2025

For the one remaining failure, it looks like the documentation for blpop in Credis is incomplete. It does, in fact, allow an array input.
https://github.com/colinmollenhour/credis/blob/v1.16.2/Client.php#L1670 will take the list of strings we pass in https://github.com/resque/php-resque/blob/develop/lib/Resque.php#L195 and make it a vararg essentially, so the signature is more like blpop(string $key1, string $key2, string $key3, ..., int $timeout), but that's obviously not supported by PHP.

I think adjusting that to string|string[] should be good enough.

@pprkut
Copy link
Collaborator

pprkut commented Mar 17, 2025

I won't stop you from pushing ahead here, but I feel like scope-creep is setting in 🙂
We don't need to push to level 5 directly, we can totally do intermediate steps which would keep the changes more manageable IMHO. But I leave that up to you

@SMillerDev
Copy link
Contributor Author

I think it was mostly editing in the web UI that made it seem bad. I've covered everything now, except the fact that $instance can be unset but phpstan doesn't acknowledge that.

@danhunsaker
Copy link
Member

$instance can be unset but phpstan doesn't acknowledge that.

With a type hint that doesn't include an uninitialized value as an option, it can't assume null is valid. Add |null and it should sort itself.

@SMillerDev
Copy link
Contributor Author

Add |null and it should sort itself.

Yeah, I had that until #91 (comment)

@pprkut
Copy link
Collaborator

pprkut commented Mar 18, 2025

I'll check it some more. Especially in newer PHP versions the assumption of "unitinitialized == null" is no longer true, that's why I don't think adding a type hint for it is correct. But maybe it's the simplest solution.

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