Ensure php 8.2 compatibility

[chris-doehring]

This PR makes sure that the current state and future changes are tested against php 8.2.

I fixed two unit tests which fail in php 8.2 only, since php introduced a new random extension. Even without this change to the extension, I think the tests make more sense now.

As you can tell when executing the Faker tests in a php 8.2 environment, the following new deprecation's occur:

Remaining self deprecation notices (605)

  404x: Use of "static" in callables is deprecated
    200x in PhoneNumberTest::testPhoneNumber from Faker\Test\Provider\de_DE
    200x in PhoneNumberTest::testE164PhoneNumberFormat from Faker\Test\Provider\de_DE
    4x in PaymentTest::testBankAccountNumber from Faker\Test\Provider\ne_NP

  201x: Use of "self" in callables is deprecated
    134x in IbanTest::testIsValid from Faker\Test\Calculator
    67x in IbanTest::testChecksum from Faker\Test\Calculator

All of those are caused by legacy Faker components that probably cannot be fixed without making a backward incompatible change (related PHP RFC). I think it makes more sense to refactor all affected areas to the new Faker structure in order to be compatible to php 9.0.

Since we can't refactor everything right now, I've configured phpunit-bridge to not report a failed test execution unless we reach more than 1000 deprecation errors (we have 605 right now).

[pimjansen]

Thanks Chris! I dont really like the idea of just ignoring it though.

It is a bit out of the comfort zone since we planned 2.x to be the refactor but isnt it better to push that to 3.x and just ensure we push 2.x and drop some old php versions here?

If we do it the way your pr describes we still are not really compatible

[chris-doehring]

Hey @pimjansen,

in the way I proposed we are not ignoring it though and deprecation notices are notices, so we are still very much compatible to php 8.2. Those deprecation notices are hints that some code will not work in php 9.x anymore and don't change any compatibility behavior in php 8.x. We are not going to stop php 8.2 from releasing on November the 24th and according to Faker's version constraint's, we do support php 8.x. Any version of it.

My PR tested successfully that Faker ^1.20 is indeed compatible to php 8.2 and most importantly ensures that it will stay like that for minor changes still to come. I very much agree that we should not refactor the affected areas in 1.x, but instead do it in 2.x. At the end the goal of my PR is to ensure php 8.2 compatibility in the current stable release branch, nothing more 🙂

[chris-doehring]

I'm not sure if you have noticed, but my change to the phpunit.xml will not hide those deprecation notices, but will ensure that we still receive exit code 0, although we have some. Otherwise the tests would fail because of just notices. Even with those, we are compatible to php ^8.2.

[TimWolla]

I fixed two unit tests which fail in php 8.2 only, since php introduced a new random extension.

One of the PHP 8.2 ext/random maintainers here: Ugh. The behavior of existing (seeded) functionality should not change with PHP 8.2. Can you provide a simple reproducer, e.g. using https://3v4l.org/, so I can take a look?

[chris-doehring]

Hi @TimWolla,

I'm surprised that you actually found this PR 😄 This is basically what happens in the test: https://3v4l.org/SclY7

However, as you can see before php 8.2 it always returned the index 0 on third execution, but still the assertion did match three different words. The reason for that is, that the last execution of array_rand is somehow executed twice but I don't have a single clue why. In the test execution of Faker before 8.2 it returns the indexes 0, 1, 0, 2 and when I add backtraces, what triggered the individual array_rand calls, the last two outputs have been triggered by the same line of code. That's some black magic if you ask me. Under php 8.2 I get the indexes 1, 0, 2 which also matches the output of 3v4l.

code with debugging changes:

        $generator->addProvider(new class(...$words) {
            private $words;

            public function __construct(string ...$words)
            {
                $this->words = $words;
            }

            public function word(): string
            {
                $key = array_rand($this->words);
                var_dump($key);

                return $this->words[$key];
            }
        });

        $uniqueGenerator = $generator->unique();
        $generatedWords = [
            $uniqueGenerator->word(),
            $uniqueGenerator->word(),
            $uniqueGenerator->word(),
        ];
        var_dump($generatedWords);

PhpUnit output with php 8.1:

Testing Faker\Test\GeneratorTest
.                                                                                                                       1 / 1 (100%)
int(0)
int(1)
int(0)
int(2)
array(3) {
  [0]=>
  string(3) "foo"
  [1]=>
  string(3) "bar"
  [2]=>
  string(3) "baz"
}

