As the discussion thread has been quiet for a while, moving this RFC to voting.
Hi
Looks good but missing an option to choose the default mode.
I would choose BC as default at least for one release (7.1).
I tend to vote against fixing mt_rand because of that.
As the discussion thread has been quiet for a while, moving this RFC to
voting.
Hi
Looks good but missing an option to choose the default mode.
I would choose BC as default at least for one release (7.1).
It's been that way since 5.2.1, I think it's had enough releases already :)
Hi
Looks good but missing an option to choose the default mode.
I would choose BC as default at least for one release (7.1).
It's been that way since 5.2.1, I think it's had enough releases already
:)
You mean we changed it, announce it and give a chance to adapt the code?
Given than the majority of users does not run 5.2 but 5.3+.
Hi
Looks good but missing an option to choose the default mode.
I would choose BC as default at least for one release (7.1).
It's been that way since 5.2.1, I think it's had enough releases already
:)You mean we changed it, announce it and give a chance to adapt the code?
Given than the majority of users does not run 5.2 but 5.3+.
So, I voted no then as it is clear that you do not see a problem to
break codes without a single warning or time to adapt before.
The other sections are fine and voted yes.
I would still strongly recommend to take a less harsh way. Having to
change code for such things from 7.0 to 7.1 sounds bad to me, this is
the kind of behaviors one won't expect to change without a previous
warning.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
So, I voted no then as it is clear that you do not see a problem to
break codes without a single warning or time to adapt before.The other sections are fine and voted yes.
For the part where you voted no. Still nobody has presented (publicly
available) source that makes legitimate use of mt_srand (yes it's mt_srand
that is "broken" here, not mt_rand) for deterministic streams of random
numbers. I can only assume by this that almost nobody does. However, for
those that do, they can still use the old algorithm.
For all of the other sections where you voted yes, they are mostly bug
fixes, but all change the output of mt_rand AND more functions (without a
single warning, like you wanted). I'm not trying to encourage you to change
any votes, but I need to make sure you understand what you're voting for.
So, I voted no then as it is clear that you do not see a problem to
break codes without a single warning or time to adapt before.The other sections are fine and voted yes.
For the part where you voted no. Still nobody has presented (publicly
available) source that makes legitimate use of mt_srand (yes it's mt_srand
that is "broken" here, not mt_rand) for deterministic streams of random
numbers. I can only assume by this that almost nobody does. However, for
those that do, they can still use the old algorithm.
I am sorry but this PR possibly breaks BC and cases have been clearly
explained how and why. Asking to show production code publically is not
something that you should request.
I can write a sample app to show you how but given the explanations many
gave already....
For all of the other sections where you voted yes, they are mostly bug
fixes, but all change the output of mt_rand AND more functions (without a
single warning, like you wanted). I'm not trying to encourage you to change
any votes, but I need to make sure you understand what you're voting for.
I am sorry but this PR possibly breaks BC and cases have been clearly
explained how and why. Asking to show production code publically is not
something that you should request.I can write a sample app to show you how but given the explanations many
gave already....
Just to be clear, you voted no to one BC break, but yes to other BC breaks.
I don't know how you pick which ones are acceptable, and which are not.
Summary BC breaks you voted for:
- No to changing the output of mt_rand after calling mt_srand with a given
seed (when not specifying a min/max) - Yes to changing the output of mt_rand after calling mt_srand with a given
seed (when specifying a min/max) - Yes to changing the output of rand, shuffle, str_shuffle and array_rand
Do you see why this looks weird to me?
So, I voted no then as it is clear that you do not see a problem to
break codes without a single warning or time to adapt before.The other sections are fine and voted yes.
For the part where you voted no. Still nobody has presented (publicly
available) source that makes legitimate use of mt_srand (yes it's mt_srand
that is "broken" here, not mt_rand) for deterministic streams of random
numbers. I can only assume by this that almost nobody does. However, for
those that do, they can still use the old algorithm.I am sorry but this PR possibly breaks BC and cases have been clearly
explained how and why. Asking to show production code publically is not
something that you should request.
ACK. However, it appears to me that it has not been sufficiently
verified that the random distribution of the current mt_rand()
implementation is as good as the original algorithm (apparently, there
are only some demos and quick investigations available). Therefore
fixing this in a minor version with the option to enforce the old
behavior looks good to me.
--
Christoph M. Becker
So, I voted no then as it is clear that you do not see a problem to
break codes without a single warning or time to adapt before.The other sections are fine and voted yes.
For the part where you voted no. Still nobody has presented (publicly
available) source that makes legitimate use of mt_srand (yes it's mt_srand
that is "broken" here, not mt_rand) for deterministic streams of random
numbers. I can only assume by this that almost nobody does. However, for
those that do, they can still use the old algorithm.For all of the other sections where you voted yes, they are mostly bug
fixes, but all change the output of mt_rand AND more functions (without a
single warning, like you wanted). I'm not trying to encourage you to change
any votes, but I need to make sure you understand what you're voting for.
Thanks, Leigh, for the RFC.
In my opinion, all sections except "Alias rand() to mt_rand()" and maybe
"Make array_rand() more efficient" are bug fixes, and even though
they might cause BC breaks, deploying them with a minor release seems to
be a sensible compromise.
Aliasing rand() to mt_rand() should better be postponed to PHP 8.0 (if
at all). I'm aware of rand()'s limitations and platform specific
behavior, but I'd prefer to leave the choice to nonetheless use it to
userland (at least for now). I would suggest to improve the
documentation of rand() wrt. to the note in the description section and
the warning in the notes section (which are mutually contradicting
themselves).
Assuming that "Make array_rand() more efficient" is indeed only about
performance (and not about fixes for the broken implementation), it
wouldn't be a bug fix, but as array_rand()'s behavior would change
anyway as noted by the RFC, it makes sense to do it as well.
--
Christoph M. Becker
Assuming that "Make
array_rand()more efficient" is indeed only about
performance (and not about fixes for the broken implementation), it
wouldn't be a bug fix, but asarray_rand()'s behavior would change
anyway as noted by the RFC, it makes sense to do it as well.Possibly badly worded by me. It makes the output of array_rand less
biased.
Assuming that "Make
array_rand()more efficient" is indeed only about
performance (and not about fixes for the broken implementation), it
wouldn't be a bug fix, but asarray_rand()'s behavior would change
anyway as noted by the RFC, it makes sense to do it as well.Possibly badly worded by me. It makes the output of array_rand less
biased.
Thanks for the clarification. In this case I consider it a bug fix, as
array_rand() is seriously broken on Windows.
--
Christoph M. Becker
As the discussion thread has been quiet for a while, moving this RFC to voting.
Nice work.
The discussion persuaded me (Nikita mostly) that aliasing rand() to
mt_rand() is sensible. And the compromise to fix the mt_rand() bug is
good enough. Everything else is pretty much uncontroversial, I would
think. A more efficient RNG can wait for another day.
Tom
As the discussion thread has been quiet for a while, moving this RFC to
voting.
Votes are now closed. Results as follows:
- 19-5 - Fix
mt_rand()implementation - 21-4 - Alias
rand()tomt_rand() - 25-0 - Fix RAND_RANGE()
- 23-0 - Replace insecure uses of php_rand() with php_random_bytes()
- 24-0 - Make
array_rand()more efficient
There are still a couple of tweaks to the implementation to be done before
merging:
- Make ranged output the same on 32 and 64 bit platforms where max-min is
less than 32 bits - In compatibility mode use the old RAND_RANGE for
mt_rand()
I intend to make time for these fixes tomorrow.