Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add create_sid to session_set_save_handler and SessionHandler #109

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

lt commented Jun 15, 2012

This patch allows the provision of a user supplied session id generation function.

Even though the session id generation can be tweaked with ini file options, they do not really afford the level of customisation you would expect for a "custom session handler", and in some cases add a lot of processing overhead (due to choice of hash function), or undesired results (selecting 6 bits per character gives sid values that will be automatically url encoded)

A lot of code already existed to allow a custom create_sid function, but lacked a specific implementation.

Therefore I have added a 7th (optional) argument session_set_save_handler, to allow a user function to be supplied for session id generation.

If a create_sid function is not supplied, the default function is called in its absence to preserve backwards compatibility.

Likewise create_sid has only been added to SessionHandler class and not the associated interface, to maintain backwards compatibility. If the result is not overridden, the default is called.

@lt lt Add create_sid to session_set_save_handler and SessionHandler
A lot of code already existed to allow a custom create_sid handler, but
lacked a specific implementation.

Therefore I have added a 7th (optional) argument
session_set_save_handler, to allow a user function to be supplied for
session id generation.

If a create_sid function is not supplied, the default function is
called in its absence to preserve backwards compatibility.

Likewise create_sid only added to SessionHandler class, and not the
interface to maintain backwards compatibility. If the result is not
overridden, the default is called.
59cf3a2
Contributor

hakre commented Jun 15, 2012

How does a session save handler needs to have business with the session id?

Contributor

lt commented Jun 15, 2012

@hakre Well in this respect I think session_set_save_handler is a bit of a misnomer, and SessionHandler is more correct. If it was strictly a "save" handler, it also wouldn't have business with garbage collection, session initialisation, or session reading.

Given that all of these functions are already defined together in ps_module_struct, and the only one not exposed is s_create_sid, it seemed like the logical place to include it.

Contributor

hakre commented Jun 15, 2012

Thank you for taking care and completing the interface.

@nikic nikic and 1 other commented on an outdated diff Jun 26, 2012

ext/session/mod_user_class.c
@@ -142,3 +142,19 @@
RETVAL_BOOL(SUCCESS == PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels TSRMLS_CC));
}
/* }}} */
+
+/* {{{ proto char SessionHandler::create_sid()
+ Wraps the old create_sid handler */
+PHP_METHOD(SessionHandler, create_sid)
+{
+ char *id;
+
+ zend_parse_parameters_none();
@nikic

nikic Jun 26, 2012

Owner

This should be

if (zend_parse_parameters_none() == FAILURE) {
    return;
}

Otherwise the function will continue to run even if parameter parsing fails.

@lt

lt Jun 26, 2012

Contributor

Thanks, I learned about that function from SessionHandler->close() which deliberately doesn't check for failure, as it could cause potential issues. Amended in my latest commit.

@nikic nikic commented on an outdated diff Jun 26, 2012

ext/session/mod_user_class.c
@@ -142,3 +142,19 @@
RETVAL_BOOL(SUCCESS == PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels TSRMLS_CC));
}
/* }}} */
+
+/* {{{ proto char SessionHandler::create_sid()
+ Wraps the old create_sid handler */
+PHP_METHOD(SessionHandler, create_sid)
+{
+ char *id;
+
+ zend_parse_parameters_none();
+
+ id = PS(default_mod)->s_create_sid(&PS(mod_data), NULL TSRMLS_CC);
+
+ RETVAL_STRING(id, 1);
+ efree(id);
+ return;
@nikic

nikic Jun 26, 2012

Owner

These three lines can be replaced by RETURN_STRING(id, 0). The 0 means that the string won't be duplicated, so PHP will take care of freeing it.

@lt lt Tests, fixes and optimisations
* Amended existing tests to cater for new functionality.
* Implemented fixes and optimisations recommended by NikiC
* Added create_sid to the registered interface. This was breaking
tests. It also now breaks BC for people implementing the interface
directly instead of extending the class.
6809c38
Contributor

lt commented Jun 26, 2012

I found that not adding create_sid to the registered interface breaks compatibility for anyone implementing the interface directly (i.e. not extending SessionHandler), due to the way the session_set_save_handler validates the implemented methods. So it breaks BC if it is implemented as well as if it is not implemented.

I had a little chat with Niki earlier, and I agree with his opinion that not implementing it in the iface kind of defeats the purpose of having the iface in the first place.

Contributor

smalyshev commented Aug 26, 2012

This kind of change needs UPGRADING note, since old interface implementations will need to be updated.

Contributor

weltling commented Sep 19, 2012

Windows tests passes :)

Contributor

lt commented Sep 19, 2012

I should probably make it an RFC?

Contributor

arraypad commented Dec 14, 2012

I like the idea in general but I'm strongly against adding this method to SessionHandlerInterface. As you note it would break BC, and it simply isn't a requirement for a session handler. The most common use case is to change how session data is stored, we shouldn't also force the user to define a hash function.

