-
Notifications
You must be signed in to change notification settings - Fork 430
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
Update return types for hash functions #822
Conversation
879cdd2
to
fa48721
Compare
Hi, the build failed, also please fix the conflict :) Thank you. |
BTW there are some build failures on current master (in testing 3rd party projects coming from phpstan/phpstan) so beware, I hope to fix them soon. |
Thank you! Yes, I currently struggle in writing test that are expected to behave differently between PHP 7.4 and PHP 8.0+. Specifically, some of those functions no longer return |
You can yield different files based on PHP_VERSION_ID :) It's already happening in that test a few times :) |
I think I got it sorted out now (not yet pushed). However, I came across this bug that affects the PHP 8 stubs. I'll wait to see what happens over there and update the stubs once that is fixed. |
fa48721
to
5f91c99
Compare
Okay, the stubs are fixed, let's try again :) This time I also removed |
This might be fine, the failures are unrelated, I'm gonna review it soon :) Thank you! |
5f91c99
to
a1c77cb
Compare
Thank you! Merged: 1ec1c91 |
This consolidates
HashHmacFunctionsReturnTypeExtension
intoHashFunctionsReturnTypeExtension
(the two classes were very similar) and adds support forhash_file()
,hash_hkdf()
andhash_pbkdf2()
.It now also deduces the return type correctly when writing
hash($cond ? 'md5' : 'sha256', ...)
.I also added
non-empty-string
for return types and parameters. Return types should be fine, but I'm not sure whether it's too dangerous to narrow function parameters fromstring
tonon-empty-string
, since that's likely to cause new errors. If that's not desired, I can remove them again.Finally, I'm not sure about why there was logic to treat a
mixed
argument for the$algo
parameter in such a way, that these functions would return(string|false)
, but I left it like that. Perhaps this can be changed tostring|false
?