user_load() should add session properties when loading the currently logged-in user

Without this code, it is easy to neuter the $user object just by reloading it (i.e. user_load()). For example, the $user->cache property disappears and thats a key part of cache::get

bad bot.

Wanted to review this but HEAD moved very far. It was hard to track down as entity_load is now used but the UserController Class does use the exact code and did miss this patch. So I reissue your patch against the latest head. My test so far run fine with the patch. Maybe the user.test should add some tests for those fields?

wrong path...

Thanks for reroll. Back to RTBC.

1. Let's get a test showing the previous buggy behaviour to ensure user_load()s continue to not blow away 'core' information in the future.
2. Should we name-space these properties in some way ($user->session->XXX perhaps) so that it's clear that sid is "session ID" and not "slushie ID"?

Reworked the patch so that we pull session info from the DB instead of from the existing user object. Thats what we do for roles already, so I think it is simplest to be consistent. This adds one (very fast) query to user_load().

1. I added a test which fails for current HEAD but passes with this patch. Ergo, we shouldn't ever break this.
2. Agreed, but for D8. This patch really isn't about moving properties, it is assuring that they are always present.

This patch is a prerequisite for #361471: Global $user object should be a complete entity

+++ modules/simpletest/tests/user_test.info
@@ -0,0 +1,8 @@
+name = "User tests"
+description = "Support module for user module testing."

Hmmm... we already have a session_test.module. Not sure whether we need to introduce user_test.module for this issue. (we definitely need to do it at some point, but all of this rather maps to testing session handling IMHO)

+++ modules/simpletest/tests/user_test.module
@@ -0,0 +1,30 @@
+  // The name of the menu changes during the course of the test. Using a $_GET.

This comment seems to be stale? At least the rest of the patch doesn't seem to pass any $_GET parameters.

+++ modules/simpletest/tests/user_test.module
@@ -0,0 +1,30 @@
+ * Menu callback. Affirm that sid element is still present. 

Should state what the test module function does, not what the test does.

"Menu callback; Output the current user's session id."

+++ modules/simpletest/tests/user_test.module
@@ -0,0 +1,30 @@
+ *
+ */

Superfluous PHPDoc line.

+++ modules/simpletest/tests/user_test.module
@@ -0,0 +1,30 @@
+  // Reload current user account and print session id

Missing trailing period (full-stop).

+++ modules/simpletest/tests/user_test.module
@@ -0,0 +1,30 @@
+  return 'session_id:' . $account->sid . "\n";

