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.
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
Hi,
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.
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
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.
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.
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
Hi,
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.
Hi Andrey,
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
Hi,
Hi Andrey,
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.
Hi Andrey,
Hi,
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?
Manager should manage how session behaves, not save handlers.
It's basic principle of modular design.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
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?Manager should manage how session behaves, not save handlers.
It's basic principle of modular design.
I still don't get it ... the session manager has to call either
write() or updateTimestamp() and both of these are part of the session
handler. Merging them into one solves the API design and BC issues, I
don't see how it breaks any principle. They can still be split to two
methods in PHP6, but for the time being, using write() for both
purposes IMO solves way more problems than sticking to this design
principle you're talking about.
Cheers,
Andrey.
Hi Andrey,
Manager should manage how session behaves, not save handlers.
It's basic principle of modular design.I still don't get it ... the session manager has to call either
write() or updateTimestamp() and both of these are part of the session
handler. Merging them into one solves the API design and BC issues, I
don't see how it breaks any principle. They can still be split to two
methods in PHP6, but for the time being, using write() for both
purposes IMO solves way more problems than sticking to this design
principle you're talking about.
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.
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.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
I still don't get it ... the session manager has to call either
write() or updateTimestamp() and both of these are part of the session
handler. Merging them into one solves the API design and BC issues, I
don't see how it breaks any principle. They can still be split to two
methods in PHP6, but for the time being, using write() for both
purposes IMO solves way more problems than sticking to this design
principle you're talking about.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 inphpinfo()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.
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.
Cheers,
Andrey.
Hi Andrey,
I still don't get it ... the session manager has to call either
write() or updateTimestamp() and both of these are part of the session
handler. Merging them into one solves the API design and BC issues, I
don't see how it breaks any principle. They can still be split to two
methods in PHP6, but for the time being, using write() for both
purposes IMO solves way more problems than sticking to this design
principle you're talking about.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 inphpinfo()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.
Writing data and updating time stamp for GC is distinct feature.
I don't see good reason not to have API for it.
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.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
I removed PS(id) dependency from s_read() with new patch as planned.
Additional note for this.
Dependency was needed to keep compatibility. Since this change is done in
release version for use_strict_mode, BC has much higher priority than clean
code.
Since 5.6 is new release, new API, i.e. PS_VALIDATE_SID, is introduced and
code
is cleaned up as planned.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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 inphpinfo()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.
Hi Andrey,
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().
It's only applicable for object based user save handler.
Since user is implemented their own handler, this limitation should not
matter.
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.
How to update time stamp is up to save handler and its storage.
Write and updating time stamp is different operation.
Memcache does not need it if data is read. RDBMS may have separate
table to keep track time stamp. For PostgreSQL, updating one field is
faster than updating whole record.
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.
I think current design of object based save handler is better to be
redesigned.
Current object based save handler registers "previous" save handler as its
base.
However, it's mostly useless without calling parent open() function. i.e.
Other
calls simply fails when it does not.
When open() is called, some resource, e.g. file handle, db connection, etc,
is
opened. These resources cannot be accessed from user land, since it's not
"PHP resource", but raw resource. Thus it's only useful for file based
storage.
If user is using their own file based storage for some reason, they are
better
to implement their own handler fully.
I would like to remove and clean up this in future release.
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().
It's better to stick to cleaner code. Clean code is worth to have.
As I mentioned in previous, current object based save handler design has
very
limited usage and base class part would be better to be removed in the
future.
(It's not documented also :)
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Andrey,
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().It's only applicable for object based user save handler.
Since user is implemented their own handler, this limitation should not
matter.
You're saying this like nobody uses the SessionHandler class. You
don't know that, it's just an assumption of yours, and based on it
you're willing to have a limited instead of a complete feature, while
there are no real technical limitations. This is surprising,
considering that you cite "lack of API" for other features that are
IMO not as useful.
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.How to update time stamp is up to save handler and its storage.
Write and updating time stamp is different operation.
Memcache does not need it if data is read. RDBMS may have separate
table to keep track time stamp. For PostgreSQL, updating one field is
faster than updating whole record.
I'm not saying it isn't faster to update just one field, I'm saying
it's still an UPDATE clause in both cases.
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.I think current design of object based save handler is better to be
redesigned.Current object based save handler registers "previous" save handler as its
base.
However, it's mostly useless without calling parent open() function. i.e.
Other
calls simply fails when it does not.When open() is called, some resource, e.g. file handle, db connection, etc,
is
opened. These resources cannot be accessed from user land, since it's not
"PHP resource", but raw resource. Thus it's only useful for file based
storage.
If user is using their own file based storage for some reason, they are
better
to implement their own handler fully.I would like to remove and clean up this in future release.
You don't know how it's used. A user may just prepend the session_id
before calling parent::, or inject logging into the logic, or whatever
- people do all kinds of crazy stuff.
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().It's better to stick to cleaner code. Clean code is worth to have.
As I mentioned in previous, current object based save handler design has
very
limited usage and base class part would be better to be removed in the
future.
(It's not documented also :)
And as I previously mentioned - it may be limited or not useful to
you, but that's just your opinion. Also, it is documented. :)
Cheers,
Andrey.
Hi Andrey,
Hi Andrey,
On Sat, Mar 29, 2014 at 8:41 AM, Andrey Andreev narf@devilix.net
wrote: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().It's only applicable for object based user save handler.
Since user is implemented their own handler, this limitation should not
matter.You're saying this like nobody uses the SessionHandler class. You
don't know that, it's just an assumption of yours, and based on it
you're willing to have a limited instead of a complete feature, while
there are no real technical limitations. This is surprising,
considering that you cite "lack of API" for other features that are
IMO not as useful.
It has very limited usage as I wrote.
Unless base save handler opens it, it's unusable by its spec. i.e.
It simply raises errors for not opening base class. Even when base save
handler open is called, there is no way to handle opened resource in user
land.
Due to these 2 limitations, only file based save handler could be extended
AFAIK.
In other words, it's useless for other save handlers currently. i.e. 2
connections
are required for server based storage. If user must open another
connection, it's
better to open by itself.
When user needs to specialized file save handler, it would better to write
complete handler. User would not be affected by save handler changes
with this way, too.
It's possible to return resources used by C written save handlers, but it's
not
good solution. It requires dependency for modules. I don't think such
dependency
worth it while user may write complete and more efficient handler in user
land.
In addition, there are many resources for a resource. PostgreSQL connection
can be PDO, pgsql, pq, or even more, for instance.
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.How to update time stamp is up to save handler and its storage.
Write and updating time stamp is different operation.
Memcache does not need it if data is read. RDBMS may have separate
table to keep track time stamp. For PostgreSQL, updating one field is
faster than updating whole record.I'm not saying it isn't faster to update just one field, I'm saying
it's still an UPDATE clause in both cases.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.I think current design of object based save handler is better to be
redesigned.Current object based save handler registers "previous" save handler as
its
base.
However, it's mostly useless without calling parent open() function. i.e.
Other
calls simply fails when it does not.When open() is called, some resource, e.g. file handle, db connection,
etc,
is
opened. These resources cannot be accessed from user land, since it's not
"PHP resource", but raw resource. Thus it's only useful for file based
storage.
If user is using their own file based storage for some reason, they are
better
to implement their own handler fully.I would like to remove and clean up this in future release.
You don't know how it's used. A user may just prepend the session_id
before calling parent::, or inject logging into the logic, or whatever
- people do all kinds of crazy stuff.
SID extension is not documented.
User may simply call session_id() before session_start() for this.
It's simpler and more efficient.
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().It's better to stick to cleaner code. Clean code is worth to have.
As I mentioned in previous, current object based save handler design has
very
limited usage and base class part would be better to be removed in the
future.
(It's not documented also :)And as I previously mentioned - it may be limited or not useful to
you, but that's just your opinion. Also, it is documented. :)
http://jp1.php.net/manual/en/class.sessionhandler.php
I overlooked this. If you find doc, it would be great if you could provide
link.
There would be a note if base class support is removed.
Just providing interface is good enough as base class is unusable mostly
and less efficient. Making base class usable does not seem to worth the
effort. I think you'll understand what I mean if you try to write it by
yourself.
BTW, you're arguing unlocked session is dangerous. To make base class
usable, user save handler must ignore or disable locking. I think unlocked
session is useful, but you are considering unlocked session is harmful,
aren't you?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Yasuo, I don't agree with one bit of what you're saying here, so I
won't even bother replying ... this argument is getting us nowhere.
I'd like to get this into voting now and I just need to know 1 thing,
is 'read_only' being renamed to 'read_and_close' or not? If it's being
renamed anyway, I'd rather not include it in the RFC.
Cheers,
Andrey.
Hi,
Yasuo, I don't agree with one bit of what you're saying here, so I
won't even bother replying ... this argument is getting us nowhere.I'd like to get this into voting now and I just need to know 1 thing,
is 'read_only' being renamed to 'read_and_close' or not? If it's being
renamed anyway, I'd rather not include it in the RFC.Cheers,
Andrey.
I guess this commit answers my question:
https://github.com/yohgaki/php-src/commit/9ba96e6eb21bb52d7bda5279381911e3a75c3497
I'll remove that part of the RFC and move it to a vote shortly.
Cheers,
Andrey.
Hi all,
I'd like to get this into voting now and I just need to know 1 thing,
is 'read_only' being renamed to 'read_and_close' or not? If it's being
renamed anyway, I'd rather not include it in the RFC.Cheers,
Andrey.I guess this commit answers my question:
https://github.com/yohgaki/php-src/commit/9ba96e6eb21bb52d7bda5279381911e3a75c3497I'll remove that part of the RFC and move it to a vote shortly.
Cheers,
Andrey.
Sorry about the delay, I was busy during the last few days. I've just
updated the RFC by removing read_only-related content, because it was
renamed. While I figure out how to start the vote (first time doing
it), I'm giving you all a few hours for last-minute comments. :)
https://wiki.php.net/rfc/session-read_only-lazy_write
Cheers,
Andrey.
Hi Julien,
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
Hi Julien,
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
Hi Julien,
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()andsession_abort().
Leavesession_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
Hi Julien,
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()andsession_abort().
Leavesession_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.
I'd like we finish the debatte for those two functions (session_abort() and
session_reset()). Those functions have not been RFC'ed from what I know.
And Andrey.A seems not to agree very well with them, or at least
session_reset().
As soon as debatte is finished, and we found a solution, then we'll merge
the ideas.
All other stuff should be done under RFC. Session module is critical, and I
would not want we merge badness or wrong stuff to 5.6 about it. If done, we
will revert anyway to have a stable API for the upcoming 5.6
All other ideas should go to 6.0 branch (to be branched soon).
Thx.
Julien
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.
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