If a contributed module stores its user related information NOT in users.data as user_schema() suggests, then that infromation is not added to the global $user object in sess_read() and this module should implement hook_init() to reload the current user.

function hook_init() {
  global $user;
  //user objects are not completly loaded when restored from session during bootstrap, so there are no extras.....
  //$user->session exists only if the object comes from sess_read()
  $user = ($user->uid && isset($user->session)) ? user_load($user->uid) : $user;
}

Is this by design or is this a bug or am I missing something?

CommentFileSizeAuthor
#46 drupal.user-load-full-361471-46.patch724 bytesIRuslan
#27 drupal.user-load-full.27.patch1.14 KBsun
#19 drupal.user-load-full.18.patch1.07 KBsun
#14 full_user.diff687 bytesmoshe weitzman
#12 full_user.diff687 bytesmoshe weitzman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

If you're not storing the data in $user->data, you would need to implement hook_user('load') to load in your own custom data into a user object.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Fixed
xmarket’s picture

xmarket CreditAttribution: xmarket commented
Title: incomplete global $user » I have it...
Status: Fixed » Active

.. that is the reason I call user_load() in hook_init() not do some db_query().

function hook_init() {
global $user;
//user objects are not completly loaded when restored from session during bootstrap, so there are no extras.....
//$user->session exists only if the object comes from sess_read()

  //I can't have my fields and values
  $user = ($user->uid && isset($user->session)) ? user_load($user->uid) : $user;
  //Now I have them

}

so my hook_user('load') is working.

Here is the sess_read() function:

function sess_read($key) {
  global $user;

  // Write and Close handlers are called after destructing objects since PHP 5.0.5
  // Thus destructors can use sessions but session handler can't use objects.
  // So we are moving session closure before destructing objects.
  register_shutdown_function('session_write_close');

  // Handle the case of first time visitors and clients that don't store cookies (eg. web crawlers).
  if (!isset($_COOKIE[session_name()])) {
    $user = drupal_anonymous_user();
    return '';
  }

  // 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));

  // We found the client's session record and they are an authenticated user
  if ($user && $user->uid > 0) {
    // This is done to unserialize the data member of $user
    $user = drupal_unpack($user);

    // Add roles element to $user
    $user->roles = array();
    $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
    $result = db_query("SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid = %d", $user->uid);
    while ($role = db_fetch_object($result)) {
      $user->roles[$role->rid] = $role->name;
    }
  }
  // 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);
  }

  return $user->session;
}

and here is a snippet from user_load:

    $result = db_query('SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid = %d', $user->uid);
    while ($role = db_fetch_object($result)) {
      $user->roles[$role->rid] = $role->name;
    }
    //THIS IS THE MISSING LINE!!! or functionality because when sess_read() called, then user.module is not yet included.....
    user_module_invoke('load', $array, $user);

and here is the snippet from user_schema, which says, do not use users.data:

'data' => array(
'type' => 'text',
'not null' => FALSE,
'size' => 'big',
'description' => 'A serialized array of name value pairs that are related to the user. Any form values posted during user edit are stored and are loaded into the $user object during user_load(). Use of this field is discouraged and it will likely disappear in a future version of Drupal.',
),

???

xmarket’s picture

xmarket CreditAttribution: xmarket commented
Title: I have it... » suggested solution

If that is acceptable, that hook_boot() implementors can access only the "core" user object, than a few lines to _drupal_bootstrap_full() can solve the problem:

function _drupal_bootstrap_full() {
  global $user;
  static $called;
.......
  if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
    //possible solution:
    //load extras skipped in sess_read()
    if ($user->uid > 0) {
      $array = (array)$user;
      user_module_invoke('load', $array, $user);
    }
    module_invoke_all('init');
  }
xmarket’s picture

xmarket CreditAttribution: xmarket commented
Title: suggested solution » incomplete global $user
ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
Status: Active » Postponed (maintainer needs more info)

@xmarket: Are you saying you want this to be added to Drupal core? Because if you are you should set the version to 7.x (new features are always first implemented in HEAD and then backported) and you would also have to set the category to feature request
Finally you would have to write a patch http://drupal.org/patch/create that implements your suggestion

Is that where you were going with this issue?

- Arie

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
Status: Postponed (maintainer needs more info) » Fixed

