Everything

php

Home » Project » Newsgroups » php.internals » [RFC] session_start(), read_only, lazy_write; Take 2 »

[RFC] session_start(), read_only, lazy_write; Take 2

php.internals

Andrey Andreev

12 years ago

Hi internals,

After feedback on the original proposal, I've updated the RFC about
changing lazy_write, read_only and I'm re-posting it. The first
discussion thread got somewhat spammy, so I thought a new one should
be better.

https://wiki.php.net/rfc/session-read_only-lazy_write

Waiting for your comments on here. :)

Regards,
Andrey Andreev.

Bill Salak

12 years ago

>After feedback on the original proposal, I've updated the RFC about changing lazy_write, read_only and I'm re-posting it. The first discussion thread got somewhat spammy, so I thought a new one should be better.
>
>https://wiki.php.net/rfc/session-read_only-lazy_write
>
>Waiting for your comments on here. :)
>
>Regards,
>Andrey Andreev.

Hi Andrey,
In your proposed example problematic use case your code shows that the following happens:

- Request 1, opens session, populates _SESSION superglobal, closes session (no writeback)
- Request 2, opens sessions, unsets $_SESSION['logged_in'], writes session
- Request 1, checks value of $_SESSION['logged_in'] and does not recognize that the value of $_SESSION['logged_in'] has changed by Request 2

The net result for Request 1 is exactly the same as what we have today except that today the path to this same result is that Request 2 can't read or modify the data until Request 1 finishes. Because this is not a new or unexpected result I don't see how this is "downright dangerous" unless the user misunderstood what the new functionality does and designed solutions around this misunderstanding that exposed problems in their new code. I think this is actually the core of your RFP on this point which I think just boils down to - is read_only the best name for this flag ? I personally think it's a descriptive and self-evident name for the option because the session in Request 1 *is* read only (can't be written to) and that what you think of as a read only is actually better called a "read lock". Ultimately I don't care what it's called so if that's all this is about then I have no more interest in this part of the discussion and am happy to let those who care determine the name of the option.

On your point about some future implementation of a write blocking, read-shared, session lock - it's an interesting idea.

On your point about, "Maybe, if session_start() didn't accept mode parameters, that would've been fine. However, session_start() also accepts all session.* INIs + 'lazy_write' and all of those are modes of operation and not additional actions per se. So that makes it not only strange, but also inconsistent", you've lost me -I don't see a problem. If I call session_start and I can pass in a bunch of options about how the session will act in this call stack that seems like the best and most pragmatic solution. The distinction between modes of operation and additional actions seem like a semantic nitpick that end-users wouldn't intuitively understand. In other words, it seems counter-intuitive to work some other way and wouldn't produce more easily read/written code to have it different.

Best,
Bill Salak

Andrey Andreev

12 years ago

Hi,

On Mon, Mar 24, 2014 at 7:39 PM, Bill Salak <bill@devtemple.com> wrote:

> Hi Andrey,
> In your proposed example problematic use case your code shows that the following happens:
>
>  - Request 1, opens session, populates _SESSION superglobal, closes session (no writeback)
>  - Request 2, opens sessions, unsets $_SESSION['logged_in'], writes session
>  - Request 1, checks value of $_SESSION['logged_in'] and does not recognize that the value of $_SESSION['logged_in'] has changed by Request 2
>
> The net result for Request 1 is exactly the same as what we have today except that today the path to this same result is that Request 2 can't read or modify the data until Request 1 finishes. Because this is not a new or unexpected result I don't see how this is "downright dangerous" unless the user misunderstood what the new functionality does and designed solutions around this misunderstanding that exposed problems in their new code. I think this is actually the core of your RFP on this point which I think just boils down to - is read_only the best name for this flag ? I personally think it's a descriptive and self-evident name for the option because the session in Request 1 *is* read only (can't be written to) and that what you think of as a read only is actually better called a "read lock". Ultimately I don't care what it's called so if that's all this is about then I have no more interest in this part of the discussion and am happy to let those who care determine the name of the option.