But I think that's nothing you have to worry about, since it's not reproducible in 3v4l. You can already see the primary reason why it was necessary for me to refactor that test when checking my 3v4l example.

[TimWolla]

I'm surprised that you actually found this PR smile

In ancient times I've used and contributed to this library myself, e.g. fzaninotto#254 😃

The reason for that is, that the last execution of array_rand is somehow executed twice but I don't have a single clue why.

That's likely a feature of the ->unique() generator.

This is basically what happens in the test: https://3v4l.org/SclY7

Oh, you're using MT_RAND_PHP … don't do that. The MT_RAND_PHP variant is horribly horribly broken and we're hoping to deprecate it with PHP 8.3. Details are here: https://wiki.php.net/rfc/deprecations_php_8_3#mt_rand_php.

Nonetheless I'll have a look on whether this behavioral change can be avoided or not. Perhaps it's a side-effect of php/php-src#9197 (in which case this is unavoidable).

In the future this library should be updated to take an Random\Engine and then e.g. create a Random\Randomizer internally.

[chris-doehring]

Oh, you're using MT_RAND_PHP … don't do that. The MT_RAND_PHP variant is horribly horribly broken and we're hoping to deprecate it with PHP 8.3.

All Faker test cases are setup with that by default:
https://github.com/FakerPHP/Faker/blob/main/test/Faker/TestCase.php#L18
https://github.com/FakerPHP/Faker/blob/main/src/Faker/Generator.php#L690

I also don't like relying of that "hacky" logic, hence my test refactoring since I would say it now checks in more logical way. And yes, the Faker source and it's test cases need some refactoring.

[TimWolla]

@chris-doehring That is indeed an unexpected regression you found. I've proposed a fix in php/php-src#9839.

[TimWolla]

The fix is now merged, the original tests should pass again with RC 6.

[chris-doehring]

Thanks a lot @TimWolla! I reverted the test change. I guess we still need to wait another day for a fresh new nightly build of 8.2.0-dev in the shivammathur/setup-php@v2 project to see it in practice.

[chris-doehring]

The new nightly build of 8.2.0-dev indeed fixed the test execution. I was even able to revert a DateTime test which relies on the same randomness iteration on every execution. Thanks for your support, Tim!

[TimWolla]

The new nightly build of 8.2.0-dev indeed fixed the test execution.

Awesome, thanks for confirming.

I was even able to revert a DateTime test which relies on the same randomness iteration on every execution.

Yes, unless there's a second bug in there, PHP 8.2 should now be 100% compatible with PHP 8.1. The bug I fixed affected array_rand(), shuffle(), and str_shuffle() for MT_RAND_PHP.

---

For the future, I would recommend the following:

• Medium term: Replace MT_RAND_PHP with MT_RAND_MT19937 to migrate to the non-broken Mt19937 implementation. MT_RAND_PHP is proposed to emit deprecations with PHP 8.3 (but vote is pending).
• Longer term: Migrate to the object-oriented API (Random\Engine and Random\Randomizer) that was introduced with PHP 8.2. Documentation is here: https://www.php.net/manual/en/class.random-randomizer.php (it's super fresh, we bootstrapped it just 21 hours ago in php/doc-en@2166824, Pull Requests welcome to flesh it out). There's also a userland polyfill for the OO API: https://github.com/arokettu/php-random-polyfill

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions.

[pimjansen]

@chris-doehring thanks for all the effort. Will review this tonight with @bram-pkg so we can push a new minor that supports 8.2

[localheinz]

@chris-doehring

Can you rebase on top of main, please?

[localheinz]

@chris-doehring

Applying

diff --git a/src/Faker/Calculator/Iban.php b/src/Faker/Calculator/Iban.php
index c8fae242..470bf039 100644
--- a/src/Faker/Calculator/Iban.php
+++ b/src/Faker/Calculator/Iban.php
@@ -17,7 +17,7 @@ class Iban
         $checkString = substr($iban, 4) . substr($iban, 0, 2) . '00';

         // Replace all letters with their number equivalents
-        $checkString = preg_replace_callback('/[A-Z]/', ['self', 'alphaToNumberCallback'], $checkString);
+        $checkString = preg_replace_callback('/[A-Z]/', [self::class, 'alphaToNumberCallback'], $checkString);

         // Perform mod 97 and subtract from 98
         $checksum = 98 - self::mod97($checkString);

might fix a few deprecation notices.

[localheinz]

Thank you, @chris-doehring, @pimjansen, and @TimWolla!