Looks like xmarket won't be posting any feedback so I'm setting this to fixed.

Feel free to reopen if you think that is wrong.

- Arie

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

subspaceeddy’s picture

subspaceeddy CreditAttribution: subspaceeddy commented
Category: support » bug
Status: Closed (fixed) » Active

I have run into this problem. I guess the question is - is this an oversight or by design? I can't find sess_read in D7 so assume things are done differently there. Basically, the problem:

1. I have custom user data that is added to global $user through hook_user(load)
2. in hook_init, I need to check this custom data and then redirect the user to a particular page depending on what's there.
3. at the point hook_init is invoked, the global $user has been pulled from the session (through sess_read) and not through user_load, meaning my custom data is not there.

It's not a big problem to load the user using $user->uid at this point, once you know what's happening, but it just feels *wrong* - it took me a while to track the problem properly, assuming that global $user would have been loaded through user_load from the start.

On looking quickly at other modules to see what they do, og,module invokes hook_user(load) at og_init, which is what I ended up doing too.

Is there a reason the fix suggested in #4 can not be implemented?

These two issues may possibly be related: http://drupal.org/node/489478, http://drupal.org/node/490108

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: incomplete global $user » Global $user object not completely loaded
Version: 6.x-dev » 7.x-dev

Coming from moshe's suggestion in #638070: Router loaders causing a lot of database hits for access checks, I agree we should fully populate $user after full bootstrap.

There are two locations where this could happen:

1) At the end of http://api.drupal.org/api/function/_drupal_bootstrap_full/7 (before module_invoke_all())

2) At the beginning of http://api.drupal.org/api/function/user_init/7

I would prefer the latter, because that would mean that a module that wants to act before this happens can still use hook_init() by using a negative module weight, so it runs before user_init().

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Can you give a use case for why a module needs to act before this and can't use hook_boot?

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Active » Needs review
FileSize
full_user.diff687 bytes

