Hi,
A while ago I submitted a patch to allow session_set_save_handler() to
accept a class, and support the inheritance of the default session
handler's methods.
The RFC has a more detailed description and the current patch:
https://wiki.php.net/rfc/session-oo
Changes since this was last discussed:
- More sanity checking to prevent handlers being called in unexpected states
- ZTS fixes
Any thoughts?
Regards,
Arpad
A while ago I submitted a patch to allow
session_set_save_handler()to
accept a class, and support the inheritance of the default session
handler's methods.The RFC has a more detailed description and the current patch:
https://wiki.php.net/rfc/session-ooChanges since this was last discussed:
- More sanity checking to prevent handlers being called in unexpected states
- ZTS fixes
Any thoughts?
Makes sense to me, and quite straight-forward in purpose. Can't comment
on the patch, though.
--
Matthew Weier O'Phinney
Project Lead | matthew@zend.com
Zend Framework | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Hi,
A while ago I submitted a patch to allow
session_set_save_handler()to
accept a class, and support the inheritance of the default session
handler's methods.The RFC has a more detailed description and the current patch:
https://wiki.php.net/rfc/session-ooChanges since this was last discussed:
- More sanity checking to prevent handlers being called in unexpected states
- ZTS fixes
Any thoughts?
Regards,
Arpad
Not an internals expert, but I do have a question.
When would the session handler object be destroyed?
If sessions are being logged to a DB (maybe via a userland PHP class),
is internal reference counting enough to handle the order of
destruction?
If the DB connection is destroyed before the session handler gets it
destructor called (or the session_write_close equivalent), the session
won't be able to log the data and there would be no warning to the
client as engine is in auto tidy up mode.
Obviously, if the developer takes care and calls the destructors in
the right order, then everything will be OK, but this is not something
I see very often.
What about circular references and interdependent references?
--
Richard Quadling
Twitter : EE : Zend : PHPDoc
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea
Not an internals expert, but I do have a question.
When would the session handler object be destroyed?
If sessions are being logged to a DB (maybe via a userland PHP class),
is internal reference counting enough to handle the order of
destruction?If the DB connection is destroyed before the session handler gets it
destructor called (or the session_write_close equivalent), the session
won't be able to log the data and there would be no warning to the
client as engine is in auto tidy up mode.
Hi,
Many thanks for your question, because I hadn't given the matter much
thought. The current patch (#7) uses static methods so it doesn't
change the status quo - the user would need still need to manually
close the session (or register a shutdown function) in order to use an
object in the close() method.
I've spent some time thinking about this and I think we have a couple
of options. First of all I've changed the interface so that the
methods are no longer static, it would already accept an object before
but it would just use it find the class name, and call the methods
statically. Now it only accepts an object.
The two options I've implemented are:
Destructor in the SessionHandler class:
http://spellign.com/patches/php-trunk-session-oo8-destruct.patch
This works ok with some provisions:
- The user's SessionHandler class must hold on to any objects it will
need in close(), (in a property, presumably). While it's possible that
the session handler would still get destructed afterwards, this is the
only way to ensure it. - If the user overrides __destruct(), they must call parent::__destruct().
Automatically register a shutdown function:
http://spellign.com/patches/php-trunk-session-oo8-shutdown_function.patch
The only argument I can think of against it is that it's possible the
user might, out of habit, register their own shutdown function after
calling session_set_save_handler(). In that case their shutdown
function would find the session already closed.
Obviously, if the developer takes care and calls the destructors in
the right order, then everything will be OK, but this is not something
I see very often.What about circular references and interdependent references?
This would give the destructor option some trouble - in this case
instances are destructed in the order in which they were created, and
it seems likely that the DB object from your example would be created
first.
I prefer the shutdown function option. We could even ensure that the
user's shutdown function always gets called first by adding a separate
hash of internal shutdown functions, however I think that would
overcomplicate matters for something we can clearly document.
The option I haven't mentioned is to preserve the status quo, I think
that would be a pity since we have the chance to address it while
keeping BC now.
I've moved the tests into a separate patch so the above differences are clearer:
http://spellign.com/patches/php-trunk-session-oo8-tests.patch
Regards,
Arpad
Not an internals expert, but I do have a question.
When would the session handler object be destroyed?
If sessions are being logged to a DB (maybe via a userland PHP class),
is internal reference counting enough to handle the order of
destruction?If the DB connection is destroyed before the session handler gets it
destructor called (or the session_write_close equivalent), the session
won't be able to log the data and there would be no warning to the
client as engine is in auto tidy up mode.Hi,
Many thanks for your question, because I hadn't given the matter much
thought. The current patch (#7) uses static methods so it doesn't
change the status quo - the user would need still need to manually
close the session (or register a shutdown function) in order to use an
object in the close() method.I've spent some time thinking about this and I think we have a couple
of options. First of all I've changed the interface so that the
methods are no longer static, it would already accept an object before
but it would just use it find the class name, and call the methods
statically. Now it only accepts an object.The two options I've implemented are:
Destructor in the SessionHandler class:
http://spellign.com/patches/php-trunk-session-oo8-destruct.patchThis works ok with some provisions:
- The user's SessionHandler class must hold on to any objects it will
need in close(), (in a property, presumably). While it's possible that
the session handler would still get destructed afterwards, this is the
only way to ensure it.- If the user overrides __destruct(), they must call parent::__destruct().
Automatically register a shutdown function:
http://spellign.com/patches/php-trunk-session-oo8-shutdown_function.patchThe only argument I can think of against it is that it's possible the
user might, out of habit, register their own shutdown function after
callingsession_set_save_handler(). In that case their shutdown
function would find the session already closed.Obviously, if the developer takes care and calls the destructors in
the right order, then everything will be OK, but this is not something
I see very often.What about circular references and interdependent references?
This would give the destructor option some trouble - in this case
instances are destructed in the order in which they were created, and
it seems likely that the DB object from your example would be created
first.I prefer the shutdown function option. We could even ensure that the
user's shutdown function always gets called first by adding a separate
hash of internal shutdown functions, however I think that would
overcomplicate matters for something we can clearly document.The option I haven't mentioned is to preserve the status quo, I think
that would be a pity since we have the chance to address it while
keeping BC now.I've moved the tests into a separate patch so the above differences are clearer:
http://spellign.com/patches/php-trunk-session-oo8-tests.patchRegards,
Arpad
Thanks for looking into this a lot more.
I've always used a First-In-Last-Out (FILO) creation/destruction order
and incorporated the concept of "ownership".
Any time an instance of a class is created, it has to be "owned" by
another instance (of any type) or by a pseudo top-parent. In my case,
I always have an instance of tApp, to which I attach my DB resource
container (invariable I communicate with multiple unlink DB servers)
and a session class (old skool).
tApp is created, DB(s) are linked to. Session is started.
As part of the app destructor, the session is closed first. Always.
Then the DB connectors. For me, during the shutdown of tApp, there is
no more activity to be processed, so the cleanup can take place in a
controlled manner. And the reverse order seems the best way.
And I always call the destructor on tApp as part of my code. A
try{}finally{} clause would certainly be beneficial but probably not
essential.
Hopefully, by the time my script has finished, all resources are
closed and finalised and the script engine shutdown/cleanup cycle is
doing nothing.
Essentially, it is moving the cleanup process into the hands of the
developer. If they don't control the order, I'm not sure you can
predict the order. Or if it can be predicted, it may not be the
desired order.
I think this is a great addition to PHP. I think it needs to be
PHP\SessionHandler. I think all new classes should be namespaced to
PHP, but that's another topic I'm sure.
--
Richard Quadling
Twitter : EE : Zend : PHPDoc
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea
Not an internals expert, but I do have a question.
When would the session handler object be destroyed?
If sessions are being logged to a DB (maybe via a userland PHP class),
is internal reference counting enough to handle the order of
destruction?If the DB connection is destroyed before the session handler gets it
destructor called (or the session_write_close equivalent), the session
won't be able to log the data and there would be no warning to the
client as engine is in auto tidy up mode.Hi,
Many thanks for your question, because I hadn't given the matter much
thought. The current patch (#7) uses static methods so it doesn't
change the status quo - the user would need still need to manually
close the session (or register a shutdown function) in order to use an
object in the close() method.I've spent some time thinking about this and I think we have a couple
of options. First of all I've changed the interface so that the
methods are no longer static, it would already accept an object before
but it would just use it find the class name, and call the methods
statically. Now it only accepts an object.The two options I've implemented are:
Destructor in the SessionHandler class:
http://spellign.com/patches/php-trunk-session-oo8-destruct.patchThis works ok with some provisions:
- The user's SessionHandler class must hold on to any objects it will
need in close(), (in a property, presumably). While it's possible that
the session handler would still get destructed afterwards, this is the
only way to ensure it.- If the user overrides __destruct(), they must call parent::__destruct().
Automatically register a shutdown function:
http://spellign.com/patches/php-trunk-session-oo8-shutdown_function.patchThe only argument I can think of against it is that it's possible the
user might, out of habit, register their own shutdown function after
callingsession_set_save_handler(). In that case their shutdown
function would find the session already closed.Obviously, if the developer takes care and calls the destructors in
the right order, then everything will be OK, but this is not something
I see very often.What about circular references and interdependent references?
This would give the destructor option some trouble - in this case
instances are destructed in the order in which they were created, and
it seems likely that the DB object from your example would be created
first.I prefer the shutdown function option. We could even ensure that the
user's shutdown function always gets called first by adding a separate
hash of internal shutdown functions, however I think that would
overcomplicate matters for something we can clearly document.The option I haven't mentioned is to preserve the status quo, I think
that would be a pity since we have the chance to address it while
keeping BC now.I've moved the tests into a separate patch so the above differences are clearer:
http://spellign.com/patches/php-trunk-session-oo8-tests.patchRegards,
Arpad
Thanks for looking into this a lot more.
I've always used a First-In-Last-Out (FILO) creation/destruction order
and incorporated the concept of "ownership".Any time an instance of a class is created, it has to be "owned" by
another instance (of any type) or by a pseudo top-parent. In my case,
I always have an instance of tApp, to which I attach my DB resource
container (invariable I communicate with multiple unlink DB servers)
and a session class (old skool).tApp is created, DB(s) are linked to. Session is started.
As part of the app destructor, the session is closed first. Always.
Then the DB connectors. For me, during the shutdown of tApp, there is
no more activity to be processed, so the cleanup can take place in a
controlled manner. And the reverse order seems the best way.And I always call the destructor on tApp as part of my code. A
try{}finally{} clause would certainly be beneficial but probably not
essential.Hopefully, by the time my script has finished, all resources are
closed and finalised and the script engine shutdown/cleanup cycle is
doing nothing.Essentially, it is moving the cleanup process into the hands of the
developer. If they don't control the order, I'm not sure you can
predict the order. Or if it can be predicted, it may not be the
desired order.I think this is a great addition to PHP. I think it needs to be
PHP\SessionHandler. I think all new classes should be namespaced to
PHP, but that's another topic I'm sure.--
Richard Quadling
Twitter : EE : Zend : PHPDoc
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea
Hi,
I've updated the patches with some changes suggested by Derick:
- Added PHPAPI in a couple of places so it doesn't break on Windows
- Changed PSF to PHP_SESSION_FUNC to avoid possible conflicts
- Split the tests into separate files
I'm also only including the shutdown function option, since I think
the destructor option is so clearly inferior for the reasons outlined
in my previous email that it's not worth complicating the vote.
The new patches are at:
http://spellign.com/patches/php-trunk-session-oo9.patch
http://spellign.com/patches/php-trunk-session-oo9-tests.patch
Regards,
Arpad
Hi,
I've updated the patches again.
The most significant change is that the shutdown function registers
another shutdown function when it's called, to (almost) ensure that
it's always the last one, and therefore user shutdown functions should
always find the session available as expected. I'd especially
appreciate feedback or improvements on this because it feels very
hacky. One option is to add a separate hash of internal shutdown
functions which run after the user shutdown functions, another is to
add an element to the module entry, a la RSHUTDOWN, which gets run in
between steps 1 and 2 in the shutdown process (see main.c:1632).
I don't know what other uses there are for this, so I've stuck with
the double shutdown function method in this patch. I'd also like to
avoid complicating the vote on this patch with a tangential issue.
I've added a second optional argument to
session_set_save_handler($obj) which is true by default and indicates
whether the shutdown function should be registered - this is so that
users with their own manual shutdown procedure can disable the
automatic approach. I think that most users will take advantage of the
automatically registered shutdown function, but this lets those with a
custom shutdown procedure handle the session shutdown within the
lifetime of the script and save the slight overhead.
http://spellign.com/patches/php-trunk-session-oo10.patch
http://spellign.com/patches/php-trunk-session-oo10-tests.patch
Regards,
Arpad
The most significant change is that the shutdown function registers
another shutdown function when it's called, to (almost) ensure that
it's always the last one, and therefore user shutdown functions should
always find the session available as expected. I'd especially
appreciate feedback or improvements on this because it feels very
hacky. One option is to add a separate hash of internal shutdown
functions which run after the user shutdown functions, another is to
add an element to the module entry, a la RSHUTDOWN, which gets run in
between steps 1 and 2 in the shutdown process (see main.c:1632).
To clarify the "almost" - it's still possible for a user shutdown
function registered after session_set_save_handler() is called, to
register another shutdown function, which would be run after the 2nd
session shutdown function. This is one reason why I prefer the other
methods, but it's a pretty small risk.
Regards,
Arpad
Hi,
I've updated the patches again.
The most significant change is that the shutdown function registers
another shutdown function when it's called, to (almost) ensure that
it's always the last one, and therefore user shutdown functions should
always find the session available as expected. I'd especially
appreciate feedback or improvements on this because it feels very
hacky. One option is to add a separate hash of internal shutdown
functions which run after the user shutdown functions, another is to
add an element to the module entry, a la RSHUTDOWN, which gets run in
between steps 1 and 2 in the shutdown process (see main.c:1632).I don't know what other uses there are for this, so I've stuck with
the double shutdown function method in this patch. I'd also like to
avoid complicating the vote on this patch with a tangential issue.I've added a second optional argument to
session_set_save_handler($obj) which is true by default and indicates
whether the shutdown function should be registered - this is so that
users with their own manual shutdown procedure can disable the
automatic approach. I think that most users will take advantage of the
automatically registered shutdown function, but this lets those with a
custom shutdown procedure handle the session shutdown within the
lifetime of the script and save the slight overhead.http://spellign.com/patches/php-trunk-session-oo10.patch
http://spellign.com/patches/php-trunk-session-oo10-tests.patchRegards,
Arpad
I'm a bit confused. If the session handler goes out of its way to
ensure that it's the last thing to run, wouldn't that cause an issue if
it tries to write session data after, say, the database connection
object it wants to use has already been cleaned up? Or is that the use
case for the "do not register" parameter? It seems like it would be a
fairly common use case...
--Larry Garfield
Hi,
2011/6/2 Arpad Ray arraypad@gmail.com:
Hi,
A while ago I submitted a patch to allow
session_set_save_handler()to
accept a class, and support the inheritance of the default session
handler's methods.The RFC has a more detailed description and the current patch:
https://wiki.php.net/rfc/session-ooChanges since this was last discussed:
- More sanity checking to prevent handlers being called in unexpected states
- ZTS fixes
Any thoughts?
Regards,
Arpad
I like the idea (and voted +1 on it), but I've some consideration to do:
+/* {{{ proto bool SessionHandler::open(string save_path, string session_name)
- Wraps the old open handler */
+PHP_METHOD(SessionHandler, open)
+{ - PS_SANITY_CHECK;
- PS(mod_user_is_open) = 1;
- RETVAL_LONG(PS(default_mod)->s_open(&PS(mod_data), PS(save_path),
PS(session_name) TSRMLS_CC));
+}
[..]
+ZEND_BEGIN_ARG_INFO(arginfo_session_class_open, 0) - ZEND_ARG_INFO(0, save_path)
- ZEND_ARG_INFO(0, session_name)
+ZEND_END_ARG_INFO()
This method does not take the save_path and session_name parameter, it
just use the INI entry.
This lead to following behavior:
$x = new SessionHandler
$x->open(1,2,3,4); // param is not used, and no param check at all
It's missing void param check for the close method as well.
Thank you for helping us make PHP better. :)
--
Regards,
Felipe Pena
I like the idea (and voted +1 on it), but I've some consideration to do:
+/* {{{ proto bool SessionHandler::open(string save_path, string session_name)
- Wraps the old open handler */
+PHP_METHOD(SessionHandler, open)
+{- PS_SANITY_CHECK;
- PS(mod_user_is_open) = 1;
- RETVAL_LONG(PS(default_mod)->s_open(&PS(mod_data), PS(save_path),
PS(session_name) TSRMLS_CC));
+}
[..]
+ZEND_BEGIN_ARG_INFO(arginfo_session_class_open, 0)- ZEND_ARG_INFO(0, save_path)
- ZEND_ARG_INFO(0, session_name)
+ZEND_END_ARG_INFO()This method does not take the save_path and session_name parameter, it
just use the INI entry.This lead to following behavior:
$x = new SessionHandler
$x->open(1,2,3,4); // param is not used, and no param check at allIt's missing void param check for the close method as well.
Hi,
Thanks for pointing that out. I've updated the patch with the param
checks and a couple of tests. Note that the close call still proceeds
if it gets arguments, although it raises the standard warning, because
the default handler would otherwise be left dangling.
The new patches (including 5.4 now):
http://spellign.com/patches/php-trunk-session-oo11.patch
http://spellign.com/patches/php-5.4-session-oo11.patch
http://spellign.com/patches/php-session-oo11-tests.patch
Cheers,
Arpad