Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73199
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain devilix.net designates 209.85.213.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <532603A0.8060802@sugarcrm.com>
References: <CAPhkiZw5xWRkc=ofh-nB5ZkjfThZFkH_+pK5RZaQFLKhno7u7A@mail.gmail.com>
	<CAGa2bXZiBuw_L_ehvTZb5_UUedtbS0n4GXYUjtKix0S9DX_8KQ@mail.gmail.com>
	<5324FE40.1070704@sugarcrm.com>
	<CAPhkiZy1oG_WHijMknB34dyo0aZRDU89EBafpaEGqGM32DCK0A@mail.gmail.com>
	<532603A0.8060802@sugarcrm.com>
Date: Mon, 17 Mar 2014 01:07:02 +0200
Message-ID: <CAPhkiZxdrxiKT3cTW9ra=0aFDKKP3-OpjmGdxqzHxu8YEUK2ww@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Yasuo Ohgaki <yohgaki@ohgaki.net>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC] Revert/extend/postpone original RFC about
 read_only, lazy_write sessions
From: narf@devilix.net (Andrey Andreev)

Hi,

On Sun, Mar 16, 2014 at 10:03 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Hi!
>
>> Well, as I said: I don't think the previous RFC was handled properly.
>> This is probably because it isn't detailed enough for people to
>> understand it (I for example wouldn't vote for, or against something
>
> Are there any other people that you know that think so? Because it makes
> a precedent for never ending RFCing - somebody creates RFC and holds a
> vote, you don't like the result so you create counter-RFC, then they
> create counter-counter-RFC and in the meantime nothing gets done. There
> should be a point where the decision is taken. That doesn't mean we
> can't reconsider, but not a minute after the vote ends.

I understand your concern, but making a precedent is not the goal. If
this can be handled without an RFC, I'll withdraw it. However, it's
also possible that somebody says "this has gone through the RFC
process and was voted, so only another vote can override it". It's
also an RFC because it's easier to read from a single place and not
get lost in discussions.

>> Yes, on a basic level the currently accepted 'read_only' matches your
>> explanation. But there's more to it than that ... options should be
>> viewed as modes of operation (IMO anyway), and currently this is an
>> "option" that actually performs an action, and that's not even
>> obvious. 'read_only' just doesn't have the same meaning as
>> 'read_and_close', which because is an action, should be a separate
>> function.
>
> I'm not sure I understand your complaint. We already had the
> session_abort function, and nothing prevents you from using it if that's
> what you want. What you couldn't previously do is to quickly say "I'm
> not going to change state" and be done with it. It is a real use case
> and the RFC enables it. We already have all the components
> (session_abort), we just enable doing it in a clear and explicit manner
> that allows people reading the code know what's going on.

What's there to understand? Just look at it:

    session_start(['read_only' => TRUE])

It's read as "start for reading only", and not "start read an close",
which what it actually does. I strongly disagree that this is "clear
and explicit".

Nowhere have I said that there's no use case. I'm questioning the API
design, not the feature itself, the proposal is not necessarily to
drop a feature. Yasuo (as an author of the whole thing) already
offered to change the name, so why are we even arguing over this?

>> As for the login check example, maybe it isn't clear enough. What it
>> demonstrates is a decision making process, you don't know what happens
>> afterwards. What if, for example, Request2 deletes a row from a
>> database at the time of logout and Request1 tries to read the same row
>> (knowing that it is there because the user is logged in)? The
>> application breaks.
>
> Right, you can write app with broken logic (like assuming the DB row is
> there because of something that is not related to the DB row and can be
> changed independently). I don't see how this relates to read_only - you
> always could and always will be able to write apps with broken logic.

Sure you can, but it's harder to make that mistake if you have to call
session_start_close() instead.

>> How useful that would be is not the point, currently. The problem is
>> that this is exactly what 'read_only' should mean as an option/mode.
>> If that name is taken, how would you call it in the future?
>
> I don't see any reason why this option "should mean" what you claim it
> should mean. That's just your misunderstanding of what it does, easily
> rectified by a quick look into the docs.

Because, it has the same meaning everywhere. Type 'define: read-only'
in google and it would show the same meaning that I'm saying it
should.

>> I didn't mean incomplete in the sense of "it works, but can be
>> better", but rather as "it only works in half" or "we don't know how
>> it works in some cases". The RFC itself is incomplete, it doesn't
>
> Where we do not know how it works? The feature is very simple - when
> session data is about to be written, we compare it to the session data
> that has been read, and if they are identical, we do not issue write.
> Not many cases here as far as I can see.

This is all explained in the RFC.

If there's no write, how do you know the session wouldn't expire
(since it's last_whatever timestamp isn't changed) because of omitting
the write call? OK, call PS_UPDATE_FUNC() instead ... but there's no
SessionHandler()::update(), SessionHandlerInterface::update(), so what
do we do then? Nobody knows that.

In fact, in the very next quote, you're saying that it needs to be
checked, which means that you don't currently know that:

>> "last used", "last updated", "last modified", "last accessed" ...
>> however you call it, it always exists in some form. Without such a
>
> Last used and last modified are very different things. Nothing prevents
> one from tracking either. We need to check it is done properly and is
> compatible with old handlers, but it should be entirely possible to do
> it in a BC way.

See, this is what I meant with 'read_only' it may be a name closely
explaining what it does, but in fact it has an essentially different
meaning. :) The session module only uses one of those timestamps is
what I meant here. And it may be possible to handle that in a BC way,
but the RFC doesn't mention if that's really the case and/or how it
could be done - this is stuff that must be addressed.

Cheers,
Andrey.