Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56740
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain sugarcrm.com designates 207.97.245.123 as permitted sender)
Message-ID: <4EDA9788.5070901@sugarcrm.com>
Date: Sat, 03 Dec 2011 13:41:28 -0800
Organization: SugarCRM
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20111105 Thunderbird/8.0
MIME-Version: 1.0
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
CC: "internals@lists.php.net" <internals@lists.php.net>
References: <CAGa2bXYDbDnTi1aF4Ts9oShyy=2NKPaTc34_+Hg7ZN0v+_MGSQ@mail.gmail.com> <4EBDC283.3040700@yahoo.co.jp> <CAGa2bXaMKzQCO=6k4WrB2W3Mp6R8R7oJz7LQBnv_4UKCk0_VxQ@mail.gmail.com> <4ED727FB.7030001@uw.no> <CAGa2bXb9k=V1X-jfx5NM=0RrRo2RwxyG6HjaKWLZHLb2Tpe7jQ@mail.gmail.com>
In-Reply-To: <CAGa2bXb9k=V1X-jfx5NM=0RrRo2RwxyG6HjaKWLZHLb2Tpe7jQ@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Strict session?
From: smalyshev@sugarcrm.com (Stas Malyshev)

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.

Maybe we can make separate binary-compatible patch for 5.3/5.4 branches 
and patch with new APIs for trunk? The user-space handlers for 5.3/5.4 
would still have to do the checks (though stock modules will have the 
checks built-in) but for trunk they could do it by defining validation 
callbacks.

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.

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.
-- 
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227