Return false for hash_hmac/hash_hmac_file on invalid algorithm#328
Return false for hash_hmac/hash_hmac_file on invalid algorithm#328ondrejmirtes merged 3 commits intophpstan:masterfrom
false for hash_hmac/hash_hmac_file on invalid algorithm#328Conversation
ondrejmirtes
left a comment
There was a problem hiding this comment.
Otherwise this looks great 👍
There was a problem hiding this comment.
This array can be here statically, it's not referenced anywhere else in the extension.
There was a problem hiding this comment.
Have inlined the value, hope this is correct?
I had orientated on other instances like ArrayPointerFunctionsDynamicReturnTypeExtension which also uses the private array $functions = [] notation.
There was a problem hiding this comment.
There needs to be a fallback for PHP 7.1 using https://www.php.net/manual/en/function.hash-algos.php
There was a problem hiding this comment.
I have added function_exists('hash_hmac_algos') ? hash_hmac_algos() : hash_algos() check but sadly PHPStan doesn't care. Any suggestion?
There was a problem hiding this comment.
It's fine, just make the tests pass and I'll fix the PHPStan issue just before merging :) Thanks.
111870c to
c72db7f
Compare
c72db7f to
4a24397
Compare
|
Just a 2 cents: Maybe it might make more sense to specify the |
|
@ondrejmirtes I have replaced the conditional declaration of those supported algorithms with a static one like @dktapps suggested. As the behaviour seems to change from return |
|
Thank you! |
|
I'll handle that case when I'll be adding support for PHP 8. |
This PR adds a dynamic function return type extension for both
hash_hmacandhash_hmac_fileas both functions can returnfalsein certain circumstances. For both functions this means providing an invalid algorithm as first parameter will statically return false, otherwisestringforhash_hmacandstring|falseforhash_hmac_file(we can't check for file readability in static analysis, and you should always check for file errors anyway).There has been a PR more than 1.5 years ago that tried to fix this within static functions map but that would lead to far more false positives.