[RFC] [Under Discussion] Random Extension Improvement

2 years ago by tim@bastelstu.be — view source

unread

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

2 years ago by Go Kudo — view source

unread

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

2 years ago by Go Kudo — view source

unread

2022年6月18日(土) 4:41 Go Kudo g-kudo@colopl.co.jp:

Hi internals.

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.

Best regards
Go Kudo

Hello.

The new name for PCG64 was not appropriate (Pcg64OneseqXslRr64) and was
changed to something more appropriate (PcgOneseq128XslRr64).

Best regards.
Go Kudo

2 years ago by Guilliam Xavier — view source

unread

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

2 years ago by Go Kudo — view source

unread

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

2 years ago by Lynn — view source

unread

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.

2 years ago by Go Kudo — view source

unread

2022年6月20日(月) 23:37 Lynn kjarli@gmail.com:

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.

I support "PascalCase except Acronyms" for readability, but would like to
see this
clarified as I get very lost when implementing new features.
I think it is necessary because I expect various OO APIs will be added in
the future,
like cURL.

But, I do not have the right to vote. :)

2 years ago by Christoph M. Becker — view source

unread

2022年6月20日(月) 23:37 Lynn kjarli@gmail.com:

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

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.

I support "PascalCase except Acronyms" for readability, but would like to
see this
clarified as I get very lost when implementing new features.
I think it is necessary because I expect various OO APIs will be added in
the future,
like cURL.

In my opinion, https://wiki.php.net/rfc/class-naming was a bit
unfortunate. It may have been better to decide on a case by case basis.

For instance, we have introduced several Curl* classes in PHP 8.0[1],
and these adhere to the appropriate example in the RFC, although CURL is
clearly an acronym[2], and the canonical spelling is even cURL. Maybe
even worse, the previously introduced CURLFile[3] uses different
capitalization, and CURLStringFile[4] which was introduced in PHP 8.1 is
aligned to that spelling.

So, obviously, the RFC didn't have a good impact on some of the namings
so far.

[1] https://www.php.net/curl
[2] https://curl.se/docs/faq.html#What_is_cURL
[3] https://www.php.net/manual/en/class.curlfile.php
[4] https://www.php.net/manual/en/class.curlstringfile.php

--
Christoph M. Becker

2 years ago by Guilliam Xavier — view source

unread

2022年6月20日(月) 23:37 Lynn kjarli@gmail.com:

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

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.

I support "PascalCase except Acronyms" for readability, but would like to
see this
clarified as I get very lost when implementing new features.
I think it is necessary because I expect various OO APIs will be added in
the future,
like cURL.

In my opinion, https://wiki.php.net/rfc/class-naming was a bit
unfortunate. It may have been better to decide on a case by case basis.

For instance, we have introduced several Curl* classes in PHP 8.0[1],
and these adhere to the appropriate example in the RFC, although CURL is
clearly an acronym[2], and the canonical spelling is even cURL. Maybe
even worse, the previously introduced CURLFile[3] uses different
capitalization, and CURLStringFile[4] which was introduced in PHP 8.1 is
aligned to that spelling.

So, obviously, the RFC didn't have a good impact on some of the namings
so far.

That seems unfortunate indeed :/ and there are others, e.g.
"XMLReader" but "XmlParser"... Moreover, I see that
https://github.com/php/php-src/blob/master/CODING_STANDARDS.md#user-functionsmethods-naming-conventions
item 7 only says "should", not "must".

I too find "HttpStatus" [or "HTTP_status", for that matter] more
readable than "HTTPStatus" (where I first see "HTTPS" before noticing
that the S actually starts a second word), and "PcgOneseq128XslRr64"
than "PCGOneseq128XSLRR64" (especially if not already familiar with
"PCG" and "XSL-RR")...