I would however be in support of defining an additional interface containing this method. Then session_set_save_handler() could do the equivalent of if ($saveHandler instanceof SessionIdGenerator) to check whether create_sid is supported.

Also I don't think the (optional) 7th argument to session_set_save_handler() should be included. The OO approach is IMHO, the way forward, and the procedural style call for that function is ugly enough already :)

Contributor

lt commented Dec 14, 2012

Hey Arpad,

I'll agree BC is a big concern, I've written a couple of mails to the internals list about how this situation should best be handled, but they've gone largely ignored. I've been meaning to write an RFC for this for far too long, maybe I'll find some willpower over the holidays. Hopefully then I'll get some better feedback on it.

I need to do some proper research on how people implement this in general. BC only occurs when they implement the interface directly:

  • MySessionHandler implements SessionHandlerInterface - BC break.
  • MySessionHandler extends SessionHandler - BC fine.

I wouldn't like to guess at this point which scenario is more widely used, however I would like to assume that session handlers are only defined once in a project in the overwhelming majority of projects (I think I'd be right in assuming nobody has 1000s of session handlers that they would need to fix)

In most cases, the fix would simply be to change implements SessionHandlerInterface to extends SessionHandler so that the internal create_sid function is used, and the class still keeps the interface. I hope you'll agree is a fairly trivial fix.

I'd argue that being notified of session id changes _is_ a requirement for a session handler. As you state the common use case is to change how the session data is stored (updating a filename, database key, ...). It is the responsibility of the session handler to manage this data, and in order to correctly manage it, it needs to know about all events that change how this data is stored.

This also isn't a new addition entirely. The create_sid function has been present in the internal session interface since PHP 4 (maybe before), it has just never been exposed to the user. So it has been the responsibility of the session handler for a long time already.

On forcing the user to define a hash function, again this only happens if they define the interface directly, and fixed in the same manner as above. I don't believe it's so much forcing them to choose a hash, as giving them control over exactly how the hash is produced.

One of the reasons I developed this initially is because the built in hashing was doing something undesirable when I used the ini settings to specify 6 bit per character encoding, something I could fix with my own function.

That's quite a specific scenario, and I envisage the typical use case being more like:

public function create_sid() {
    $sid = parent::create_sid();
    // rename file / update db
    return $sid;
}

Obviously again depends on them extending the class rather than implementing the interface directly.

Also not a fan of forcing people into a specific coding style without a very good reason. The procedural approach caters for people who don't already have an OO session handler, and allows them to add additional functionality without having to redevelop the whole thing. Just pretend it's not there, the code is already done, no point removing it.

Whether or not you think it is ugly is a moot point ;)

Contributor

arraypad commented Dec 14, 2012

Hi Leigh,

If an existing handler extends a class which doesn't descend from SessionHandler, then fixing the BC break isn't so straightforward. Anyway I still think the BC break is unnecessary.

I think your argument for including this in SessionHandlerInterface is disingenuous. Although the possibility is there, none of the core session handlers define their own create_sid. Also people have been fine using session_set_save_handler() for years without this functionality, so it's incontrovertibly not a requirement of a session handler.

Having said that, I do still think it's a handy feature, and I agree that it should be in SessionHandler. If you update your pull request to make it a separate interface which SessionHandler implements, I'd be happy to take a look at merging it.

Also, more tests please! :)

@arraypad arraypad commented on the diff Dec 14, 2012

ext/session/session.c
@@ -1988,13 +1989,14 @@ static PHP_FUNCTION(session_register_shutdown)
ZEND_BEGIN_ARG_INFO(arginfo_session_void, 0)
ZEND_END_ARG_INFO()
-ZEND_BEGIN_ARG_INFO_EX(arginfo_session_set_save_handler, 0, 0, 6)
+ZEND_BEGIN_ARG_INFO_EX(arginfo_session_set_save_handler, 0, 0, 7)
@arraypad

arraypad Dec 14, 2012

Contributor

This should probably just be 1 required arg actually.

@lt

lt Dec 14, 2012

Contributor

This isn't the required arguments, it's just describing the arguments that the function takes.

The logic that determines what is required and optional is in the function itself.

If only 1 parameter is passed, it checks it is a SessionHandlerInterface, then the second param is optional. Otherwise the first 6 are required and the 7th is optional.

@arraypad

arraypad Dec 14, 2012

Contributor

I'm aware of the arguments logic, I wrote it ;)

From Zend/zend_API.h:
#define ZEND_BEGIN_ARG_INFO_EX(name, pass_rest_by_reference, return_reference, required_num_args)

It doesn't seem to break anything though.

Contributor

lt commented Dec 14, 2012

Thanks for your feedback on it, when I get around to making an RFC I'll definitely include your suggestions as possible methods of implementation.

Comment on behalf of arpad at php.net:

Merged for 5.5.1 with separate SessionIdInterface

@php-pulls php-pulls closed this Jun 27, 2013

@lt lt deleted the lt:create_sid branch Dec 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment