Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114680
MIME-Version: 1.0
References: <CABTDVhDRH8+0WG0nJ5QRYVT9-8m1u2VMe-gvY5cWra-OKQ4t+g@mail.gmail.com>
 <CABTDVhBrpNjhN00fYY3iktW_XX1pTeJPQEdzc72qGOO2XXXknw@mail.gmail.com> <CABTDVhAP55PT4x61MT7KotuiLO3=AAban7_7cL-eOT5EQE2CTA@mail.gmail.com>
In-Reply-To: <CABTDVhAP55PT4x61MT7KotuiLO3=AAban7_7cL-eOT5EQE2CTA@mail.gmail.com>
Date: Tue, 1 Jun 2021 12:43:16 +0200
Message-ID: <CAF+90c8n-ouuzeG=S67jUDeW=Ss__d6_jiH=+ANjL3vWnDijbw@mail.gmail.com>
To: Go Kudo <zeriyoshi@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000706c8b05c3b203c8"
Subject: Re: [PHP-DEV] Re: [RFC] [Draft] Add Randomizer class (before: Add RNG extension)
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000706c8b05c3b203c8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 31, 2021 at 4:04 PM Go Kudo <zeriyoshi@gmail.com> wrote:

> Hi Internals.
>

Hi :) Thanks for your continued work on this RFC. This is a tough one, with
a lot of feedback, some of it contradictory. I'm still going to add my 2c..=
.

I think that this proposal can basically work out in two ways:

1. Pragmatic: Have a Random class that represents both the raw randomness
source and ways to use it.
2. Pure: Separate the randomness source from transformations based on it.

Your current design sits in a weird middle ground between these, where you
can extend the Random class to insert a user-provided randomness source.

I think you should pick between one or the other option. If you pick the
first option, then user-extensibility should simply not be supported at
all. The class should be final, and only support a limit set of
extension-provided RNGs. This is a pragmatic design that covers most
use-cases.

If you pick the second option, then you should consistently separate the
source for both extension-provided RNGs and user-provided ones. I don't
think it makes sense to have extension provided RNGs use a `new
Random(RANDOM_XORSHIFT128PLUS)` interface, while user-provided ones use
`new Random(new SomeRandomSource)`. Going down this route, it should be
`new Random(new XorShift128Plus)`. This is a pure design, which also means
that it's more complicated, and may make simple usages harder (though there
is certainly room for a Random::createDefault() here.)

As a minor note, I don't think we should add something like
random.ignore_generated_size_exceeded. Our general policy is to avoid the
introduction of ini settings that affect runtime behavior where possible.
We should pick one of the behaviors here and stick with it.

Regards,
Nikita

I apologize for the discussion outside the ML. Here's a brief history.
>
> https://github.com/php/php-src/pull/7079
>
> - To test the implementation, I sent a Pull-Request to php-src on GitHub.
> - An excellent point was made, and we ended up having a discussion about =
it
> on GitHub.
> - I was going to change the current implementation RFC to Under Discussio=
n,
> but decided against it.
>
> Also, sorry for the delay in answering your questions. I am not very good
> at English and it is taking me a long time.
>
> The current implementation and RFC:
>
> - https://github.com/php/php-src/pull/7079
> - https://wiki.php.net/rfc/rng_extension
>
> However, the implementation of this user-defined class is very ugly and w=
e
> intend to improve it as follows.
>
> Before:
> ```php
>
> class Random
> {
>     public function __construct(string $algo =3D RANDOM_XORSHIFT128PLUS, =
?int
> $seed =3D null) {}
>     //....
> }
>
> class UserDefinedRandom extends Random
> {
>     protected function next(): int
>     {
>         return 1;
>      }
> }
> ```
>
> After:
> ```php
> interface RandomNumberGenerator
> {
>     public function generate(): int;
> }
>
>
> final class Random
> {
>     public function __construct(string|RandomNumberGenerator $algo =3D
> RANDOM_XORSHIFT128PLUS, ?int $seed =3D null) {}
> }
> ```
>
> 2021=E5=B9=B45=E6=9C=8826=E6=97=A5(=E6=B0=B4) 0:35 Go Kudo <zeriyoshi@gma=
il.com>:
>
> > Hi, Thanks for the response.
> >
> > The RFC has been revised based on the points you pointed out.
> >
> > https://wiki.php.net/rfc/rng_extension
> >
> > The main changes are as follows:
> >
> > - Class name has been changed from `Randomizer` to `Random` .
> > - Added a static method `getNonBiasedMax()` to get the safe range.
> > - `int()` and `bytes()` have been renamed to `getInt()` and `getBytes()=
`
> > for avoid future reserved words.
> > - `getInt()` arguments no longer accept null.
> > - `shuffle(array|string $target): array|string` has been separated into
> > `arrayShuffle(array $array)` and `stringShuffle(string $string): string=
`
> > for more comfortable static-analysis.
> > - fix: php_random_algo struct (included uint64_t -> int64_t)
> >
> > Answer a few questions:
> >
> > > When $seed is null, what is used for the seed value?
> >
> > Depends on the algo's implementation, but basically it is using interna=
l
> > `php_random_int()`.
> > It is similar to `mt_srand()` on PHP 8.1.
> >
> >
> >
> https://github.com/php/php-src/commit/53ee3f7f897f7ee33a4c452100146480433=
86e13
> >
> > > Why cancelled RNG Extension?
> >
> > As a result of discussions during the draft, the functions became a
> single
> > class and no longer need to be separated. The functionality for random
> > numbers is now included in ext/standard and will conform to this. (e.g.
> > rand.c random.c)
> >
> > > Deprecation
> >
> > Dropped in the last RFC update.
> > These were premature and should not have been included with the RFCs th=
at
> > add new features.
> >
> > If the direction seems generally okay, I'd like to start implementing i=
t
> > to show more details.
> >
> > Regards,
> > Go Kudo
> >
> > 2021=E5=B9=B45=E6=9C=8823=E6=97=A5(=E6=97=A5) 5:56 Go Kudo <zeriyoshi@g=
mail.com>:
> >
> >> Hi, Internals and all participated in the previous discussion.
> >>
> >> RFCs have been cleaned up and the proposal has been substantially
> changed.
> >>
> >> https://wiki.php.net/rfc/rng_extension
> >>
> >> First of all, I apologize for not checking out the implementation of
> >> password_hash(), which is a precedent to learn from.
> >>
> >> I think I've answered all the questions I've been getting on Open
> Issues.
> >> If I have left anything out, please let me know.
> >>
> >> Regards,
> >> Go Kudo
> >>
> >
>

--000000000000706c8b05c3b203c8--