Re: [RFC] [Draft] Add RNG extension and deprecate mt_srand()

On Thu, May 20, 2021 at 2:42 PM Go Kudo <zeriyoshi@gmail.com> wrote: > Sorry. I couldn't make it in time for my lunch break. > No worries, there was quite a lot to read/reply ;) > > How many bytes is Source::next() supposed to generate? Is > > it even > intended to be called directly (vs `Randomizer::generateBytes(int > $length)`)? > > No, this is not intended to be used directly. > In fact, as many people have pointed out, I think it would be more > appropriate to merge it into a single class. > > As per the original implementation (ext/orng), I didn't think it needed to > be separated originally. One of the comments on a previous RFC suggested > that it would probably be better to separate them, and I took that into > account. > I see. Sorry that you had so many back-and-forth suggestions :s I guess that the simplest-to-use API (for the main intended/expected use cases) is more likely to get acceptance. > > About the MT19937PHP variant: The PHP manual for > > mt_srand() > describes the MT_RAND_PHP mode like this: "Uses an > *incorrect* Mersenne > Twister implementation which was used as the default up till PHP 7.1.0. > This mode is available for backward compatibility."; do we really need/want > to port it to this new extension? > > This is for backward compatibility. If it is not needed, we would like to > eliminate the implementation itself. > Well, to keep the language core clean, maybe this should be provided by an > appropriate external extension. > Yes, I think that a new core extension for PHP 8.1 needn't [or even shouldn't] provide BC for an incorrect implementation that is not used anymore since PHP 7.1 (unless someone here argues?). > > - PlatformProvided feels like the "odd one out" here: > its constructor > [assuming same as previous RFC] doesn't take a seed, and it isn't > serializable; I understand why, but the main point of the RFC is "RNG > reproducibility", which this class cannot achieve. > > You're absolutely right. I would like to remove this. > *Caution though:* after your recent discussion with Rowan, if you remove the "change shuffle()/str_shuffle()/array_rand()'s internal RNG from mt_rand()-like to random_bytes()-like" part from the proposal, then this class becomes potentially "useful" again, not for reproducibility but for shuffling an array/string by a "truly random" [actually CSPRNG] algorithm, isn't it? > > > Technical, about Randomizer::generateFloat(): Is the > > returned float > guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return > -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other > floats? > Also, no generateFloat(float $min, float $max) variant like > for > generateInt()? > > As a matter of fact, we believe that the functions provided by this class > are sufficient for mt_rand(), > shuffle(), str_shuffle(), and > array_rand() equivalents. At most, the equivalent of > random_byte() > might be provided. > I'll reorganize these. > Sorry I'm not sure to understand your answer here, are you planning to remove generateFloat() and generateBool()? Well if generateBool() is a shorthand for (bool) generateInt(0, 1) [or generateInt(PHP_INT_MIN, PHP_INT_MAX) >= 0] then it's not necessary, but maybe convenient? As for generateFloat(), I didn't know if you intended something like (float) generateInt(PHP_INT_MIN, PHP_INT_MAX) / PHP_INT_MAX or rather "call generateBytes(8) then reinterpret those 64 bits as an IEEE 754 double [in C, not PHP]"? But it's true that I haven't seen many people requesting new "random_bool()" and/or "random_float()" functions, so... By the way, for generateInt() (without explicit $min/$max args), I assume that $max defaults to PHP_INT_MAX, but does $min default to PHP_INT_MIN or actually 0 [if it's like [mt_]rand() I guess that the answer is probably 0, but that could be written clear in the RFC]? > > Thanks for the detailed remarks. Based on these, I would like to clean up > the RFC. > > Regards, > Go Kudo > Regards, -- Guilliam Xavier