Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.When og_init() reloads the global $user object, it wipes out $user properties from the sessions table that were initially set by sess_read() but are not set by user_load(). One missing $user property that is actually used (that I know of) is $user->cache. Without a $user->cache property, calling cache_get() results in PHP notice: Undefined property: stdClass::$cache in includes/cache.inc on line 42.
In this patch, I thought it would be a good idea to restore the other missing session properties as well: sid, hostname, timestamp, and session. It's a little ugly, maybe there's some nicer way to do this :)
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | og-load-subscriptions.patch | 1.25 KB | marcp |
| og-user-load.patch | 959 bytes | mfb | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedthat strikes me as a bug in user_load(). or at least most uses that i have seen are $user = user_load(). perhaps user_load itself should retrieve these columns from sessions table. i'm not opposed to a fix like this for d6, but we need to decide what the right fix is for 7.
Comment #2
mfbOK created a core issue: #490108: user_load() should add session properties when loading the currently logged-in user
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedDoes it make sense to actually get the properties form the DB. Thats kinda the point of doing a user_load() on the current user. It "refreshes" from storage.
Comment #4
mfbWell, I'm not aware of a reason we need to reload from the sessions table. I do know that cache_clear_all() modifies $user->cache without writing to the sessions table. That doesn't happen until drupal_session_commit() in drupal_page_footer(). By reloading from the sessions table you might lose a modified $user->cache?
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedWell, maybe I am making a moot point. This is a good patch, AFAICT. Needs to go into D7 first though, no?
Comment #6
marcp CreditAttribution: marcp commentedMaybe it's time for Drupal's bootstrapping code to just load up the global $user object (see #46551: problem with call to hook_user in custom module which was rejected long ago).
Since that's not going to happen, though, doesn't it make sense for contrib modules to just never load up the global $user object in hook_init()?
Since og_user('load', ...) just fills up $user->og_groups, can't you just do that same thing in og_init() instead of doing the full user_load() which winds up calling every module's hook_user() at init time? Seems a lot safer, and then when the user gets reloaded, you'll still end up going through og_user('load', ...) again.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedSure, og could fix the problem but it is simply a bug that user_load() is dangerous for the current user.
Comment #8
mfbWell really replacing global $user is the dangerous part :) If it's ok to just add the og bits onto the original $user that seems best for now (I wasn't sure if some contrib module needed og to run the full user_load() or something..)
Comment #9
marcp CreditAttribution: marcp commentedReplacing the global $user is definitely the dangerous part -- hopefully someday we won't be seeing
global $user;littered throughout core and contrib.In the meantime, OG probably should fix itself (or we can provide patches) knowing that core is flawed in this way.
@mfb - have you tried to add those bits into the original $user yet?
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedCommitted a fix. Can you guys look at http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/og/og.modul... and confirm that this fixes any issues. Thanks.
Comment #11
marcp CreditAttribution: marcp commentedMoshe - Attached is a patch that does what I was thinking. It has the same effect as what you committed, but it is more future-proof in case hook_user() goes away and it helps nudge OG a little more towards providing an API.
Note that I haven't tested your fix or this one. This smacks for a simpletest.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedThanks marcp. I actually prefer to call og_user(load) since we might add new bits one day that happen when loading a user.
Comment #13
marcp CreditAttribution: marcp commentedYeah, it's 6 of one half a dozen of the other.
I'd still prefer to pull out a separate og_load_user() and do everything (which is just 1 line for now) in there, leaving the 'load' part of hook_user() as a 1-line call to og_load_user(). Then, if we need to add new bits when loading a user, if there's specific code that should only happen in hook_user() the code would go in hook_user(). Any code that should always run when loading a new user would go in the generic load function.
The direct call to og_user() with the empty array() parameter just seems like it's going to come back to cause us (okay, you) an extra half hour of porting time when hook_user()'s footprint changes someday.
No big whoop either way -- just a nit that I've got with calling hook functions directly.
Move along!