This patch adds the user_load() at end of FULL bootstrap. It depends on #490108: user_load() should add session properties when loading the currently logged-in user.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ includes/common.inc
@@ -4534,6 +4534,10 @@ function _drupal_bootstrap_full() {
+  
...
+

Trailing white-space here.

+++ includes/common.inc
@@ -4534,6 +4534,10 @@ function _drupal_bootstrap_full() {
+  // Now that all modules are loaded, build full $user object.

"build [the] full"...?

Perhaps also mention that we are _entirely_ overriding/replacing the previously defined global $user. Whatever was contained, whatever was altered, will be gone.

This review is powered by Dreditor.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
FileSize
full_user.diff687 bytes

Made changes suggested by sun

Status: Needs review » Needs work

The last submitted patch, , failed testing.

Status: Needs work » Needs review

Re-test of from comment #2392312 was requested by @user.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal.user-load-full.18.patch1.07 KB

Proper patch extracted from #490108: user_load() should add session properties when loading the currently logged-in user

Status: Needs review » Needs work

The last submitted patch, drupal.user-load-full.18.patch, failed testing.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs work » Reviewed & tested by the community

thanks for keeping this moving.

Status: Reviewed & tested by the community » Needs review

Re-test of drupal.user-load-full.18.patch from comment #19 was requested by Dries.

Status: Needs review » Needs work

The last submitted patch, drupal.user-load-full.18.patch, failed testing.

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented

I'm -1 on this patch:

In D6 this created all kinds of problems since user_load() wasn't statically cached. D7 caches it though, so multiple calls to user_load() in various hooks and callbacks add a very small performance hit to reference the static variable. I'm a proponent of lazy-loading and I see this as a good chance to use it, since we now have the static caching necessary.

Doing user_load() accounts for 5-8% of a page load, which is wasteful if we don't use the full $user. #638070: Router loaders causing a lot of database hits for access checks is intended on cutting that waste, and this patch would nullify that effort.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Same here, there's no critical bug caused by this, most people figure it out pretty quickly, and it's no longer the false optimization it was in D6. We just go to the point where you can do a full bootstrap without any database queries, and this would make that impossible, as well as the 4-6% hit on sites which turn out not to need it.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

It is true that there is no critical core bug caused by this, and many people figure it out. Those are not reasons to block this. This fixes a Drupal WTF that traps many developers. Most eventually recover, but I think we should aim to not have the trap to begin with.

1. I think that on almost all substantial sites, the user object gets loaded for the current user. It is mostly in the vacuum of core where it appears that we can avoid this.
2. Sites watch performance close enough to care about are going to run with persistent entity caching and thus its a non-issue.

I think this is a case where we should choose correctness over performance.

Anyone up for a reroll? Its looking like we need some committer input soon.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal.user-load-full.27.patch1.14 KB

I guess it doesn't really make sense for the anonymous user, because there's most likely not much to load. And I guess this is what makes others here a bit nervous about performance.

For authenticated users, it's very very likely that the full $user object of the current user will be loaded anyway.

So how about this compromise?

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Works for me.

carlos8f’s picture

carlos8f CreditAttribution: carlos8f commented

I prefer sun's patch, but I'm still kind of skeptical. I don't think core should get in the habit of doing non-trivial processing unless the data is actually required. Whether or not core is a 'vacuum' in this case is unfounded, let's see an actual list of contrib modules that require a user_load($user->uid) on every request.

@moshe FYI, entity caching on users may not be possible in D7, see:

http://drupal.org/node/638070#comment-2390978
#665618: Add entity caching for users

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Yes, entity caching of users is a lot more tricky than other entities, and while I have working code for everything else either in patches or contrib, I've only had major test breakage for users so far. So relying on that isn't a safe bet as this point.

Doing this in any full bootstrap is also going to affect things like autocomplete etc, which are quite likely to run without a user_load() unless you have another module doing it in hook_boot() or hook_init(), so I really don't think it's a false optimization, and would also like to see a list of contrib modules which force it.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Postponed

OK, I'm willing to let this one go. Lets revisit it when entity cache is available for users (D8, I hope). If someone else to move it back to 7, feel free to don your battle gear and do so.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If we can get user caching in core, that's likely to be the place where it's actually a decent gain even with database caching out of the box, so hopefully (early) D8 yeah.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Title: Global $user object not completely loaded » Global $user object should be a complete entity
Category: bug » feature
Status: Postponed » Active

Only feature requests and tasks can be assigned to D8 for now, because the Drupal 8 tree is not opened yet.

Fortunately, this is more a feature request.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

We can at least easily fix this for the [current-user] token: #967330: The [current-user:?] token should provide the actual loaded user.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

fwiw entitycache has had user support for a little while now, I haven't checked whether it can pass core tests yet but it's been running in production for months with no noticeable side effects. So I'd have much less of an issue with this for Drupal 8. Although entitycache is currently only used on 5 sites, so doing this without caching support in core still concerns me a bit.

bleen’s picture

bleen CreditAttribution: bleen commented

subscribing

ohnobinki’s picture

ohnobinki CreditAttribution: ohnobinki commented

.

playahater’s picture

playahater CreditAttribution: playahater commented
Status: Active » Needs review

#12: full_user.diff queued for re-testing.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

Subscribe.

Ran into this with a field attached to the user that effected node access. When attempting to check the field in hook_node_access() $account did not contain it, but user_load($account->uid) did.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

I'm confused. How is this not a bug? If I call user_load(1234) and user 1234 is the currently logged in user, none of the fields are loaded. If user 1234 is NOT logged in, all fields ARE loaded.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

Whoops, sorry, I'm wrong. My dpm was one page load behind. So user_load did contain the fields and data, but global $user did not.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Issue tags: +Dependency Injection (DI)

Tagging Dependency Injection since we could dynamically lazy-instantiate the User object with it. Should this wait until we actually have a class for it? #1361228: Make the user entity a classed object

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I think we should ditch this proposal entirely and do the opposite:
#1549526: Change global $user into $session

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

postponed http://drupal.org/node/1634280 on this

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Closed (won't fix)

I guess it is safe to say by now that this won't happen.

As a stop-gap fix we may do #1549526: Change global $user into $session, but in reality, we want to replace the entire session handling (which would eliminate global $user entirely).

IRuslan’s picture

IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented
Issue summary: View changes
FileSize
drupal.user-load-full-361471-46.patch724 bytes

I just would like to re-roll patch from #27 to last drupal 7 version. Maybe will be useful for someone.