[RFC] [Under Discussion] Random Extension Improvement

  117994
June 17, 2022 19:41 g-kudo@colopl.co.jp (Go Kudo)
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
  117996
June 17, 2022 19:50 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/17/22 21:41, Go Kudo wrote:

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
  117997
June 17, 2022 19:53 g-kudo@colopl.co.jp (Go Kudo)
2022年6月18日(土) 4:50 Tim Düsterhus <tim@bastelstu.be>:

> Hi > > On 6/17/22 21:41, Go Kudo wrote: > > 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
  118003
June 20, 2022 04:45 g-kudo@colopl.co.jp (Go Kudo)
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
  118008
June 20, 2022 13:15 guilliam.xavier@gmail.com (Guilliam Xavier)
Hi,


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
  118010
June 20, 2022 14:12 zeriyoshi@gmail.com (Go Kudo)
2022年6月20日(月) 22:16 Guilliam Xavier 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > > Hi
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
  118011
June 20, 2022 14:36 kjarli@gmail.com (Lynn)
On Mon, Jun 20, 2022 at 3:15 PM Guilliam Xavier 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.
  118012
June 20, 2022 14:44 zeriyoshi@gmail.com (Go Kudo)
2022年6月20日(月) 23:37 Lynn <kjarli@gmail.com>:

> On Mon, Jun 20, 2022 at 3:15 PM Guilliam Xavier 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. :)
  118016
June 20, 2022 15:25 cmbecker69@gmx.de ("Christoph M. Becker")
On 20.06.2022 at 16:44, Go Kudo wrote:

> 2022年6月20日(月) 23:37 Lynn <kjarli@gmail.com>: > >> On Mon, Jun 20, 2022 at 3:15 PM Guilliam Xavier 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
  118032
June 21, 2022 09:22 guilliam.xavier@gmail.com (Guilliam Xavier)
On Mon, Jun 20, 2022 at 5:25 PM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> > On 20.06.2022 at 16:44, Go Kudo wrote: > > > 2022年6月20日(月) 23:37 Lynn <kjarli@gmail.com>: > > > >> On Mon, Jun 20, 2022 at 3:15 PM Guilliam Xavier 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
  118034
June 21, 2022 11:38 g-kudo@colopl.co.jp (Go Kudo)
2022年6月21日(火) 18:23 Guilliam Xavier xavier@gmail.com>:

> On Mon, Jun 20, 2022 at 5:25 PM Christoph M. Becker <cmbecker69@gmx.de> > wrote: > > > > On 20.06.2022 at 16:44, Go Kudo 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > 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
  118015
June 20, 2022 15:22 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/20/22 16:36, Lynn wrote:
> 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
  118013
June 20, 2022 14:57 nicolas.grekas+php@gmail.com (Nicolas Grekas)
> 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
  118014
June 20, 2022 15:12 zeriyoshi@gmail.com (Go Kudo)
2022年6月20日(月) 23:58 Nicolas Grekas 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
  118019
June 20, 2022 15:42 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/20/22 17:12, Go Kudo wrote:
>> 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
  118020
June 20, 2022 16:03 zeriyoshi@gmail.com (Go Kudo)
2022年6月21日(火) 0:42 Tim Düsterhus <tim@bastelstu.be>:

> Hi > > On 6/20/22 17:12, Go Kudo wrote: > >> 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: 1. change `Randomizer::pickArrayKey(array $array, int $num =1): int|string|array` to `Randomizer::pickArrayKeys(array $array, in $num): array`. 2. 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
  118018
June 20, 2022 15:40 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/20/22 16:57, Nicolas Grekas wrote:
> 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
  118048
June 22, 2022 14:35 zeriyoshi@gmail.com (Go Kudo)
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 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 3) *1: Added with a little sample code. Regards Go Kudo
  118052
June 22, 2022 15:02 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/22/22 16:35, Go Kudo wrote:
> 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 > 3) > > *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
  118069
June 23, 2022 11:18 g-kudo@colopl.co.jp (Go Kudo)
2022年6月23日(木) 0:02 Tim Düsterhus <tim@bastelstu.be>:

> Hi > > On 6/22/22 16:35, Go Kudo wrote: > > 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 > > 3) > > > > *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
  118114
June 28, 2022 14:29 zeriyoshi@gmail.com (Go Kudo)
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
  118116
June 28, 2022 15:39 guilliam.xavier@gmail.com (Guilliam Xavier)
> 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
  118117
June 29, 2022 01:53 g-kudo@colopl.co.jp (Go Kudo)
2022年6月29日(水) 0:39 Guilliam Xavier 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
  118118
June 29, 2022 08:40 guilliam.xavier@gmail.com (Guilliam Xavier)
>> > 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: ```diff -$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
  118119
June 29, 2022 09:25 tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)
Hi

On 6/29/22 10:40, Guilliam Xavier wrote:
> 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