Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96474
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1d22f996-27b6-35bb-03a6-df3acf9e33d4@gmail.com>
References: <CAGa2bXYq=656z9eq-b=agpqJmojDn+DpDt1zfLLc_QTcFcLt8g@mail.gmail.com>
 <CAGa2bXaGg0PocgtJEQ2Xhi6Us4F=ncEN-6zhZKEjGE1UuSoQ5A@mail.gmail.com> <1d22f996-27b6-35bb-03a6-df3acf9e33d4@gmail.com>
Date: Wed, 19 Oct 2016 20:57:25 +0900
Message-ID: <CAGa2bXZ3ehA-ufXn68B2W-vVXRc1s9JKEFviFyWKDn2O++iKRw@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>
Cc: Davey Shafik <davey@php.net>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Re: Fixing insane session_start() behaviors
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Stas,

On Wed, Oct 19, 2016 at 3:08 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> I pushed patch fixes number of nonsense/inconsistent session function
>> behaviors. The additional patch is pushed so that it's easy to cherry
>> pick minimum fixes. The last push is the additional fixes.
>
> These changes look like a reasonable cleanup. I'm not a big fan of
> zend_parse_parameters_none additions, since I don't see any useful
> function for them, but they don't hurt either. I left some notes on the
> patch though, please look there.
>
> I'm not sure about 7.1 - while I don't see anything that would break any
> reasonable code, except issues noted on the pull, the patch is pretty
> big and I might have missed something. I guess it's for RM to decide. No
> objection for 7.2.
>
> I notice that most test changes deal with scenarios that were covered
> before, are there tests that test the new things - the scenarios for
> which handling has changed? I couldn't find tests for some cases, like
> INI modification, would be nice to add them.
>
> Also, a reminder that you'd have to update all the documentation in the
> manual to reflect the change in the return values. And UPGRADING too.

Thank you.
I replied to your comments.

There is one debatable comment regarding session_cache_limiter().
We may delay error check at 'headers sent'. I suppose almost all usage
is misuse.

Only valid use case is

<?php
ob_start();
session_start();
session_set_cache_limiter('public'); // <== Call this between
session_start() and session_regenerate_id()
session_regenerate_id();
?>

Other than this, all calls after session_start() is invalid. Valid use
case is very limited and unlikely. If users have to change cache
limiter, they can session_commit(), then session_start(). So I would
like to keep as it is now.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net