Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96456
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.217.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXbUmRZ8EK4HB=WyKLK5EcACXZ_wisH1v3=2ib81-HetUQ@mail.gmail.com>
References: <CAGa2bXbUmRZ8EK4HB=WyKLK5EcACXZ_wisH1v3=2ib81-HetUQ@mail.gmail.com>
Date: Wed, 19 Oct 2016 07:52:04 +0700
Message-ID: <CAEZPtU7JK=RCvyVJ6yyLHziH0Eog4pu4kXGHNTcS4gtY9Oxu-g@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Bug fix and RFC/Merge rule
From: pierre.php@gmail.com (Pierre Joye)

hi Yasuo,

On Wed, Oct 19, 2016 at 4:41 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi all,
>
> Following commit is reverted because someone requested "RFC" and/or
> demand "Merge approval from RM",

Yes, this is the rule for non obvious things. Or no matter what for
stable branches in security mode or RC phase.

> http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
>
> I don't understand the reasoning why it requires RFC nor Merge
> approval. As we all know, there are many bug fixes without RFC nor
> discussion.
>
> So let's discuss about what bug fix requires RFC and merge approval.
>
> The patch fixes "uniqid() is not unique enough" bug.
> ---------------
> ==Rationale==
> uniqid() is simply _broken_ because it does not provide expected uniqueness due
> to timestamp based php_combined_lcg().
> (large warning is added to the manual recently)
>
> unique id (time stamp) + entropy (timestamp based entropy)
>
> Problems of this implementation
>   - NTP is used to adjust system time.
>   - Result is not reasonably unique by logic.
>
> ==Resolution==
>   Replace php_combined_lcg() (Poor PRNG) to php_random_bytes() (CSPRNG).
>
>   Patch
>   http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
>   (less than 30 lines of diff)
>
> ==BC impact==
>   There is no BC.  php_random_bytes() raises exception for bad CSPRNG,
>   but this very unlikely and such system shouldn't be used anyway.
> ---------------
>
> Discussions in previous thread
> ---------------
> =="new uniqid() can cause /dev/urandom cannot read error"==
>
> This is FUD. Almost all "/dev/urandom cannot read" issues on the net
> is due to "open_basedir" restriction. "open_basedir" is nothing to do
> with internal functions, so the discussion is FUD.
>
> =="Any new error is BC so the patch should be reverted"==
>
> We do have many bug fixes emit new errors. We don't revert them at all
> even when it's very common and annoying.
>
> Example:  https://bugs.php.net/bug.php?id=73238
> This bug fix made WordPress display few additional E_WARNING errors
> that can be remove by php.ini or code fix.
>
> =="Someone explicitly requested RFC/Merge approval, so it should be reverted"==
>
> IMHO, the patch is very simple patch only fixes problem. Many bug
> fixes include more severe BC issues than the patch, behavior changes
> and raised errors. Yet, no RFC nor discussions.
>
> If someone requested RFC/Merge request for very simple patch, should
> we follow always?
>
> =="It does not have to hurry"==
>
> I agree with this. However, reverting simple bug fix patch does not make sense.
> --------------
>
> Question is:
>   - What kind of bug fix requires RFC?

No behavior change, strong consensus, no BC break. But really a case
by case question.

>   - What kind of bug fix requires discussion and approval to merge
> released versions?

Same answer.

>
> After all, my question is "Should we discuss all bugs before commits?"
> "The revert is valid and reasonable?"

The main point is what is a bug, what is a BC break. And we had many
different opinions in the past. Even in this case where "this system
should not be used anyway" makes me wonder.

Like your session change, while not really critical to me (has been
like that for ages) is relatively logical while as a RM, I would not
merge it in 7.1 at this stage (RC).

Cheers,
Pierre