Re: Strict session?

Hi Stats, I updated the patch to address discussion. https://gist.github.com/1379668 2011/12/4 Stas Malyshev <smalyshev@sugarcrm.com>: > Hi! > > My main concern with this change is that it is binary incompatible with > existing session implementation, which means it would be hard to get it into > 5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in > this case I'm not sure why it is required. So far the only reason I > understand was "If we do this, users cannot know if ps_modules is new one > that prevents adoption or not, especially for third party ps_modules." but > this is definitely a documentation issue and even with new API you can not > really know that - you just know there's a handler but you can not know the > module implements it properly - maybe it just always returns OK. I updated the patch and fixed them all. There is no API incompatibility also, since I added PS_MOD_SID2/PS_MOD_FUNCS_SID2. Thanks Daniel K. > I also still not sure why default SID creation function was changed to 3 > identical cloned functions. I do not think it's the right way - if some > macro is not allowing us to leave it as is, we can have another macro, but I > do not see how splitting one standard SID creation function into a number of > identical clones is beneficial. I think it needs to be changed. I consolidated key validation function into session.c. Session ID was created by php_session_create_id() function by default. So there weren't 3 creation function, but key validation functions. I hope you feel comfortable with new patch. > > For user module, with the patch if you add validation handler you also have > to add create_sid handler - which most users don't even want to change. We > should allow for using standard SID creation functions there (maybe by > passing NULL there? or by having some function that implements > php_session_create_id, maybe even better), and we shouldn't check > validate_sid when we mean create_sid - if we allow to set create_sid, we > should also support case where we set create_sid but not validate_sid. > Also, all other modules when SID creation fails set invalid_sid and return > NULL, while new user module SID creation returns "" and doesn't set > invalid_sid. Why is that? > > We also probably need to deal then with the question what should be done > when create_sid fails (I'd say we can not do much but issue fatal error but > maybe there are any better ideas) if we allow it to fail. We don't seem to > check invalid_sid now after create_sid, so if we're setting it, we must > check it. > Thank you for catching this. I added fallback to php_session_create_id() with SHA1 hash as hash function. Even if users/ps_module writers did something wrong in their code, it always has valid PS(id). It may be better to raise notice or warning error, but I don't add it. Any comments? I think the patch is much better than the original thanks to discussion. Any comments are appreciated. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net