Skip to content

Enhancement: Enable random_api_migration fixer #699

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

Merged
merged 2 commits into from
Sep 16, 2022

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Sep 11, 2022

This pull request

  • enables the random_api_migration fixer
  • runs make coding-standards

Replaces #545.
Follows #559.

πŸ’β€β™‚οΈ For reference, see https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.4.0/doc/rules/alias/random_api_migration.rst.

localheinz and others added 2 commits September 11, 2022 23:01
Co-authored-by: Andreas MΓΆller <[email protected]>
Co-authored-by: Mathias Reker <[email protected]>
Co-authored-by: Andreas MΓΆller <[email protected]>
Co-authored-by: Mathias Reker <[email protected]>
@cmb69
Copy link
Member

cmb69 commented Sep 12, 2022

Hmm, replacing rand() with mt_rand() is okay (to use the canonical function, not the alias), but wouldn't all these occurrences profit from using random_int() instead?

@localheinz
Copy link
Contributor Author

localheinz commented Sep 12, 2022

@cmb69

Looks like it is not possible to seed the generator when using random_int(), see https://stackoverflow.com/q/39352599/1172545.

In other words, the tests would be failing.

We could, however, extract (and thus decouple) a RandomNumberGenerator or similar with corresponding implementations. What do you think?

@TimWolla
Copy link
Member

Looks like it is not possible to seed the generator when using ramdon_int()

Yes, this is by design.

We could, however, extract (and thus decouple) a RandomNumberGenerator or similar with corresponding implementations. What do you think?

Likely not worth it. Use random_int() wherever reasonably possible and then switch to \Random\Randomizer once PHP 8.2 is available to use.

@localheinz
Copy link
Contributor Author

@TimWolla

Likely not worth it. Use random_int() wherever reasonably possible and then switch to \Random\Randomizer once PHP 8.2 is available to use.

Sounds great, but the production system currently runs on PHP 7.3 (maybe), and apparently nobody knows who is responsible for it nor how it could be upgraded to use a current PHP version.

@TimWolla
Copy link
Member

Yes, that's why I put the "once PHP 8.2 is available to use" there. This was not referring to the gold release in November, but to whenever the requirements for web-php are increased.

Adding a wrapper for the pre-PHP-8.2 RNGs is IMO pure busy work, using the CSPRNG would be great, but if it's painful to do, then it's not worth it just for the anti-spam challenges.

@morrisonlevi
Copy link
Contributor

IMO, we can merge this as-is. Is there something better? Maybe, but this is a clear improvement that we can merge right this moment, yes?

@cmb69
Copy link
Member

cmb69 commented Sep 16, 2022

Oh, indeed, I forgot about the tests. So I agree with @morrisonlevi that this is a step forward. Thank you all!

@cmb69 cmb69 merged commit 11c5706 into php:master Sep 16, 2022
@localheinz localheinz deleted the feature/random-api-migration branch September 16, 2022 13:27
@localheinz
Copy link
Contributor Author

Thank you, @cmb69, @morrisonlevi, and @TimWolla!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants