Pave the way for pluggable sessions handling and sessions caching

rm extraneous ''

Hmmm.... maybe the '' wasn't so extraneous:

function sess_destroy($key, $type = 'sid') {
  db_query("DELETE FROM {sessions} WHERE %s = '%s'", $type, $key);
}

Does this open us up to SQL injection? Then use the first patch.

function drupal_count_sessions($timestamp = 0, $anonymous = 0) {
  switch ($anonymous) {
    case 0:
      $query =  ' AND uid = 0';
      break;
    case 1:
      $query = ' AND uid > 0';
      break;
    default:
      $query = '';
  }

Does anyone else find it counter-intuitive if $anonymous = 0? Would the function feel better like this?

function drupal_count_sessions($timestamp = 0, $anonymous = true) {
  $query = ($anonymous) ? ' AND uid = 0' : ' AND uid > 0';
  ...

I like this one better. Addresses both of the above concerns. (note: the first patch doesn't work due to the '%s' = '%s' construction, so to avoid SQL injection I introduced my own validation on the parameter).

Use true and false instead of 0 and 1 for better clarity (it is too psychologically confusing since we're talking about whether or not to query for uid = 0).

Ooops, I'd introduced a bug in the query that gets the info for online authenticated users.

the switch is not to keep you from calling the function with a wrong second parameter, it is to allow me to safely put that parameter in the query without '', which is our protection against SQL injection. Calling the function without the switch and with SQL injection in the 2nd parameter is what I was worried about; maybe I'm misplacing my concern?

Rerolled to track HEAD; followed all of Drumm's suggestions.

Changed syntax for the parameter checking to something less verbose.

This patch changes almost no logic... it just moves some code around.

To test you would make sure that people can log in, log out; that stuff that goes into their session stays in their session (such as comment format preferences). You would test that the number of authenticated and anonymous users in the "Who's online" block is accurate.

There is no need to test performance because there is *no* performance gain implicit in this patch. However, since it moves all of the session logic to one file, it is now possible to easily swap that file out with one that handles sessions totally differently, with memcached or LDAP or whatever.

I think session_destroy and so forth are off limits as they're already built-in php functions. I could make drupal_count_sessions into sess_count, or I could rename all the redefined session functions drupal_session_*. Do you have a preference?

sess_count it is. I also discovered that the query on the users table to count authenticated users was wrong because it doesn't address authenticated users who log off. Now both anonymous and authenticated users are counted using sess_count, which is the way it was intended. I also renamed the variables in user.module to $authenticated and $anonymous to better reflect what it is they do.

ok I broke the users list. Going back to fix. The sad truth is, we can't count authenticated users from the users table... the only accurate counting is the sess_count. That means that the list of online users is also inaccurate and doesn't account for users who just logged out.

now we do both sess_count for authenticated users and the query on the users table to get the users' data. The count is accurate but the list may not be. We could fix this by changing the wording from "Online users" to "Recently seen".

you don't see the need for ending a session based on SID?

without the second parameter to sess_destroy.

no, this is broken. Don't commit.

replaced a call to session_destroy (the php function) in user_logout. It was calling sess_destroy with the SID. Now we always call sess_destroy directly with $uid, simplifying the matter greatly.

patch

Looking for reviewers.

ok

there's a misnamed variable in the last version.

and unrelated to the bug in the previous followup, here is a patch against 4.7 in case anybody is interested.