Skip to content

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

Merged
kocsismate merged 4 commits intophp:masterfrom
kocsismate:gh-10292-default
Jan 20, 2023
Merged

Fix GH-10292 make the default value of the first param of srand() and mt_srand() nullable#10380
kocsismate merged 4 commits intophp:masterfrom
kocsismate:gh-10292-default

Conversation

@kocsismate
Copy link
Member

@kocsismate kocsismate commented Jan 19, 2023

No description provided.

@kocsismate kocsismate linked an issue Jan 19, 2023 that may be closed by this pull request
srand(0) set random seed to 0 instead random #10292
Closed
TimWolla
TimWolla reviewed Jan 19, 2023
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

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

@kocsismate @TimWolla
Update ext/random/random.c
3da0bf5 
Co-authored-by: Tim Düsterhus <[email protected]>
zeriyoshi
zeriyoshi approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@zeriyoshi zeriyoshi left a comment

Choose a reason for hiding this comment

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

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

zeriyoshi commented Jan 20, 2023

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

kocsismate commented Jan 20, 2023

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
cmb69 approved these changes Jan 20, 2023
View reviewed changes
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!

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

TimWolla commented Jan 20, 2023

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

kocsismate commented Jan 20, 2023

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

Perfect!

@kocsismate
Copy link
Member Author

kocsismate commented Jan 20, 2023

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

TimWolla commented Jan 20, 2023

P.S. Is the upgrading note ok?

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

@TimWolla
Copy link
Member

TimWolla commented Jan 20, 2023

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 kocsismate changed the title Fix GH-10292 make the default value of mt_srand() nullable Fix GH-10292 make the default value of the first parame of srand() and mt_srand() nullable Jan 20, 2023
@kocsismate
Copy link
Member Author

kocsismate commented Jan 20, 2023 •
edited
Loading

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
TimWolla approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@kocsismate kocsismate merged commit 1f05d6e into php:master Jan 20, 2023
@kocsismate kocsismate deleted the gh-10292-default branch January 20, 2023 22:35
@TimWolla TimWolla changed the title Fix GH-10292 make the default value of the first parame of srand() and mt_srand() nullable Fix GH-10292 make the default value of the first param of srand() and mt_srand() nullable Jan 20, 2023
@TimWolla
Copy link
Member

TimWolla commented Jan 22, 2023

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

kocsismate commented Jan 22, 2023

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

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

@kocsismate kocsismate mentioned this pull request Jan 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TimWolla TimWolla TimWolla approved these changes

@cmb69 cmb69 cmb69 approved these changes

@zeriyoshi zeriyoshi zeriyoshi approved these changes

Assignees

No one assigned

Labels

Extension: random

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

srand(0) set random seed to 0 instead random

4 participants

@kocsismate @zeriyoshi @TimWolla @cmb69