+++ modules/user/user.test
@@ -1340,3 +1340,41 @@ class UserEditTestCase extends DrupalWebTestCase {
+    $matches = array();
+    // debug($this->drupalGetContent());
+    preg_match('/\s*session_id:(.*)\n/', $this->drupalGetContent(), $matches);
+    $this->assertTrue(!empty($matches[1]) , t('Found session ID after user_load()'));

If we would simply use a DIV id="session_id" here, having the user's session id as text content, then we would simply use xpath() to retrieve the value in the test.

+++ modules/user/user.module
@@ -247,6 +247,15 @@ class UserController extends DrupalDefaultEntityController {
+    // Add session details from the database if available for current user.

"Re-attach session data for the current user."

+++ modules/user/user.module
@@ -247,6 +247,15 @@ class UserController extends DrupalDefaultEntityController {
+      if ($columns = db_query("SELECT * FROM {sessions} WHERE sid = :sid", array(':sid' => session_id()))->fetchAssoc()) {
+        foreach ($columns as $key => $value) {
+          $queried_users[$record->uid]->$key = $value;
+        }
+      }

Since we are still in user_load() and generating the value that will be re-assigned to $user, can't we simply copy over the values from $user to $record->uid instead of querying the db?

+++ modules/user/user.module
@@ -247,6 +247,15 @@ class UserController extends DrupalDefaultEntityController {
+    

+++ modules/user/user.test
@@ -1340,3 +1340,41 @@ class UserEditTestCase extends DrupalWebTestCase {
+  
...
+    
+    

Trailing white-space here.

+++ modules/user/user.test
@@ -1340,3 +1340,41 @@ class UserEditTestCase extends DrupalWebTestCase {
+ * Test case to test user_load() behaviour.

s/case to test//

This review is powered by Dreditor.

I now see your reasoning for querying the database.... however, catch really tried hard to cut down the database queries recently and succeeded doing so. Following this direction, I guess we should not run a query and just copy over.

Since session.inc is pluggable the data may not be in session table, so yeah copying over seems right here.

Implemented all feedback including copying of properties and using session_test module.

I wonder whether we shouldn't move the copying of user object values into session.inc - hence, we don't have to consult the schema, and furthermore, an alternative session handling could do whatever it likes. I.e. we simply call the function drupal_session_user_load($record).

In addition, it seems like we could remove the drupal_unpack() + user roles handling from http://api.drupal.org/api/function/_drupal_session_read/7 with this patch? Or should code be able to expect that $user->roles is defined prior full bootstrap? I wouldn't necessarily expect that. At the very least, I don't think that anything needs $user->data unpacked before full bootstrap.

Attached patch contains what I have in mind. The previous patch implemented the conditional copying of $user properties at the wrong place, and I can see why that happened, so I additionally cleaned up the UserController::attachLoad() function.

Since that would fail without the change from #361471: Global $user object should be a complete entity, I've incorporated those two lines in here. (errr, and while doing so, also fixed those two lines ;)

Thoughts?

That still seems to be doing a direct query, it looks like the only way we can get this according to _drupal_session_read() is to copy over from $_SESSION.

huh? Which query?

+++ includes/session.inc	21 Dec 2009 00:54:32 -0000
@@ -407,3 +403,33 @@ function drupal_save_session($status = N
+function drupal_session_user_load($record) {
+  global $user;
+
+  // Retrieve the unprocessed schema for {sessions} to account for eventually
+  // added or altered columns.
+  $fields = drupal_schema_fields_sql('sessions');
+  foreach ($fields as $field) {
+    $record->$field = isset($user->$field) ? $user->$field : NULL;
+  }
+}

Or do you mean the schema "query"?

I'm on crack. Are you, too?

ahh, I missed what was going on in Moshe's patch, and crossposted with yours which makes it more obvious. The approach seems fine in itself, but note there's an 8% performance improvement over at #638070: Router loaders causing a lot of database hits for access checks and we don't have the Drupal 6 problem of no static caching of user_load(). So while in principle I dislike the fake $user object we get, it's a pattern we've had for several releases, and while it might have been a false optimization before, it's not now.

Also note that this patch removes 1 query from all requests that don't do a full bootstrap.

Fixed the tests.

Thanks for the enhancements. Looking good. Removing that query for all requests, even cached ones, is a pretty big win. It makes all permission checks before full bootstrap invalid. Some hook_boot() implementations might not be happy about that. Hmmm.

+++ includes/session.inc	21 Dec 2009 00:54:32 -0000
@@ -407,3 +403,33 @@ function drupal_save_session($status = N
+ * Takes over session data into a fully loaded user object.

How about "Add session specific properties to $user account object."

+++ includes/session.inc	21 Dec 2009 00:54:32 -0000
@@ -407,3 +403,33 @@ function drupal_save_session($status = N
+ * can be taken over. All session handling implementations are required to

" ...can be populated."

+++ includes/session.inc	21 Dec 2009 00:54:32 -0000
@@ -407,3 +403,33 @@ function drupal_save_session($status = N
+ * replaced, too.

no comma needed

+++ modules/user/user.test	21 Dec 2009 01:47:48 -0000
@@ -1307,6 +1307,49 @@ class UserCreateTestCase extends DrupalW
+    // Verify that session data and authenticated user role is present when
+    // reloading the current user.

The browser based test below also does an account reload. Perhaps only do the role test on the current user and not the reload test. Or do the role test in the session callback also? Current way seems like it duplicates.

+++ modules/user/user.test	21 Dec 2009 01:47:48 -0000
@@ -1307,6 +1307,49 @@ class UserCreateTestCase extends DrupalW
+    if ($uid = $user->uid) {

== operator instead of =

We don't start a session for anonymous users until we have to, so for most cached pages that query shouldn't be called (although I'm not 100% what the status is for that), and if you put sessions elsewhere, then it goes anyway. I'm sticking to #25.

I equally tried to think of any code/implementation that would want or require access to $user->roles before full bootstrap, but couldn't think of any. At least, core doesn't need it, and if a module does, then it's still able to query that info on its own... Note that this also affects user_access() during bootstrap - but equally, I'm not sure whether there's anything that tries to perform access checks before full bootstrap.

catch is right that this probably only affects uncached pages for authenticated users. But also in that scenario, we're retrieving user roles two times currently, and needlessly unpack {users}.data for the half-baked $user object.

Getting rid of the half-baked $user object in full bootstrap is a huge win, IMHO. I know that I at least ran into this inconsistency issue a couple of times and was all over confused again why $user didn't contain what I expected.

Incorporated moshe's suggestions.

Good point that core does not start a session until is needed. However, I think that most sites in the wild will have $_SESSION populated by one module or another and will be starting sessions even for anon. It will be performance sensitive sites that look into this and avoid those modules or patch them. So, I think the removed query will help a lot of sites in the real world.

For that matter, I think most sites will load the user object on uncached pages so avoiding that like we have discussed in the router items issue is not worth it, IMO. @catch makes some good points about how we have user object static caching in D7 but even then I think it makes sense to just provide a fully baked user object. Few sites/pages will actually get the performance win that a half baked user object provides. IMO, this is a good patch.

+++ includes/session.inc	21 Dec 2009 02:39:15 -0000
@@ -407,3 +403,34 @@ function drupal_save_session($status = N
+  // Retrieve the unprocessed schema for {sessions} to account for eventually

I don't think 'unprocessed' is correct here. We do take schema_alter into account.

This is a tiny function, and is fully explained by the doxygen. I suggest omitting this inline comment.

If we want to remove the double query for user_roles(), we could move that to a helper function with a static cache on $uid and try to use it in both places before hitting the database again.

So the installer fails due to the removed $user->roles before full bootstrap or what? Let's have the testbot a crack at this.

Updated with latest compromise of #361471: Global $user object should be a complete entity

Anyone willing to refresh this?

any further updates on this?

Looking back at this I'm not sure why we do this at all. We should just access $_SESSION instead of copying over. If this is still a valid bug in D7 then the schema call here needs to be removed to avoid loading the entire schema cache every request.

i'm happy with with just accessing $_SESSION

is this still valid for 8.x?

I think we're working on removing those special properties from the current user object, this goes in the opposite direction. Won't fix?