So, it may indeed be OK (and preferred?) to "canonicalize" the
proposed class names (just, "PHP applies strict PascalCase to class
names" wasn't a good argument for it).

PS @Go Kudo: please don't be offended, but in general, maybe you
should wait "a bit more" [or even ask] for other opinions, rather than
changing the RFC "too fast" after only one (especially when I said "I
am not sure" and asked a question)

Regards,

--
Guilliam Xavier

2 years ago by Go Kudo — view source

unread

2022年6月21日(火) 18:23 Guilliam Xavier guilliam.xavier@gmail.com:

On Mon, Jun 20, 2022 at 5:25 PM Christoph M. Becker cmbecker69@gmx.de
wrote:

2022年6月20日(月) 23:37 Lynn kjarli@gmail.com:

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

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.

I support "PascalCase except Acronyms" for readability, but would like
to
see this
clarified as I get very lost when implementing new features.
I think it is necessary because I expect various OO APIs will be added
in
the future,
like cURL.

In my opinion, https://wiki.php.net/rfc/class-naming was a bit
unfortunate. It may have been better to decide on a case by case basis.

For instance, we have introduced several Curl* classes in PHP 8.0[1],
and these adhere to the appropriate example in the RFC, although CURL is
clearly an acronym[2], and the canonical spelling is even cURL. Maybe
even worse, the previously introduced CURLFile[3] uses different
capitalization, and CURLStringFile[4] which was introduced in PHP 8.1 is
aligned to that spelling.

So, obviously, the RFC didn't have a good impact on some of the namings
so far.

That seems unfortunate indeed :/ and there are others, e.g.
"XMLReader" but "XmlParser"... Moreover, I see that

https://github.com/php/php-src/blob/master/CODING_STANDARDS.md#user-functionsmethods-naming-conventions
item 7 only says "should", not "must".

I too find "HttpStatus" [or "HTTP_status", for that matter] more
readable than "HTTPStatus" (where I first see "HTTPS" before noticing
that the S actually starts a second word), and "PcgOneseq128XslRr64"
than "PCGOneseq128XSLRR64" (especially if not already familiar with
"PCG" and "XSL-RR")...

So, it may indeed be OK (and preferred?) to "canonicalize" the
proposed class names (just, "PHP applies strict PascalCase to class
names" wasn't a good argument for it).

PS @Go Kudo: please don't be offended, but in general, maybe you
should wait "a bit more" [or even ask] for other opinions, rather than
changing the RFC "too fast" after only one (especially when I said "I
am not sure" and asked a question)

Regards,

--
Guilliam Xavier

--

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

Hi

It's a difficult problem...

At least in this thread, "Always PascalCase" seems to be more favored.
(I know this is probably not the best way to put it, but I can't think of
any other analogy...)

I am wondering how to write to the RFC, but if there are no problems, I
will make the following changes.

Change to Randomizer::pickArrayKey(array $array, int $num = 1): int|string|array -> Randomizer::pickArrayKeys(array $array, int $num): array
Apply "Always PascalCase" based on ML's opinion

PS

Thanks for the advice. I am not very familiar with the OSS community, so it
helps a lot.

Regards
Go Kudo

2 years ago by tim@bastelstu.be — view source

unread

Hi

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.

Yes, I agree here. I switched to ucfirst(strtolower()) acronyms a while
ago and find those much more readable. It also avoids issues when there
are multiple acronyms within a single identifier (think PCGRNG vs PcgRng).

Best regards
Tim Düsterhus

2 years ago by Nicolas Grekas — view source

unread

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

2 years ago by Go Kudo — view source

unread

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

2 years ago by tim@bastelstu.be — view source

unread

Hi

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.)

I wouldn't object to dropping CombinedLCG, especially since its internal
parameters are not defined via the name (contrary to MT19937).

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.

I much prefer the marker interface.

Best regards
Tim Düsterhus

2 years ago by Go Kudo — view source

unread

2022年6月21日(火) 0:42 Tim Düsterhus tim@bastelstu.be:

Hi

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.)

I wouldn't object to dropping CombinedLCG, especially since its internal
parameters are not defined via the name (contrary to MT19937).

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.

I much prefer the marker interface.

Best regards
Tim Düsterhus

Hi

Added option to discontinue CombinedLCG.

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

I am struggling with the following issue:

change Randomizer::pickArrayKey(array $array, int $num =1): int|string|array
to Randomizer::pickArrayKeys(array $array, in $num): array.
Change the class name to "Always PascalCase" or "PascalCase except
Acronyms".

I hope I can solve the discussion by e-mail, but if I can't, I will further
add it to the RFC options.

Best regards,
Go Kudo

2 years ago by tim@bastelstu.be — view source

unread

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

2 years ago by Go Kudo — view source

unread

2022年6月18日(土) 4:42 Go Kudo g-kudo@colopl.co.jp:

Hi internals.

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.

Best regards
Go Kudo

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

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

*1: Added with a little sample code.

Regards
Go Kudo

2 years ago by tim@bastelstu.be — view source

unread

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

Add: Refine classnames

Add: Random\SerializableEngine is outdated

Add Add Randomizer::pickArrayKeys(array $array, int $num): array method *1

Add Random\SerializableEngine is outdated

Remove: PCG64 is ambiguous (replaced by 1)

Remove: Mersenne Twister is ambiguous (replaced by 1)

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

2 years ago by Go Kudo — view source

unread

2022年6月23日(木) 0:02 Tim Düsterhus tim@bastelstu.be:

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

Add: Refine classnames

Add: Random\SerializableEngine is outdated

Add Add Randomizer::pickArrayKeys(array $array, int $num): array method *1

Add Random\SerializableEngine is outdated

Remove: PCG64 is ambiguous (replaced by 1)

Remove: Mersenne Twister is ambiguous (replaced by 1)

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 Tim

Thanks for the suggestion! It looked much better to me and I have reflected
it in the RFC.

"Remove the CombinedLCG engine?" replaced by "Remove the CombinedLCG
class?". The reason is that the implementation will still remain for
backward compatibility. (only the class will be removed).

Regards
Go Kudo

2 years ago by Go Kudo — view source

unread

2022年6月18日(土) 4:42 Go Kudo g-kudo@colopl.co.jp:

Hi internals.

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.

Best regards
Go Kudo

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

Best regards
Go Kudo

2 years ago by Guilliam Xavier — view source

unread

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

2 years ago by Go Kudo — view source

unread

2022年6月29日(水) 0:39 Guilliam Xavier guilliam.xavier@gmail.com:

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

This is certainly a complicated issue.

I proposed the signature Randomizer::arrayPickKeys(array $array, int $num): array because it can be solved with the current PHP sugar syntax
and the default value of $num is 1 despite the name "arrayPickKeys".

However, this is a bit tricky and may not be user-friendly for the average
user.

So, how about adding two methods, Randomizer::arrayPickKey(array $array): int|string and Randomizer::arrayPickKeys(array $array, int $num): array?

This may seem redundant, but it may avoid user confusion.

Regards
Go Kudo

2 years ago by Guilliam Xavier — view source

unread

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

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.

This is certainly a complicated issue.

I proposed the signature Randomizer::arrayPickKeys(array $array, int $num): array because it can be solved with the current PHP sugar syntax and the default value of $num is 1 despite the name "arrayPickKeys".

However, this is a bit tricky and may not be user-friendly for the average user.

So, how about adding two methods, Randomizer::arrayPickKey(array $array): int|string and Randomizer::arrayPickKeys(array $array, int $num): array?

This may seem redundant, but it may avoid user confusion.

Sorry if I wasn't clear: I just suggested to make this little change
in the example:

-$beforeSingle = array_rand(['foo' => 'foo', 'bar' => 'bar', 'baz' =>
'baz'], 1); // (string) foo
+$beforeSingle = array_rand(['foo' => 'foo', 'bar' => 'bar', 'baz' =>
'baz']); // (string) foo

to make it more "realistic".

As concerns the rest (about pickArrayKeys): sorry for the digression,
I was just "thinking out loud", I don't want any change there
(first, it makes sense that pickArrayKeys has int $num without a
default value [even if array_rand has 1]; second, "pickArrayKey" [if
really wanted] is trivial to implement in userland as a wrapper around
pickArrayKeys [but the opposite would not be so], and I don't think
that adding both methods to Randomizer is desirable either [better
keep it simple/minimal]).

Regards,

--
Guilliam Xavier

2 years ago by tim@bastelstu.be — view source

unread

Hi

pickArrayKeys [but the opposite would not be so], and I don't think
that adding both methods to Randomizer is desirable either [better
keep it simple/minimal]).

Agreed. I think it's best to leave pickArrayKeys() as it is within the
current version of the RFC and NOT add pickArrayKey(). It's easy-ish to
add new stuff. It's hard to take stuff away.

Best regards
Tim Düsterhus

2 years ago by Go Kudo — view source

unread

2022年6月29日(水) 17:40 Guilliam Xavier guilliam.xavier@gmail.com:

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

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.

This is certainly a complicated issue.

I proposed the signature Randomizer::arrayPickKeys(array $array, int $num): array because it can be solved with the current PHP sugar syntax
and the default value of $num is 1 despite the name "arrayPickKeys".

However, this is a bit tricky and may not be user-friendly for the
average user.

So, how about adding two methods, Randomizer::arrayPickKey(array $array): int|string and Randomizer::arrayPickKeys(array $array, int $num): array?

This may seem redundant, but it may avoid user confusion.

Sorry if I wasn't clear: I just suggested to make this little change
in the example:
-$beforeSingle = array_rand(['foo' => 'foo', 'bar' => 'bar', 'baz' =>
'baz'], 1); // (string) foo
+$beforeSingle = array_rand(['foo' => 'foo', 'bar' => 'bar', 'baz' =>
'baz']); // (string) foo
to make it more "realistic".

As concerns the rest (about pickArrayKeys): sorry for the digression,
I was just "thinking out loud", I don't want any change there
(first, it makes sense that pickArrayKeys has int $num without a
default value [even if array_rand has 1]; second, "pickArrayKey" [if
really wanted] is trivial to implement in userland as a wrapper around
pickArrayKeys [but the opposite would not be so], and I don't think
that adding both methods to Randomizer is desirable either [better
keep it simple/minimal]).

Regards,

--
Guilliam Xavier

--

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

Hi

Thank you. That feels true.
I will try to keep the RFC as it currently stands.

Sorry for the delay in replying. I was a little held up with personal
business.
I will delay the start of voting by one day.

Best regards
Go Kudo