Skip to content

Fix password_hash return type#2260

Merged
ondrejmirtes merged 4 commits intophpstan:1.10.xfrom
VincentLanglet:bug5978
Mar 1, 2023
Merged

Fix password_hash return type#2260
ondrejmirtes merged 4 commits intophpstan:1.10.xfrom
VincentLanglet:bug5978

Conversation

@VincentLanglet
Copy link
Copy Markdown
Contributor

@ondrejmirtes
Copy link
Copy Markdown
Member

This isn't great. If the inputs are correct, users shouldn't be forced to handle null.

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

After some try, password_hash returns null

  • if the second parameter is not PASSWORD_DEFAULT| PASSWORD_BCRYPT| PASSWORD_ARGON2I| PASSWORD_ARGON2ID
  • if the second parameter is PASSWORD_DEFAULT| PASSWORD_BCRYPT and the salt option is not a 22-length string
  • if the second parameter is PASSWORD_DEFAULT|PASSWORD_BCRYPT and the cost option is not int<4, 31>
  • if the second parameter is PASSWORD_ARGON2I| PASSWORD_ARGON2ID and ext-libargon2 is not installed
  • if the second parameter is PASSWORD_ARGON2I| PASSWORD_ARGON2ID and the memory_cost/time_cost/threads is not a correct int but I don't know all the allowed values.

This won't be an easy dynamicReturnTypeExtension...

This isn't great. If the inputs are correct, users shouldn't be forced to handle null.

I understand, but currently PHP 7.4 users are already forced to handle false
https://phpstan.org/r/5464625f-1925-4c4d-a8c5-7aea19ace58f

When I looked at
https://stackoverflow.com/questions/39729941/php-password-hash-returns-false
https://bugs.php.net/bug.php?id=77218
we find that

So basically, for PHP ≤ 7.3, the function returned FALSE for a
failing implementation, and NULL for invalid parameters in
combination with a warning (the latter likely according to the
general convention to return NULL on ZPP failures).

So even without dynamicReturnTypeExtension it would be more useful (and correct ?) for functionMap_php74delta.php to have

- 'password_hash' => ['string|false', 'password'=>'string', 'algo'=>'string|null', 'options='=>'array'],
+ 'password_hash' => ['string|null', 'password'=>'string', 'algo'=>'string|null', 'options='=>'array'],

while I can let

'password_hash' => ['string|false', 'password'=>'string', 'algo'=>'string|null', 'options='=>'array'],

for the original functionMap_php.

Would you be ok with this first step ?

@ondrejmirtes
Copy link
Copy Markdown
Member

I'd just do benevolent union of string|false|null and call it a day.

@VincentLanglet
Copy link
Copy Markdown
Contributor Author

I'd just do benevolent union of string|false|null and call it a day.

Nice idea. Done :)

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

You're closing an issue with this PR - needs a regression test.

public function testBug5978(): void
{
if (PHP_VERSION_ID >= 80000) {
$this->markTestSkipped('Test does not run on PHP >= 8.0.');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead, you could assert different errors on PHP 7.x vs. 8.x.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that's better

@ondrejmirtes ondrejmirtes merged commit 4b3a9d5 into phpstan:1.10.x Mar 1, 2023
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you!

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.

Something wrong with checking return values of password_hash?

2 participants