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 pre-PHP 8.2 compatibility for php_mt_rand_range() with MT_RAND_PHP #9839

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

TimWolla
Copy link
Member

Example: https://3v4l.org/0gEEY


As some left-over comments indicated:

Legacy mode deliberately not inside php_mt_rand_range()
to prevent other functions being affected

The broken scaler was only used for php_mt_rand_common(), not php_mt_rand_range(). The former is only used for mt_rand(), whereas the latter is used for array_rand() and others.

With the refactoring for the introduction of ext/random php_mt_rand_common() and php_mt_rand_range() were accidentally unified, thus introducing a behavioral change that was reported in FakerPHP/Faker#528.

This commit moves the checks for MT_RAND_PHP from the general-purpose range() function back into php_mt_rand_common() and also into Randomizer::getInt() for drop-in compatibility with mt_rand().

As some left-over comments indicated:

> Legacy mode deliberately not inside php_mt_rand_range()
> to prevent other functions being affected

The broken scaler was only used for `php_mt_rand_common()`, not
`php_mt_rand_range()`. The former is only used for `mt_rand()`, whereas the
latter is used for `array_rand()` and others.

With the refactoring for the introduction of ext/random `php_mt_rand_common()`
and `php_mt_rand_range()` were accidentally unified, thus introducing a
behavioral change that was reported in FakerPHP/Faker#528.

This commit moves the checks for `MT_RAND_PHP` from the general-purpose
`range()` function back into `php_mt_rand_common()` and also into
`Randomizer::getInt()` for drop-in compatibility with `mt_rand()`.
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me.

ext/random/engine_mt19937.c Show resolved Hide resolved
@TimWolla TimWolla merged commit 7f0b228 into php:PHP-8.2 Oct 28, 2022
TimWolla added a commit to TimWolla/php-src that referenced this pull request Oct 28, 2022
* PHP-8.2:
  Fix pre-PHP 8.2 compatibility for php_mt_rand_range() with MT_RAND_PHP (php#9839)
@TimWolla TimWolla deleted the mt-rand-php-compat branch October 29, 2022 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants