These tests already contain a PHP version detection logic for this purpose. Should we rather fix that instead of adding another layer?

You mean addingif (\PHP_VERSION_ID < 80200) { $this->markAsSkipped(...); } at the top of the test instead of having the annotation ?

if (\PHP_VERSION_ID <80218 || \PHP_VERSION_ID >=80300 && \PHP_VERSION_ID <80305) {// password_hash() does not accept passwords containing NUL bytes since PHP 8.2.18 and 8.3.5$this->assertFalse($hasher->verify(password_hash($plainPassword, \PASSWORD_BCRYPT, ['cost' =>4]),$plainPassword));}

This part should probably be updated.

But it is actually a bit different. In the part you highlight, we still try to hash+verify, whereas prior to 8.2,password_hash() directly throws.$hasher->verify() is then skipped. Because of this, it doesn't seems relevant to me to trigger the test before PHP 8.2. 🤔

Checkingpassword_hash() actually throws something would be the same as testing the function itself, which is probably already done somewhere in the PHP engine.

However, changing from the annotation to anif statement at the beginning of the test case suits me fine 🙂

$this->assertTrue($hasher->verify($hasher->hash($plainPassword), $plainPassword));

This line should always be executed as our password hasher makes sure to not pass the nul byte topassword_hash(). That's why we have theif. I actually do not really understand why the PHP 7.2 and 7.4 suddenly fail as the changes in PHP I referenced in#54587 did not happen on these versions. 🤔

What puzzles me here: PHP 7.2 is around for a while already, why does this test start to become a problem just now?

Ok I understand what I missed, I didn't see that the calls inassertFalse() andassertTrue() were different 🤦 I'll investigate a bit more and reopen something when I have the right fix

What puzzles me here: PHP 7.2 is around for a while already, why does this test start to become a problem just now?

same for me, could it be that the Docker image that we use in the CI uses PHP versions that have patches backported? 🤔

Here is the commit responsible of the failure:php/php-src@11f2568

same for me, could it be that the Docker image that we use in the CI uses PHP versions that have patches backported? 🤔

In that case, shall we drop the version detection entirely and make the test tolerant to the error raised by PHP?

It seems thatpassword_hash() doesn't support\0 at all now. ShouldNativePasswordHasher but updated to forbid the nul char? Currently, it allows it if the password is more than 72 chars long

for a job based on PHP 7.4.33 I would have expected to to have that same instruction appear inhttps://github.com/php/php-src/blob/PHP-7.4.33/ext/standard/password.c too

Currently, it allows it if the password is more than 72 chars long

Nope, when a plain password contains nul it's always sha512 hashed and base 64 encoded (the same is also true if the password is longer than or equal to 72 characters no matter if it contains nul).

Maybe@shivammathur can tell us if that patch has been backported to his PHP 7 builds. Sorry for the shameless ping. 😓

when a plain password contains nul it's always sha512 hashed and base 64 encoded

Oh yes my bad, nothing to do in the hasher indeed

Answering my own question: Yes, the patch has been backported asshivammathur/php-src-backports@d22d9eb.

Much love for this repository,@shivammathur. ❤️

In that case, I'm back to my proposal in#54858 (comment)

In that case, shall we drop the version detection entirely and make the test tolerant to the error raised by PHP?

yes, makes sense to me then

What do you think of this new approach?

Thank you@alexandre-daubois.