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

Fix password_hash return type #2260

Merged
merged 4 commits into from
Mar 1, 2023
Merged

Conversation

VincentLanglet
Copy link
Contributor

@ondrejmirtes
Copy link
Member

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

@VincentLanglet
Copy link
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
Member

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

@VincentLanglet
Copy link
Contributor Author

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

Nice idea. Done :)

Copy link
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
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
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
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
2 participants