-
Notifications
You must be signed in to change notification settings - Fork 542
More precise hash return type #3665
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
Conversation
resources/functionMap.php
Outdated
'HaruPage::textOut' => ['bool', 'x'=>'float', 'y'=>'float', 'text'=>'string'], | ||
'HaruPage::textRect' => ['bool', 'left'=>'float', 'top'=>'float', 'right'=>'float', 'bottom'=>'float', 'text'=>'string', 'align='=>'int'], | ||
'hash' => ['non-empty-string|false', 'algo'=>'string', 'data'=>'string', 'raw_output='=>'bool'], | ||
'hash' => ['(non-falsy-string&lowercase-string)|false', 'algo'=>'string', 'data'=>'string', 'raw_output='=>'bool'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.php.net/manual/en/function.hash.php - must be conditional return, when $binary
flag is set, the string can be anything (contain UC easily)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! I updated the PR, please take a new look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not sure if non-falsy-string
in the functionMap.php is correct as short hash functions like crc32
can quite easily produce falsy string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvorisek can you show an example on 3v4l.org ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so your examples shows us a case where it is non-falsy-string
- which is the opposite of your thesis?
crc32
can quite easily produce falsy string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! '00..'
is non-falsy-string
per https://www.php.net/manual/en/language.types.boolean.php#language.types.boolean.casting but only '00..' == 0
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK every string with more then 1 char is non-falsy-string
This pull request has been marked as ready for review. |
What do you think about this PR @ondrejmirtes ? If preferred, I can use |
Thank you! |
Follow up of #3541 (cc @staabm)
Currently
md5('foo')
is considered non-falsy-string&lowercase-string, buthash('md5', 'foo')
is still non-empty-string.At first, I planned to list hash algo with lowercase output, but
and get everytime a non-falsy-string and lowercase-string (when binary is false)
So it seems like it's always
non-falsy-string&lowercase-string
with binary false, andnon-falsy-string
when binary true.