[RFC] [Under Discussion] Random Extension Improvement

2022年6月18日(土) 4:50 Tim Düsterhus tim@bastelstu.be:

Hi

https://wiki.php.net/rfc/random_extension_improvement

There's still a section for array_rand() at the top of the RFC
(
https://wiki.php.net/rfc/random_extension_improvement#randomizer_lacks_array_rand_replacement_method),

but no voting section.

My understanding is that you don't plan to add an array_rand()
replacement, then you should remove that section.

Best regards
Tim Düsterhus

Oops, I fixed it.

Thank you!

Best regards
Go Kudo

2022年6月20日(月) 22:16 Guilliam Xavier guilliam.xavier@gmail.com:

Hi,

https://wiki.php.net/rfc/random_extension_improvement

Thanks, but I am not sure about your argument in "Classnames are not
canonicalized": does "PHP applies strict PascalCase to class names"
(which remains to be proved) really imply to rename acronyms (e.g.
"CombinedLCG" to "CombinedLcg")? especially given existing classes
like "SimpleXMLElement" (not "SimpleXmlElement"), and that the
accepted "Class Naming" RFC (https://wiki.php.net/rfc/class-naming)
voted for "PascalCase except Acronyms" (not "Always PascalCase") --
excerpts:

| Abbreviations start with a capital letter followed by lowercase
letters, whereas acronyms and initialisms are written according to
their standard notation.

| Good:
| 'CurlResponse'
| 'HTTPStatusCode'

| Bad:
| 'curl_response'
| 'HttpStatusCode'

(Not that I really care but...)

Regards,

--
Guilliam Xavier

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi

"Class Naming" RFC (https://wiki.php.net/rfc/class-naming)

My apologies! I had missed this RFC.
I had assumed from the XmlParser implementation that it was "Always
PascalCase".
Actually, I much prefer "PascalCase except Acronyms".

I have corrected the RFC and removed the section.

Thanks!

Regards
Go Kudo

On Mon, Jun 20, 2022 at 3:15 PM Guilliam Xavier guilliam.xavier@gmail.com
wrote:

Hi,

https://wiki.php.net/rfc/random_extension_improvement

Thanks, but I am not sure about your argument in "Classnames are not
canonicalized": does "PHP applies strict PascalCase to class names"
(which remains to be proved) really imply to rename acronyms (e.g.
"CombinedLCG" to "CombinedLcg")? especially given existing classes
like "SimpleXMLElement" (not "SimpleXmlElement"), and that the
accepted "Class Naming" RFC (https://wiki.php.net/rfc/class-naming)
voted for "PascalCase except Acronyms" (not "Always PascalCase") --
excerpts:

Not specifically directed at this discussion, but perhaps this needs a
revision. HTTPStatus is much harder to read for me than HttpStatus and it's
unclear where the boundary of an acronym starts or stops. If anyone ever
decides to make an RFC for this, you have my vote. These Acronyms are
treated as words and thus should follow the same naming convention. If they
shouldn't be treated as words, write their full name:
HypertextTransferProtocolStatus.

2022年6月20日(月) 23:58 Nicolas Grekas nicolas.grekas+php@gmail.com:

An RFC has been created to fix an issue in Random Extension 5.x.

https://wiki.php.net/rfc/random_extension_improvement

Voting on this RFC will begin in two weeks (2022-07-02), in time for the
PHP 8.2 Feature Freeze. (Vote finished in 2022-07-16, Feature Freeze is
2022-07-19)

In the unlikely event that the Random Extension 5.x RFC is rejected, this
RFC will become invalid regardless of the outcome of the vote.

Hi, thanks for the update, that makes sense to me.

I'm wondering: does Random\SerializableEngine extend Random\Engine? Can
this be mentioned in the RFC? If not, what about making it this way? Having
this interface only contain __(un)serialize would look strange to me, aka
too broad for the name and the purpose.

I'm also wondering: is CombinedLCG worth it? I must admit I don't know when
I should use it instead of MT19937.

Since the names are all super opaque to many of us, documentation should be
very clear about the use case of each implementation... (if can reduce the
number of implementations, that's even better :) )

Cheers,
Nicolas

Hi

Having this interface only contain __(un)serialize would look strange to
me, aka
too broad for the name and the purpose.

Indeed. This was designed back when the Serializable interface was still
going strong,
so it is already outdated.

The OO approach to serialization no longer applies, and this may need to be
eliminated.

CombinedLCG

This is provided as an OOP implementation for the lcg_value() function,
but I don't actually
want it to be used anymore, so I probably shouldn't provide a class for it.

And to begin with, the current CombinedLCG cannot even be seeded with
arbitrary values.

However, I think it needs to remain in the internal API either way. (The
option of not providing
it to userland is a valid one.)

What do you think about the Random\CryptoSecureEngine interface?
It is just a marker interface with no methods.

However, I currently think it is better than adding a method like
isSecure(): bool
to the Random\Engine.

Best regards
Go Kudo

Hi

I'm wondering: does Random\SerializableEngine extend Random\Engine? Can
this be mentioned in the RFC? If not, what about making it this way? Having
this interface only contain __(un)serialize would look strange to me, aka
too broad for the name and the purpose.

Yes, it does. All the other interfaces extend Random\Engine as well. See
the stub at:

https://github.com/colopl/php-src/blob/upstream_rfc/scoped_rng_for_pr/ext/random/random.stub.php

I'm also wondering: is CombinedLCG worth it? I must admit I don't know when
I should use it instead of MT19937.

If the RFC passes you really should use neither:

• CombinedLCG: That's a horrible RNG, that is implemented for backwards
  compatibility with the existing functionality.
• MT19937: That one is not quite as horrible, but it comes with 2.5kB of
  state. It's also implemented for backwards compatibility with the
  existing RNGs (it's what backs rand() and mt_rand()).

Use 'Secure', you can't do anything wrong with that and that's why it's
the default for the Randomizer.

If you need performance or if you need support for seeding then either
use PCGOneseq128XslRr64 or Xoshiro256StarStar (depending on whether the
latter makes it into the extension).

Since the names are all super opaque to many of us, documentation should be
very clear about the use case of each implementation... (if can reduce the
number of implementations, that's even better :) )

Regarding the names: Yes, they are opaque, but will become a little less
opaque if the extension RFC passes, because then the names will refer to
a very specific implementation of standard RNGs, making them Googleable.

For Xoshiro256StarStar the upstream documentation is here:

https://prng.di.unimi.it/

For PCGOneseq128XslRr64 the upstream documentation is here:

https://www.pcg-random.org/

But I agree that the documentation should help the user make an educated
choice.

Best regards
Tim Düsterhus

Hi

No additional comments seemed to be forthcoming, so the RFC was upgraded to
1.5.
The following changes have been made

https://wiki.php.net/rfc/random_extension_improvement

1. Add: Refine classnames
2. Add: Random\SerializableEngine is outdated
3. Add Add Randomizer::pickArrayKeys(array $array, int $num): array method *1
4. Add Random\SerializableEngine is outdated
5. Remove: PCG64 is ambiguous (replaced by 1)
6. Remove: Mersenne Twister is ambiguous (replaced by 1)
7. Remove: Randomizer lacks array_rand() replacement method (replaced by

*1: Added with a little sample code.

Thank you for the update. The grouping makes sense to me and it looks
very organized.

Let me just propose some wording changes:

a)

Random\SerializableEngine is outdated

I would rename the headline to "Random\SerializableEngine is not
useful", that's a little more fitting.

b)

CombinedLCG is outdated

I would rename the headline to "Random\Engine\CombinedLCG is low
quality", that's a little more accurate.

c)

In the "Refine classnames" section:

To make it more readable and regular, the class name is changed as
follows:

I would reword this as:

To clearly identify the implemented algorithm the PCG64 and
MersenneTwister twister engines should be renamed to their canonical
upstream name:

The issue with the previous wording is it's not clear what "more
regular" means.

d)

For the vote titles I propose the following changes for a more
consistent wording that succinctly describes the change to avoid voter
confusion:

Engine implementations to final
to
Make all implemented engines 'final'?

Remove Random\SerializableEngine
to
Remove the SerializableEngine interface?

Drop Random\Engine\CombinedLCG
to
Remove the CombinedLCG engine?

Add Random\Randomizer::pickArrayKeys(array $array, int $num): array
to
Add the pickArrayKeys() method to the Randomizer?

Rename Random\Randomizer::shuffleString() to
Random\Randomizer::shuffleBytes()
to
Rename Randomizer::shuffleString() to Randomizer::shuffleBytes()?

Change classnames
to
Rename PCG64 and MersenneTwister?

Implement Random\Engine\Xoshiro256StarStar
to
Add the Xoshiro256StarStar engine?

Best regards
Tim Düsterhus

Hi Internals.

Random Extension 5.x has been accepted by a vote of 20(+1)/0. (I made a
mistake in timing the closing of the vote and thus received one more vote)
Therefore, voting on the Random Extension Improvement RFC will begin on
2022-07-02 as scheduled.

Please check the RFC. This is the last chance to improve the implementation.

https://wiki.php.net/rfc/random_extension_improvement

Hi,

I just realized a little thing: in the array_rand() example, for
$beforeSingle, it would probably be "more realistic" to omit , 1
(which is already the default for $num).

Note: for Randomizer::pickArrayKeys(array $array, int $num): array,
it makes sense that $num does not have a default value (1 would be
"weird" because the method always returns a list of keys, and
count($array) [via null] would be "useless" because keys are returned
in their original order [so it would make the method equivalent to
array_keys($array) by default]),
and that's probably a good thing (it forces to update the call by
adding an explicit , 1 argument and reminds to add a [0] or
similar on the returned value).

An alternative design would be Randomizer::pickArrayKey(array $array): int|string, but migrating existing uses with $num != 1 would
be harder, so probably not better.

Regards,

--
Guilliam Xavier