Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117302
MIME-Version: 1.0
References: <CAMs36Hco05vXydtM8qUfpcDkj6oKYufifAr-gOr573OwmVtimA@mail.gmail.com>
 <cab85ccc-bef9-43b2-a967-e91553022761@www.fastmail.com>
In-Reply-To: <cab85ccc-bef9-43b2-a967-e91553022761@www.fastmail.com>
Date: Fri, 11 Mar 2022 20:18:52 +0900
Message-ID: <CAMs36Hc==0qL=fxkx9BO6UGoM8hyh0cBAE6=EZtzu-zHJb68PA@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary="00000000000088724405d9ef7f7a"
Subject: Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 5.x
From: g-kudo@colopl.co.jp (Go Kudo)

--00000000000088724405d9ef7f7a
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2022=E5=B9=B43=E6=9C=8810=E6=97=A5(=E6=9C=A8) 3:07 Larry Garfield <larry@ga=
rfieldtech.com>:

> On Wed, Mar 9, 2022, at 4:48 AM, Go Kudo wrote:
> > Hello internals.
> >
> > Proposed RFCs and implementations have been reorganized. The main chang=
es
> > are as follows
> >
> > https://wiki.php.net/rfc/rng_extension
> > https://github.com/php/php-src/pull/8094
> >
> > - Remove XorShift128Plus, Xoshiro256StarStar
> >     - To avoid confusion, Can be added if needed
> > - All classes can now omit the seed argument
> >     - Seeding with CSPRNG
> > - Throws RuntimeException if number generation fails
> >
> > The merge window to PHP 8.2 will remain open for some time to come. Due
> to
> > the global turmoil, I will try to delay the start of the RFC voting pha=
se
> > as long as possible.
> >
> > However, I believe that PHP RFCs tend to become more controversial once
> the
> > voting phase begins.
> >
> > Therefore, I would like to solicit feedback on this RFC, whatever it ma=
y
> > be. (Just only positive or negative feedback would be okay)
> >
> > Regards,
> > Go Kudo
>
> This looks good to me overall, although I have a few comments.
>
> * There's several places where a sentence or portion of a sentence repeat=
s
> itself.  I assume this is just an editing error.
>
> * The list headers "implemented of" doesn't really make grammatical
> sense.  I think you mean "implemented by"?
>
> * The last section "PRNG Shootout" seems to imply that you're only
> implementing one algorithm, but it doesn't say which one.  The earlier
> sections, though, show several algorithms that you are implementing.
> Again, I assume this is an editing error from many revisions but it's
> confusing as is.
>
> * If no engine is provided to the randomizer, what gets used?  If the
> default is listed somewhere in the RFC, I missed it.  I'm assuming there
> should be one, for usability, but changing it in the future would be an
> RFC-level change (much like changing the defaults for password_hash()).
>
> * The example of using different algorithms in different environments
> needs much higher billing.  You're burying the lead there.  The ability t=
o
> have a bad but predictable random seed for tests but then a CSPRNG for
> production is *huge*.  The approach is similar to the nearly-done PSR-20
> spec in FIG, which does essentially the same thing for the "current time"
> routines, albeit in userspace.  I would highlight this benefit way more, =
as
> it's probably the biggest benefit from these changes that most developers
> will see and has me more excited about this RFC than anything else in the
> entire document.
>
> * How do the existing random functions interact with the new API?  Do the=
y
> shift to be shells over the new classes with an internal shared global, o=
r
> something else?
>
> * A side effect of the current design is that SeedableEngine is just a
> marker interface; there's no actual standardized way to set the seed.  Th=
e
> examples imply that passing it as a constructor is the recommended way (a=
nd
> I agree it probably should be), but that's not explicit.  I'm also unclea=
r
> then what value the marker interface is, then.  Does the randomizer do
> something different with a seedable engine?  I'm not against the current
> design, but its explanation/implications could be improved.
>
> Looking forward to this passing in a few weeks!
>
> --Larry Garfield
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>
Hi  Larry,

I have attempted to correct what you pointed out. However, I don't think
the RFC is complete at this point. We will work on correcting this over the
next few days.

I'll answer a few questions. (These have been corrected in the RFC)

> If no engine is provided to the randomizer, what gets used?  If the
default is listed somewhere in the RFC, I missed it.  I'm assuming there
should be one, for usability, but changing it in the future would be an
RFC-level change (much like changing the defaults for password_hash()).

As Tim has answered, Random\Engine\Secure is automatically generated and
used.

> The example of using different algorithms in different environments needs
much higher billing

This is a very important aspect, but also a side effect of the current
problem solving. I am very concerned about how to appeal to this.
For the time being, I have added a sentence to the Proposal.

> How do the existing random functions interact with the new API

As BC break is none, it does not affect existing APIs. The internal
implementation will be changed.
This is to avoid redundancy in implementation.

https://github.com/php/php-src/pull/8094/files#diff-8ba10cd3f979cdab4491a2d=
02149ff10638b88854e79664748b3ab620fd3f1bfR255-R259

I am very concerned about how to make the RFC easier to read.
I Will continue to work on improving it.

Regards
Go Kudo

--00000000000088724405d9ef7f7a--