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: use default mode for mt_srand (MT_RAND_MT19937) #828

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

NicolasDievart
Copy link

@NicolasDievart NicolasDievart commented Dec 5, 2023

What is the reason for this PR?

Hello 👋

PHP 8.3 is release since November 23
This PR is a following of #691 and the first step described by Tim Wolla here
#528 (comment)

Author's checklist

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

@NicolasDievart

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

Also see #15 (comment).

@stephpy
Copy link

stephpy commented Dec 20, 2023

Need it too since we upgrade to 8.3, why don't consider that easy solution #694 (comment) which make supports on each php versions ?

@NicolasDievart
Copy link
Author

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

localheinz commented Jan 2, 2024

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 localheinz changed the base branch from 2.0 to 1.23 January 2, 2024 13:27
@localheinz localheinz changed the base branch from 1.23 to 2.0 January 2, 2024 13:27
@localheinz
Copy link
Member

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

Closing in favour of #844.

@localheinz localheinz closed this Jan 2, 2024
@localheinz localheinz added the bug Something isn't working label Jan 2, 2024
@ip512
Copy link

ip512 commented Jan 3, 2024

Thank you @localheinz for fixing this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants