Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73475
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain devilix.net designates 209.85.160.177 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXYutFpQFb+d3g3VT-zUutGYLVasBESkG+uyZTunWc6QCw@mail.gmail.com>
References: <CAPhkiZyFHshFc56UEHiDFK9k801ztno+xoS82Yf6qAcwiRPg1A@mail.gmail.com>
	<010e01cf4788$05f3c5a0$11db50e0$@devtemple.com>
	<CAPhkiZx8DuDPfks-RNNNunHsjso-9KuwRBor8TjRaK3E10qUiA@mail.gmail.com>
	<01c801cf47c0$59acbf20$0d063d60$@devtemple.com>
	<CAPhkiZwNywUi_LjOcTmB_aDONHcmpHFtExW6h+S4yYHf4ypitQ@mail.gmail.com>
	<CAPhkiZwPBoN+Y+gyTXZB68HTfYkA5eZ-yn_=2FMvir+oFza5TQ@mail.gmail.com>
	<CAMUwpuRH-cQ9fasUyOft5C7JRi5N=6szwDV5inf+wNi-eAC2iA@mail.gmail.com>
	<CAPhkiZwLk4Kjh732WAkEF+U6VyMVEz5fzi4SNVhaQK9-Rdbnew@mail.gmail.com>
	<CAGa2bXYpS6HA2LHd3C8BQ+OK0nsDYEwCfF=K7_-A0tWeOqp3vA@mail.gmail.com>
	<CAPhkiZyWJ_602JzpP9p-3T_ABPDvENE=VriK8RSLPqNNw=zUWw@mail.gmail.com>
	<CAGa2bXbHCkFNx1U0e+Cuv7ixPonRZBmnXwvU8WUgu=Z=R=YMsA@mail.gmail.com>
	<CAPhkiZwnugyOWWsPghQY0L+Q_VMuwf1vHnjuZyAGUe7=Mox-ng@mail.gmail.com>
	<CAGa2bXYEpQENja0W2FbfU_NkUJ-sJsXJY_0Xb4NFmHyo7r9qug@mail.gmail.com>
	<CAPhkiZw9Agv=OnnSzsTNz+YwwrnMgdq7c2GY1U_Q6rcL2w6o5Q@mail.gmail.com>
	<CAGa2bXYutFpQFb+d3g3VT-zUutGYLVasBESkG+uyZTunWc6QCw@mail.gmail.com>
Date: Sat, 29 Mar 2014 01:41:10 +0200
Message-ID: <CAPhkiZzBchEyw14PkYoWW2dKJkiU6f6f1miGuHRMHEMPY4aqzQ@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Julien Pauli <jpauli@php.net>, Bill Salak <bill@devtemple.com>, 
	"internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC] session_start(), read_only, lazy_write; Take 2
From: narf@devilix.net (Andrey Andreev)

Hi,

>> > Without API, manager cannot manage how it behaves. In general, submodule
>> > should
>> > avoid manager state dependency in general. It should have dedicated API
>> > for
>> > each
>> > distinct task rather than leaving it to managed by submodule. In
>> > addition,
>> > manager
>> > cannot know if save handler supports API or not. If there is API, I can
>> > display save
>> > handler capability in phpinfo() page, for example.
>> >
>> > If manager expects sub module to behave in some way, it should have
>> > explicit
>> > API
>> > for each feature. Otherwise, sub module implementation may differ module
>> > by
>> > module.
>> > Defined set of feature is better to have explicit API with modular
>> > design.
>> > It's not mandatory,
>> > but the best practice. I don't see reason not to follow the practice
>> > here.
>>
>> I agree in general, but you just gave a good reason not to follow that
>> practice - you can't know if the submodule supports it. Plus, both are
>> write operations with one of them just writing more data. I'm all for
>> best practices, but in this case there's a lot of sense not to do
>> everything by the book.
>
>
> If there is API, sub module capability can be detected.

You yourself told me that there's no way for the SessionHandler class
to know if the storage module supports PS_UPDATE_TIMESTAMP_FUNC() or
not, and that because of that we can't have
SessionHandler::updateTimestamp().

> Writing data and updating time stamp for GC is distinct feature.

For file-based sessions - it is, but for i.e. a database, there's not
much difference. write() does update the timestamp, I see no reason
why it shouldn't be able to update just the timestamp.

> I don't see good reason not to have API for it.

 - Not being able to call parent::updateTimestamp() is a good reason.
 - Having the completely unnecessary SessionUpdateTimestampInterface
is a good reason.
 - Breaking the design of current userland implementations is a good reason.
 - Not being able to have lazy_write (a performance improvement, I
remind you) by default is a good reason.

>> > Besides modular design, if write() and updateTimestamp() are merged,
>> > flag
>> > parameter
>> > for write() should be added. It breaks compatibility with current save
>> > handlers. I don't
>> > want BC that could be avoided also.
>>
>> No it shouldn't, the decision whether to write or just update the
>> timestamp is based on an internal flag, or on $session_data. No
>> additional parameters are required.
>
>
> I removed PS(id) dependency from s_read() with new patch as planned. Why
> should I introduce new dependency to s_write(), i.e. sub module, that breaks
> design?  It does not make sense.

Convenience and consistency in the userland APIs is more important
than "breaking" internal design. I understand you want to stick to
best practices, but those practices are not a religion and sometimes
you need to break them. If you ask me, a lot of things in ext/session
are broken by design ... creating files from inside read() is one
example, all methods exposed to userland to be in an interface is
another. I agree, it's not perfect, but that's just what happens when
we introduce new features into something that was never designed with
possible changes in the future in mind. For PHP6, we'll have the
freedom to redesign the whole thing completely, but for the time being
- userland code is more important than not having some dependancy in
write().

Cheers,
Andrey.