This bug only happens in certain situations, especially when you have changed the weights of the og_user_roles module.
The problem lays with the static $querie_results in function og_user_roles_all_roles and the 'load' 'op' in og_user_roles_user, let me explain in the next part of my report.
Btw all code in this report are taken from og_user_roles.module and are used to explain the problem.
og_user_roles_all_roles is called each time by the init hook; og_user_roles_init. Which is fine. It sets the static with the last result of the query based on the given user;
Snip code from og_user_roles_all_roles; (at the end of the function)
while ($row = db_fetch_array($results)) {
$x1 = $row['rid'];
$ogroles[$x1] = $row['name'];
}
$querie_results[$uid.'_'.$gid] = $ogroles;
Once the static is set, it prevents the query the next time, but also prevents that $user->roles is not set again with the same roles;
Snip code from og_user_roles_all_roles;
$c = $user->roles;
//
// Next, we add the $user->roles array to the $ogroles array, if there is an ogroles array;
//
if ($x1 > 0) {
$d = array();
$d = $c + $ogroles;
$d = array_unique($d);
}
else {
$d = $c;
}
It returns $d which is copied into $user->roles. So far, so good.
Now when ever hook og_user_roles_user is called with the same user as the user that been set here and with 'op' 'load', the bug happens; og_user_roles_user is called with a reloaded $user, with the unchanged $user->roles;
Snip code from og_user_roles_user;
// Add the group roles to $user->roles if this is a group
// This should only be effective until the next global $user call
if ($op == 'load' && ($GLOBALS['user']->uid == $user->uid || variable_get('user_roles_always_get_roles', 0))) {
$roles = og_user_roles_all_roles($user); // This returns normal $user->roles and includes OG roles if any
$user->roles = $roles;
} // end $op load
Now if we call og_user_roles_all_roles($user) with the same user as was processed during the hook_init, the user->roles will not be changed. This is now needed because hook_user is called with a clean reloaded user object, and $user->roles is without the og rules previously set.
You can trigger this bug by giving -10 weight on the og_user_roles module (give og 0 weight like default) in the system table. It will then run hook_init before of the og module. The og_init function will load the user, triggering the bug because its called after the hook_init of og_user_roles and with the same user.
Fix options?:
1st; always appended and unique the role array.
2nd; check if one of the og roles is inside $user->roles and if not present; append it.
3rd option is setting $user->og_user_roles_set with true and test if its set and append when missing (which i prefer to be honest).
Comments
Comment #1
somebodysysop commentedThis is not a part of the 2.8 release. It has been suggested, but not implemented. You can see the code for yourself here: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/og_user_rol...
I was evaluating whether to include it or not, but this problem you've documented points out why it should not.
Use the official 2.8 release. If the problem persists, I'll look into it. Sorry.
Comment #2
robert_madcap commentedHmmm well I didn't see the 2.8 release removed the static. Removing the static will 'fix' the problem, but.
og_user_roles, like every drupal module is very DB intensive. On our website, which is still under development, og_user_roles_all_roles function is sometimes called 2-3 times per page or even more. Removing the static is a nice idea, but then you are creating another problem of performance. Which is currently in my eyes a big problem with Drupal. The current website we are developing with Drupal had in his basic setup around 200-250 queries per page, which has been reduced to 60-70 by rewriting a lot of stuff of the basic core of Drupal. And yes all of these performances will be commited back to Drupal.org.
Anyway, thanks for your time and please think about performance more. I rather see a static than another query. And is its still a nice idea to use $user->og_user_roles_set (to test if roles where set) or even better, instead of the static, store the resulted roles into $user->og_user_roles and just copy them each call to the function to $user->roles when its present (or else do the query).
Comment #3
somebodysysop commentedThere has never been a static variable "querie" in *any* official version of OG User Roles.
I think it's a good idea as well. If you want to submit some code to address this, please do so.
Comment #4
somebodysysop commentedI am now at the point where I can address a few performance issues. I wanted to proceed with the "
$querie_results" idea (http://drupal.org/node/250720), but ran into your issue here.Any code suggestions for any of this?
I rather like the idea of loading all the roles for all groups into the user object once, but not sure where that should happen.
If you're still interested, would appreciate input on this.
Comment #5
sunAfter the rise of the rewritten OGUR 4.x for Drupal 6, in which many unrelated features of OGUR were removed, and nearing a release of Drupal 7, I'm closing down old issues.