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

Update return types for hash functions #822

Closed
wants to merge 1 commit into from

Conversation

jlherren
Copy link
Contributor

@jlherren jlherren commented Dec 1, 2021

This consolidates HashHmacFunctionsReturnTypeExtension into HashFunctionsReturnTypeExtension (the two classes were very similar) and adds support for hash_file(), hash_hkdf() and hash_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 from string to non-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 to string|false?

@ondrejmirtes
Copy link
Member

Hi, the build failed, also please fix the conflict :) Thank you.

@ondrejmirtes
Copy link
Member

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.

@jlherren
Copy link
Contributor Author

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 false in PHP 8. But I'm not yet sure how to test that with NodeScopeResolverTest. I'll continue trying when I get time again.

@ondrejmirtes
Copy link
Member

You can yield different files based on PHP_VERSION_ID :) It's already happening in that test a few times :)

@jlherren
Copy link
Contributor Author

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.

@jlherren
Copy link
Contributor Author

Okay, the stubs are fixed, let's try again :) This time I also removed non-empty-string from parameters, it seems it causes too much troubles.

@ondrejmirtes
Copy link
Member

This might be fine, the failures are unrelated, I'm gonna review it soon :) Thank you!

@ondrejmirtes
Copy link
Member

Thank you! Merged: 1ec1c91

@jlherren jlherren deleted the hash-functions branch February 1, 2022 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants