Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118291
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-755-g3e1da8b93f-fm-20220708.002-g3e1da8b9
Mime-Version: 1.0
Message-ID: <b6caf617-91b3-4927-bc6f-a887d835cb46@www.fastmail.com>
In-Reply-To: 
 <CABTDVhDnC2oBqTf5O+8KY-dAUKZnWVGsRWoHY0oYEv5aj5Z+9g@mail.gmail.com>
References: 
 <CABTDVhDnC2oBqTf5O+8KY-dAUKZnWVGsRWoHY0oYEv5aj5Z+9g@mail.gmail.com>
Date: Wed, 20 Jul 2022 13:50:57 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Random Extension 5.x needs follow-up
From: larry@garfieldtech.com ("Larry Garfield")

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