Re: Random Extension 5.x needs follow-up

On Wed, Jul 20, 2022, at 9:24 AM, Go Kudo wrote: > Hi Internals. > > The Random Extension 5.x and Random Extension Improvement RFCs have been > passed and recently merged into the master branch. However, the proposal is > still problematic and implementation fixes are needed. > > Currently, the mt_rand() function is overloading its arguments, and I have > designed a Randomizer::getInt() signature based on this. However, argument > overloading has several problems, including reflection, and is now not > recommended. > > I wish I had been aware of it during the RFC proposal, but that did not > happen. > > To solve the problem, I thought of separating the argumentless > Randomizer::getInt() as > Randomizer::nextInt(). > Randomizer::getInt() > must require int $min, int $max arguments. > > I have received support for this proposal from several people, but there > will be a discrepancy between the RFC and the implementation. > > I believe this should be corrected to avoid future BC Break, what do you > think about this? > > Discussions: > - https://externals.io/message/118163#118269 > - https://github.com/php/php-src/pull/8094/files#r919693108 > (just scroll a > little please) > > Fix PR: https://github.com/php/php-src/pull/9057 > > Best Regards > Go Kudo This is ultimately for the branch maintainers to decide. I would personally favor fixing the signature now, even post-freeze; the whole point of betas is to find problems and edge cases while they're still semi-fixable, and this sounds like one. Splitting it into getInt(int $min, int $max) and nextInt() would also be acceptable for me; if the branch maintainers are OK with it, I would favor that as methods doing double-duty is just all around bad news. If not, handling it like touch() is the next-best option. --Larry Garfield