Fix: use default mode for mt_srand (MT_RAND_MT19937)

FakerPHP / Faker

Faker is a PHP library that generates fake data for you

https://fakerphp.github.io

Other

3.62k stars 353 forks source link

Fix: use default mode for mt_srand (MT_RAND_MT19937) #828

Closed NicolasDievart closed 10 months ago

NicolasDievart commented 11 months ago

What is the reason for this PR?

Hello :wave:

PHP 8.3 is release since November 23 This PR is a following of https://github.com/FakerPHP/Faker/pull/691 and the first step described by Tim Wolla here https://github.com/FakerPHP/Faker/pull/528#issuecomment-1295790637

[x] Fixed an issue (resolve)

Author's checklist

[x] Follow the Contribution Guide
[ ] New features and changes are documented

Summary of changes

The method mt_srand has a default mode since PHP 7.1, which is MT_RAND_MT19937, and Faker requires at least PHP 7.4. As of PHP 8.3, MT_RAND_PHP is deprecated, this PR remove the deprecated mode used, to use the default one.

As PHP 8.3 is released, the CI file has been modified to test against PHP 8.3, not as experimental

Review checklist

[ ] All checks have passed
[ ] Changes are added to the CHANGELOG.md
[ ] Changes are approved by maintainer

localheinz commented 11 months ago

@NicolasDievart

Let's rebase after the potential merge of #829 to see on which versions of PHP tests are failing.

Also see https://github.com/FakerPHP/Faker/discussions/15#discussioncomment-7787434.

stephpy commented 11 months ago

Need it too since we upgrade to 8.3, why don't consider that easy solution https://github.com/FakerPHP/Faker/issues/694#issuecomment-1828742194 which make supports on each php versions ?

NicolasDievart commented 11 months ago

I let @localheinz decide this part as there are a few discussion about PHP versions supported and the global design of this part of Faker.

As I understand the failing tests, the change of the mode from MT_RAND_PHP to MT_RAND_MT19937, the result is different, that's why I had to change a few tests to match the new results. However, some tests can't be fixed as is, as the new results do not match in some case : If you fix this one, your break the next one.

If you take the "easy solution", you would have to keep the tests for older PHP versions, and adapt the tests for PHP 8.3 with MT_RAND_MT19937

localheinz commented 10 months ago

We currently have the following testing strategy:

we seed the random generator
we generate values and assert that they assume specific values

As we can see in this and other pull requests, generating and asserting that generated values assume specific values is bound to fail.

As far as I can see, we have the following options:

we drop support for PHP 7.4, 8.0, 8.1, and 8.2 - not going to happen, if you see how much resistance there is from people demanding support for PHP 7.4
we adjust existing tests to avoid asserting that generated values assume specific values and instead assert that they have a specific shape at most
after applying ef3ba0f, we split and adjusts failing tests into tests that require PHP <= 8.2 and PHP > 8.3

Do you see any other options?

localheinz commented 10 months ago

@ip512 @NicolasDievart @stephpy

How do you feel about #844?

We just need someone else from @FakerPHP/maintainers to approve, then we can move forward and get at least a version of fakerphp/faker on 1.2 that is compatible with PHP 8.3 without triggering deprecations.

localheinz commented 10 months ago

Closing in favour of #844.

ip512 commented 10 months ago

Thank you @localheinz for fixing this issue