The issue is exactly that it would be easy to not understand what
'read_only' does. Nowhere have I said that the functionality itself is
dangerous or that it is not useful, exactly the opposite - I love the
idea, it's pretty neat. My problem is with how it is named, and that's
what the RFC talks about. :)
Unless I'm mistaken, you already mentioned in another thread that it
is you who suggested the 'read_only' name, so I guess it's natural
that you don't see a problem with it and that it seems descriptive and
self-evident to you. Somebody else already applied the same logic - it
only reads and it doesn't allow a write. However, as described in the
RFC, the _close immediately_ part is very unclear.
I don't want to get into lengthy arguments with you or somebody else
over what "read-only" means. I'm not just talking about my
understanding of it here ... the term already has a meaning that is
recognized everywhere (google it if you don't believe me) and that's
why I'm so strongly against keeping it as it is.

> On your point about some future implementation of a write blocking, read-shared, session lock - it's an interesting idea.

Glad you like it, but as I already told you previously - it's just for
reference, I'm not interested in adding that feature right now.

> On your point about, "Maybe, if session_start() didn't accept mode parameters, that would've been fine. However, session_start() also accepts all session.* INIs + 'lazy_write' and all of those are modes of operation and not additional actions per se. So that makes it not only strange, but also inconsistent", you've lost me -I don't see a problem. If I call session_start and I can pass in a bunch of options about how the session will act in this call stack that seems like the best and most pragmatic solution. The distinction between modes of operation and additional actions seem like a semantic nitpick that end-users wouldn't intuitively understand. In other words, it seems counter-intuitive to work some other way and wouldn't produce more easily read/written code to have it different.

Well, I certainly can't understand why you think that a separate
function would be counter-intuitive or that it won't produce
easily-read code. With what we currently have, chances are that the
following line would be seen quite often:

session_start($options);

What do you understand from that line (regardless of whether
'read_only' is in $options or not)? I see "start a session with some
options". This is again where the closing part is lost, nothing
implies that anything but "start a session" would be performed, as an
action. While on the other hand:

session_start_close($options);

I'm quite certain that everybody would have a better understanding of
what this line does, simply because it's explicit.
Yes, it is nitpicky and it's nothing but semantics, but semantics are
important. :)

Cheers,
Andrey.

Bill Salak

12 years ago

Andrey,

> The issue is exactly that it would be easy to not understand what 'read_only' does. Nowhere have I said that the functionality itself is dangerous
> or that it is not useful, exactly the opposite - I love the idea, it's pretty neat. My problem is with how it is named, and that's what the RFC talks
> about. :) Unless I'm mistaken, you already mentioned in another thread that it is you who suggested the 'read_only' name, so I guess it's natural
> that you don't see a problem with it and that it seems descriptive and self-evident to you.

I only suggested the functionality, not the name of the option. Interestingly enough I originally suggested it as a new function just like you suggest
but Yasuo asked me if I thought it would be better as an option. After mocking up some code using both I found the option passed to session_start
to give me more flexibility in my solution design and I found it more intuitive so I agreed it was better as an option passed into session_start and
not a function.

...

>> On your point about, "Maybe, if session_start() didn't accept mode parameters, that would've been fine. However, session_start() also accepts
>> all session.* INIs + 'lazy_write' and all of those are modes of operation and not additional actions per se. So that makes it not only strange, but
>> also inconsistent", you've lost me -I don't see a problem. If I call session_start and I can pass in a bunch of options about how the session will
>> act in this call stack that seems like the best and most pragmatic solution. The distinction between modes of operation and additional actions
>> seem like a semantic nitpick that end-users wouldn't intuitively understand. In other words, it seems counter-intuitive to work some other way
>> and wouldn't produce more easily read/written code to have it different.
>
> Well, I certainly can't understand why you think that a separate function would be counter-intuitive or that it won't produce easily-read code. With
> what we currently have, chances are that the following line would be seen quite often:
>
> session_start($options);
>
> What do you understand from that line (regardless of whether 'read_only' is in $options or not)? I see "start a session with some options". This is
> again where the closing part is lost, nothing implies that anything but "start a session" would be performed, as an action. While on the other hand:
>
> session_start_close($options);
>
> I'm quite certain that everybody would have a better understanding of what this line does, simply because it's explicit.
> Yes, it is nitpicky and it's nothing but semantics, but semantics are important. :)

