Fix GH-10292 make the default value of the first param of srand() and mt_srand() nullable

[kocsismate]

No description provided.

[TimWolla]

Diff LGTM apart from that nit, I don't have any strong target branch preference.

[zeriyoshi]

This fix causes a BC Break. Currently, if null is passed to mt_srand(), integer 0 is used as the seed value unless in strict type mode. As a result, reproducibility-dependent code is broken.

https://3v4l.org/dNTXu
https://3v4l.org/t5G2B

That said, this seems like an appropriate modification to me personally as well. I did not make this change when proposing the Random extension in order not to include an unnecessary BC Break.

Perhaps the target branch should be master, don't PHP-8.2.

[zeriyoshi]

Slightly off topic, but I am thinking of proposing to deprecate mt_srand itself in PHP 8.3. This function is useful, but it takes the MT state globally on the PHP VM. This is dangerous in the always-on PHP that has become popular in recent years.

I would recommend using random_int if you don't want reproducibility of results, and using Randomizer if reproducibility is required.

This change may not be necessary if the above proposal is included in PHP 8.3, just in case.

[kocsismate]

Slightly off topic, but I am thinking of proposing to deprecate mt_srand itself in PHP 8.3

I welcome this change! I'd be even happier if rand() and mt_rand() was included as well. :)

This change may not be necessary if the above proposal is included in PHP 8.3, just in case.

Since PHP 8 series is going to be used for a long time, I think we can and should fix the issue in question independently from the RFC.

[cmb69]

Thank you!

The implementation looks good, and only targeting "master" makes sense to me. We should not forget to fix the documentation; probably claiming an UNKNOWN default is the way to go for now, and that is likely also the proper solution for the stub.

[TimWolla]

I welcome this change! I'd be even happier if rand() and mt_rand() was included as well. :)

@kocsismate see https://wiki.php.net/rfc/deprecations_php_8_3#global_mersenne_twister

I think we can and should fix the issue in question independently from the RFC.

I agree.

[kocsismate]

see https://wiki.php.net/rfc/deprecations_php_8_3#global_mersenne_twister

Perfect!

[kocsismate]

We should not forget to fix the documentation; probably claiming an UNKNOWN default is the way to go for now, and that is likely also the proper solution for the stub.

Do you mean to update the stub for PHP 8.1 and PHP 8.2? I can take care of it, sure! P.S. Is the upgrading note ok?

[TimWolla]

P.S. Is the upgrading note ok?

LGTM, but this likely should also have a corresponding NEWS entry.

[TimWolla]

LGTM

Or, hmm. Perhaps something like this:

Changed mt_srand() and srand() to not check the number of arguments to determine whether a random seed should be used. Passing null will generate a random seed, 0 will use zero as the seed. The functions are now consistent with Mt19937::__construct().

[kocsismate]

Or, hmm. Perhaps something like this:

I liked the suggestion so I used it in the upgrading guide, while I included something much shorter in the news.

[TimWolla]

LGTM, thanks.

[TimWolla]

Do you mean to update the stub for PHP 8.1 and PHP 8.2? I can take care of it, sure!

@kocsismate Will you handle whatever is required for the non-master versions?

[kocsismate]

Will you handle whatever is required for the non-master versions?

Yeah, I'll do it tomorrow! Thanks for the reminder!