sid not available in anonimous user
I just noticed that $user->sid is not available for anonymous users. It get chopped once $user get overwritten in sess_read even if the full session row get loaded. (5.X and 6.X). include/session.inc $user = db_fetch_object(db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = // $sid=$user->sid; //... // We didn't find the client's record (session has expired), or they are an anonymous user. else { $session = isset($user->session) ? $user->session : ''; $user = drupal_anonymous_user($session); // $user->sid=$sid; } since it makes more uniform to access sid through the code, is there a good reason to kill it? If there isn't any good reason to kill it... could this small change get into core? thx -- Ivan Sergio Borgonovo http://www.webthatworks.it
Quoting Ivan Sergio Borgonovo <mail@webthatworks.it>:
If there isn't any good reason to kill it... could this small change get into core?
Isn't it a problem that uid 0 has many sessions? Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
On Wed, 16 Jan 2008 10:14:12 -0500 Earnie Boyd <earnie@users.sourceforge.net> wrote:
Quoting Ivan Sergio Borgonovo <mail@webthatworks.it>:
If there isn't any good reason to kill it... could this small change get into core?
Isn't it a problem that uid 0 has many sessions?
yes and no... All anonymous users share one entry in the users table BUT they don't share the same row in the sessions table. You can find what happens in include/sessions.inc sess_read drupal load stuff from sessions table... but then discharge it if the user is anonymous reloading from drupal_anonymous_user. I know that sid could be easily be loaded from $_SESSION but a common interface to registered/anon users would be nicer. You call your function the same way passing $user->sid no matter if the user is authenticated or not. I was wondering if there are any assumptions in the rest of drupal code about $user->sid for anonymous users... Generally uid is checked... but maybe in some cleanup place (eg. logout, session expiration, whatever in the thousands lines of code of drupal) the assumption that $user->sid is not set is made and I'd like to have surprises. If such assumption is not made... it would be nice if people that can commit on core made sid available even for anon users. I already patched my drupal... but patching others code without the hope your patch get included upstream is always a maintenance nightmare on the long run. If people find it a good idea to be able to get $user->sid even for not authenticated users... I don't mind about the implementation ;) If people think it is a bad idea, I'd like to know why. thx -- Ivan Sergio Borgonovo http://www.webthatworks.it
Quoting Ivan Sergio Borgonovo <mail@webthatworks.it>:
If people find it a good idea to be able to get $user->sid even for not authenticated users... I don't mind about the implementation ;) If people think it is a bad idea, I'd like to know why.
Create the patch and put it to an issue. The discussion can continue there. Earnie -- http://for-my-kids.com/ -- http://give-me-an-offer.com/
If people find it a good idea to be able to get $user->sid even for not authenticated users... I don't mind about the implementation ;) If people think it is a bad idea, I'd like to know why.
i can't make out what your proposal is. please post a patch.
On Wed, 16 Jan 2008 15:04:03 -0500 "Moshe Weitzman" <weitzman@tejasa.com> wrote:
If people find it a good idea to be able to get $user->sid even for not authenticated users... I don't mind about the implementation ;) If people think it is a bad idea, I'd like to know why.
i can't make out what your proposal is. please post a patch.
Index: session.inc =================================================================== --- session.inc (revisione 419) +++ session.inc (copia locale) @@ -48,6 +48,7 @@ else { $session = isset($user->session) ? $user->session : ''; $user = drupal_anonymous_user($session); + $user->sid=session_id(); } return $user->session; -- Ivan Sergio Borgonovo http://www.webthatworks.it
i can't really get excited one way or another. if you want a universal way to get session id, call session_id(). your patch is a very slight improvement, i suppose. On 1/16/08, Ivan Sergio Borgonovo <mail@webthatworks.it> wrote:
On Wed, 16 Jan 2008 15:04:03 -0500 "Moshe Weitzman" <weitzman@tejasa.com> wrote:
If people find it a good idea to be able to get $user->sid even for not authenticated users... I don't mind about the implementation ;) If people think it is a bad idea, I'd like to know why.
i can't make out what your proposal is. please post a patch.
Index: session.inc =================================================================== --- session.inc (revisione 419) +++ session.inc (copia locale) @@ -48,6 +48,7 @@ else { $session = isset($user->session) ? $user->session : ''; $user = drupal_anonymous_user($session); + $user->sid=session_id(); }
return $user->session;
-- Ivan Sergio Borgonovo http://www.webthatworks.it
On Wed, 16 Jan 2008 23:10:36 -0500 "Moshe Weitzman" <weitzman@tejasa.com> wrote:
i can't really get excited one way or another. if you want a universal way to get session id, call session_id(). your patch is a very slight improvement, i suppose.
I'm not excited as well... because it's wrong. Thank you for pointing it out in a gentle way (very slightly improvement <g>). It is not at all an improvement... it is error prone mixing current user sid with sid of an user object. Previously the code was: Index: includes/session.inc =================================================================== --- includes/session.inc (revisione 419) +++ includes/session.inc (copia locale) @@ -30,6 +30,7 @@ // Otherwise, if the session is still active, we have a record of the client's session in the database. $user = db_fetch_object(db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = '%s'", $key)); + $sid=$user->sid; // We found the client's session record and they are an authenticated user if ($user && $user->uid > 0) { @@ -48,6 +49,7 @@ else { $session = isset($user->session) ? $user->session : ''; $user = drupal_anonymous_user($session); + $user->sid=$sid; } return $user->session; The situation I'm trying to deal with are anonymous users who own temporary objects not suited to be placed in $session, eg. e-commerce carts. I didn't use $key to fill $user->sid because I need a pk in {sessions}. Most of my functions accept a $user as argument so I can manipulate the current user or a user from the admin interface. They see if the $user is authenticated, otherwise they resort to the sid to find the objects. Does it make any more sense now? Never pretend you can judge if your own code is elegant near midnight... especially if you went to sleep at 5:00am the previous day. Sorry. -- Ivan Sergio Borgonovo http://www.webthatworks.it
On Thu, 17 Jan 2008 12:01:57 +0100 Ivan Sergio Borgonovo <mail@webthatworks.it> wrote: OK... I've found another thing that can't find a place in the big picture. Even after login $user doesn't have $user->sid. You can't find it in hook load and login. While I can't find any better place to offer uniform access to sid for anonymous users other than the the below patch in core, it is very easy to add sid to $user object in a module... but well more than one module may need it as soon as the user is logged in, and since sid is getting into the $user object very soon later and anyway authenticated users have sid... why don't anticipate the addition of sid property into sess_regenerate? While the below patch have a marginal cost in term of memory (but well even carriage return in code have) anticipating the assignment of sid property shouldn't have it. Comments?
Index: includes/session.inc =================================================================== --- includes/session.inc (revisione 419) +++ includes/session.inc (copia locale) @@ -30,6 +30,7 @@
// Otherwise, if the session is still active, we have a record of the client's session in the database. $user = db_fetch_object(db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = '%s'", $key)); + $sid=$user->sid;
// We found the client's session record and they are an authenticated user if ($user && $user->uid > 0) { @@ -48,6 +49,7 @@ else { $session = isset($user->session) ? $user->session : ''; $user = drupal_anonymous_user($session); + $user->sid=$sid; }
return $user->session;
The situation I'm trying to deal with are anonymous users who own temporary objects not suited to be placed in $session, eg. e-commerce carts. I didn't use $key to fill $user->sid because I need a pk in {sessions}.
-- Ivan Sergio Borgonovo http://www.webthatworks.it
participants (3)
-
Earnie Boyd -
Ivan Sergio Borgonovo -
Moshe Weitzman