To answer your question, as a single line yes, it read more clearly in regards to that 1 characteristic of the session, but...
when I wrote my mockups I ran through a bunch of these scenarios and found myself wrapping session_start and what you call session_start_close
with a php end user function so I could consolidate my calls to start a session into a common interface that accepted an argument to tell the function
which php native function to call to start the session. Maybe that's just me but I think this will in fact become common in codebases which liberally use
both types of sessions starts. I went down this path pretty quickly when running through use cases with my existing codebases and it felt unnecessarily
restrictive to me considering it was solved by the passing as an option to session_start solution Yasuo had suggested at that time.

The separate function approach also doesn't account for adding more things like this in the future or to chain those options together in the same session
without creating a new function for every permutation possible for any new options we add unless this acts as a one-off and all future options like this
are added as flags passed into via the options array. In other words, as a rule "make it a function not an option" doesn't scale if we add more stuff like this
in the future.

The separate function approach also doesn't cleanly (cleanly is my subjective opinion) support the option having some value outside of TRUE,
i.e. session_start(['read_only'=>FOO]) might be a viable option at some point.

Best,
Bill Salak

Andrey Andreev

12 years ago

Hi,

>> Well, I certainly can't understand why you think that a separate function would be counter-intuitive or that it won't produce easily-read code. With
>> what we currently have, chances are that the following line would be seen quite often:
>>
>> session_start($options);
>>
>> What do you understand from that line (regardless of whether 'read_only' is in $options or not)? I see "start a session with some options". This is
>> again where the closing part is lost, nothing implies that anything but "start a session" would be performed, as an action. While on the other hand:
>>
>> session_start_close($options);
>>
>> I'm quite certain that everybody would have a better understanding of what this line does, simply because it's explicit.
>> Yes, it is nitpicky and it's nothing but semantics, but semantics are important. :)
>
> To answer your question, as a single line yes, it read more clearly in regards to that 1 characteristic of the session, but...
> when I wrote my mockups I ran through a bunch of these scenarios and found myself wrapping session_start and what you call session_start_close
> with a php end user function so I could consolidate my calls to start a session into a common interface that accepted an argument to tell the function
> which php native function to call to start the session. Maybe that's just me but I think this will in fact become common in codebases which liberally use
> both types of sessions starts. I went down this path pretty quickly when running through use cases with my existing codebases and it felt unnecessarily
> restrictive to me considering it was solved by the passing as an option to session_start solution Yasuo had suggested at that time.

If you don't have to wrap the function call, you'll have to wrap the
option somehow, you can't escape from that. Thankfully it's something
that's only written once within a single application, hence why it
should be more explicit and easily recognizable.
Otherwise, everybody is entitled an opinion and their own preferences,
so we'll never agree on which one feels better in general.

> The separate function approach also doesn't account for adding more things like this in the future or to chain those options together in the same session
> without creating a new function for every permutation possible for any new options we add unless this acts as a one-off and all future options like this
> are added as flags passed into via the options array. In other words, as a rule "make it a function not an option" doesn't scale if we add more stuff like this
> in the future.
>
> The separate function approach also doesn't cleanly (cleanly is my subjective opinion) support the option having some value outside of TRUE,
> i.e. session_start(['read_only'=>FOO]) might be a viable option at some point.

I don't see the potental for neither another similar option
(representing an action instead of mode) or another possible value.
Pretty much every session-related action has its own function now, I
consider this one to be an edge case.

Cheers,
Andrey.

Andrey Andreev

12 years ago

Hi,

In order to avoid further arguments about whether a separate function
for read-and-close is better or not, I've added an alternative
proposal - to rename the option to 'read_close' or 'read_and_close'.
After all, the most important thing is that it's not 'read_only'.

