Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96457
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: <CAGa2bXacjycHuCedWBtfxouWzUsDkWBu0-u63zS3KM3hQWBZiw@mail.gmail.com>
References: <CAGa2bXYq=656z9eq-b=agpqJmojDn+DpDt1zfLLc_QTcFcLt8g@mail.gmail.com>
 <CAGa2bXacjycHuCedWBtfxouWzUsDkWBu0-u63zS3KM3hQWBZiw@mail.gmail.com>
Date: Wed, 19 Oct 2016 07:54:38 +0700
Message-ID: <CAEZPtU5Cfm0ASL20d1286kGGas-BJrb2oeooc2efymw0puBNEA@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Davey Shafik <davey@php.net>, "internals@lists.php.net" <internals@lists.php.net>, 
	Joe Watkins <pthreads@pthreads.org>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Re: Fixing insane session_start() behaviors
From: pierre.php@gmail.com (Pierre Joye)

Hi Yasuo,

On Mon, Oct 17, 2016 at 4:47 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Davey,
>
> On Sun, Oct 16, 2016 at 6:08 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>>
>> I was planning to fix session_start() behaviors by PHP 7.1, but I
>> forgot to do this completely. Partial fix is merged currently.
>>
>> Following PR makes session_start() return FALSE when it cannot start
>> session always.
>>
>> https://github.com/php/php-src/pull/2167
>>
>> In short, this patch fixes number of session_start()'s insane behaviors.
>
> I think I've finished the patch for now.
>
> There are many other places that I would like to improve/fix error
> handlings. e.g. Session save handlers. This patch is too large
> already. I'll fix them in master.
>
> The patch could be divided into "session_start() error handling fix
> only" and "other session core error handling issues". It may be good
> to include "session_start() error handling fix only" for PHP 7.1, but
> other fixes won't bother correct PHP scripts. Please note that
> session_start() fix in PHP-7.1 and master branch is partial fix. I
> suggest to merge "session_start() error handling fix only" at least,
> but it's your choice.
>
> Please let me know when you decide what to do or merge appropriate
> patch from the PR.
>
> BTW, I expect UnitTest failures, framework/etc, that test session
> module functionalities. They have tests for failure cases. Even if
> there are minor BC issues (UnitTests report error, bad code emit
> errors), this fix must be done at some point anyway.

I like this change as it adds some cleanup and common sense to this
API. However 7.1 is in RC, this patch, while covering logical changes,
affect behaviors and is quite big (lot of similar changes but still).
I would feel much more comfortable to push it after 7.1 (as in, for
7.2 and later).

Cheers,
-- 
Pierre

@pierrejoye | http://www.libgd.org