Cheers,
Andrey.

Julien Pauli

12 years ago

On Tue, Mar 25, 2014 at 1:55 PM, Andrey Andreev <narf@devilix.net> wrote:

> Hi,
>
> In order to avoid further arguments about whether a separate function
> for read-and-close is better or not, I've added an alternative
> proposal - to rename the option to 'read_close' or 'read_and_close'.
> After all, the most important thing is that it's not 'read_only'.

I agree "read_and_close" is much better discribing what it really does , so
I prefer it.

For non BC changes etc.. , please, consider that you'll have a big time for
rethinking the whole session module for PHP6 if you want to (and I think
I'll be part of deep discussions here)
So don't bother too much in searching solutions for introducting new
concepts in PHP5.X session module while keeping BC. Keep all those for PHP6.

We are near 5.6 freeze, not that I dont want new shinny features, but what
I want for 5.6 is something both consistent and voted, should it be "just a
tiny feature".
Work and thoughts are not lost anyway.

Julien.P

Andrey Andreev

12 years ago

Hi,

On Wed, Mar 26, 2014 at 12:29 PM, Julien Pauli <jpauli@php.net> wrote:

> For non BC changes etc.. , please, consider that you'll have a big time for
> rethinking the whole session module for PHP6 if you want to (and I think
> I'll be part of deep discussions here)
> So don't bother too much in searching solutions for introducting new
> concepts in PHP5.X session module while keeping BC. Keep all those for PHP6.

Of course, I've got more ideas for PHP6, but what I'm targeting here
are already voted features that could've been designed in a better
way.

Btw, another argument in merging updateTimestamp() into write() is
that userland implementations of it up until now have only been
possible by altering write(), so at least some users should be more
familiar with that approach. For example:

class MySessionHandler extends SessionHandler {

public $fingerprint;

public function read($session_id)
{
$session_data = parent::read($session_id);
$this->fingerprint = md5($session_data);
return $session_data;
}

public function write($session_id, $session_data)
{
if ($this->fingerprint === md5($session_data)
{
return touch(ini_get('session.save_path').'/sess_'.$session_id);
}

return parent::write($session_id, $session_data);
}

Cheers,
Andrey.

Yasuo Ohgaki

12 years ago

Hi Andrey,

On Wed, Mar 26, 2014 at 8:15 PM, Andrey Andreev <narf@devilix.net> wrote:

> Btw, another argument in merging updateTimestamp() into write() is
> that userland implementations of it up until now have only been
> possible by altering write(), so at least some users should be more
> familiar with that approach.
>

Component only does its jobs with good implementation.
Session module consists of session manager, session save handler and session
serializer.

Session manager should manage how it works.
Session save handler should save/retrieve session data only.
Session serializer should serialize/unserialize data only.

It breaks this design with your suggestion.
So letting save handler do the manager's job is not good. IMO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Andrey Andreev

12 years ago

Hi,

On Fri, Mar 28, 2014 at 9:07 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Andrey,
>
> On Wed, Mar 26, 2014 at 8:15 PM, Andrey Andreev <narf@devilix.net> wrote:
>>
>> Btw, another argument in merging updateTimestamp() into write() is
>> that userland implementations of it up until now have only been
>> possible by altering write(), so at least some users should be more
>> familiar with that approach.
>
>
> Component only does its jobs with good implementation.
> Session module consists of session manager, session save handler and session
> serializer.
>
> Session manager should manage how it works.
> Session save handler should save/retrieve session data only.
> Session serializer should serialize/unserialize data only.
>
> It breaks this design with your suggestion.
> So letting save handler do the manager's job is not good. IMO.

I don't understand how that's breaking any kind of design. Could you elaborate?

Cheers,
Andrey.

Yasuo Ohgaki

12 years ago

Hi Julien,

On Wed, Mar 26, 2014 at 7:29 PM, Julien Pauli <jpauli@php.net> wrote:

> > In order to avoid further arguments about whether a separate function
> > for read-and-close is better or not, I've added an alternative
> > proposal - to rename the option to 'read_close' or 'read_and_close'.
> > After all, the most important thing is that it's not 'read_only'.
>
>
> I agree "read_and_close" is much better discribing what it really does , so
> I prefer it.

I'm not sure if it's good to have "and" or not, but I'm OK with or without
"and".

Should I change it now?
I mean in my github repo.
I haven't committed the RFC patch yet.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Julien Pauli

12 years ago

On Fri, Mar 28, 2014 at 8:34 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Julien,
>
> On Wed, Mar 26, 2014 at 7:29 PM, Julien Pauli <jpauli@php.net> wrote:
>
>> > In order to avoid further arguments about whether a separate function
>> > for read-and-close is better or not, I've added an alternative
>> > proposal - to rename the option to 'read_close' or 'read_and_close'.
>> > After all, the most important thing is that it's not 'read_only'.
>>
>>
>> I agree "read_and_close" is much better discribing what it really does ,
>> so
>> I prefer it.
>
>
> I'm not sure if it's good to have "and" or not, but I'm OK with or without
> "and".
>
> Should I change it now?
> I mean in my github repo.
> I haven't committed the RFC patch yet.
>

Yes please.

Also, for error raising, I saw your github discussion.
We already raise errors in session functions when the session state is not
the good one at some point.
I suggest we do it as well for new session functions that's been introduced
: session_reset() and session_abort().
Leave session_write_close() as it is.

Thx.

Julien

Yasuo Ohgaki

12 years ago

Hi Julien,

On Fri, Mar 28, 2014 at 6:15 PM, Julien Pauli <jpauli@php.net> wrote:

> Yes please.
>
> Also, for error raising, I saw your github discussion.
> We already raise errors in session functions when the session state is not
> the good one at some point.
> I suggest we do it as well for new session functions that's been
> introduced : session_reset() and session_abort().
> Leave session_write_close() as it is.
>

OK. I'll update my github soon.
When you think it's ready to merge, please let me know or please merge it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Bill Salak

12 years ago

> In order to avoid further arguments about whether a separate function for read-and-close is better or not,
> I've added an alternative proposal - to rename the option to 'read_close' or 'read_and_close'.
> After all, the most important thing is that it's not 'read_only'.

Hi Andrey,
I don't expect to change your mind on the option read_only needing to be changed and frankly I'm not
really all that concerned about what it's going to be called, but since it's been mentioned several times that
read only has a commonly understood meaning I supply this for voter consideration:
----
# vim -R ~somefile~
:help 'readonly'

# man vim
-R Read-only mode. The 'readonly' option will	be  set.   You can still edit the buffer, but will be prevented
from accidently overwriting a file.
---
This is only 1 example that's analogous to opening a session as read only, I'm sure there's many more
lurking in my subconscious that causes me to think it's an intuitive name for what it does.

If I didn't know already, when presented with the option for "read_and_close" I would probably have a
minor wtf moment but it *would* make me read the docs to understand what exactly it means to me if I
use it, since it's an unconventional option name.

Sanford Whiteman

12 years ago

> This is only 1 example that's analogous to opening a session as read
> only

Is it, though?

Vim's read-only mode prevents you from silently saving changes to the
original file, but it does not prevent you from saving changes once
you confirm them.

Vim ro is like session_start( read_only = true ) and then something
like session_write_close( confirm_write = true ) -- an extra arg that
forces you to confirm the write but allows you to do it if you insist,
without manually reopening.

Of course interactive warnings and unattended code can't really be
compared. But the "continue, if you're sure" experience of Vim ro
surely isn't the "start over, you screwed up" expected from read_only
= true.

> If I didn't know already, when presented with the option for
> "read_and_close" I would probably have a minor wtf moment but it
> *would* make me read the docs to understand what exactly it means to
> me if I use it, since it's an unconventional option name.

I think that you've made a ringing endorsement of an explicit,
unambiguous name.

No robust language can seek to be self-explanatory, but it can aim for
clear, complete documentation. This term has multiple meanings that
are all "mainstream"; debate about THE meaning is fruitless. IMO.

-- Sandy