Incompatibility with OG User Roles module

Hi. I'm the maintainer of the OG User Roles module, and at least two users report to me that a critical menu fails to appear when the Buddylist module is also installed.

I've asked them to check and double-check their settings. They report back that the menu in question appears when Buddylist is disable, and disappears when it it re-enabled.

I'd just like some feedback on what could possibly cause this hook_menu item to not appear when Buddylist is installed.

Here is the hook_menu entry for the menu in question. It is supposed to appear for users with the appropriate permissions at the URL: og/users/(GroupID). I am told by module users who have run our debug utility that the permissions currently returned at this URL are correct, but the tab still doesn't appear:

     ...
    // Add another tab to the group subscribers page for admins to
    // configure member roles
    if (arg(0) == 'og' && arg(1) == 'users' && is_numeric(arg(2))) {
      $gid = arg(2);
      if (og_user_roles_is_allowed($gid)) {
        $items[] = array(
          'path' => "og/users/$gid/roles",
          'title' => t('Configure member roles'),
          'callback' => 'og_user_roles_page',
          'callback arguments' => array($gid),
          'access' => user_access('configure member roles'),
          'weight' => 5,
          'type' => MENU_LOCAL_TASK
        );
      }
    }
    ...

The function referred to above (for reference) is here. All it does is make sure the group in question is an OG User Roles group. I don't know how this could be affected by Buddylist, but perhaps someone else can see something I can't:

/**
 * Determine if a group is allowed to configure member roles.
 *
 * @param $nid
 *   A node ID
 * @return boolean
 *   TRUE if this group type allows roles to be assigned, otherwise FALSE
 */
function og_user_roles_is_allowed($nid) {
  $node = node_load($nid);
  if (in_array($node->type, variable_get('og_node_types', array('og'))) && variable_get("og_user_roles_roles_$node->type", NULL)) {
    return TRUE;
  }
  else {
    return FALSE;
  }
}

I don't fully understand what og_user_roles is doing, but buddylist is calling user_access() from buddylist_user('load', ...) and this seems to be throwing it off, somehow. The attached patch fixed it for me.

bah. forgot to change status.

Maybe it's just me, but it looks like in this patch, you replace a line of code with the same exact code, just typed differently. Is this all it took to resolve the issue?

If you look closely, you'll see that the different typing causes php's && operator to short-circuit the call to user_access() when $type is not 'view' or 'login', thus preventing it from being called when $type = 'load', thus preventing it from caching the roles before og_user_roles gets a chance to do its thing. At least that's what seems to be happening. As I said, I don't fully understand how og_user_roles works. Does this make sense to you?

Hmmmm.... It does make some sense, in a way. OG User Roles determines the group roles a user belongs to, and adds them to the $user object (if, of course, the user is in that particular group's context). It does this using hook_user('load').

If the og_user_roles and buddylist modules are weighted the same, then og_user_roles would load the $user object after buddylist loads it.

So, I'm wondering if another way to resolve this might not be to weight the og_user_roles module lighter than buddylist? Just a guess.

Anyway, thank you so much for the patch! I'm also waiting to hear from others to see if it solves their problem.

Ahh,,. I think I understand it now... buddylist's hook_user('load') (unintentionally, it seems) calls user_access() which caches the *permissions*, based on the normal roles. Then your hook_user('load') adds more roles, but the new roles have no effect because the old permissions are already cached.

In that case, yes, I would expect that reducing og_user_roles' weight should also fix the problem.

The patch looks right - we should be checking the $op before we check the other if criteria and call user_access. The problem as written is that it does cause the user_access to be called without a fully loaded user object since the "load" op has not been handled yet. Thus, as you saw the user permissions get cached without the users roles being properly loaded.

So I'll make work of getting this patch committed and you should not have to worry about the module weighting - this is an error in buddylist.

Nice work tracking it down - that was a subtle bug.

Got a confirmation that weighting og_user_roles lighter than buddylist also resolves issue here: http://drupal.org/node/167032

Here's a slightly reworked patch that I felt was just better from a code readability standpoint - same idea as the original patch. Please try this and verify it fixes the issue and I'll commit it.

  else if ($type == 'login' || $type == 'view') {
    // show any buddylist notifications upon login and upon viewing own profile
    if ($thisuser->uid == $user->uid && user_access('maintain buddy list')) {
      buddylist_setmsg_received($thisuser);
    }

Thanks.

Yes, that's much more readable. However, when $type == 'view', that last else if won't be executed, because it's ultimately within the else of the very first if ($type == 'view'). Maybe it should just be a separate if?

Ah geez, yeah!

Let's try again. I redid this even further and moved the access checks into the buddylist_setmsg_received function thus making the code in buddylist_user even simpler.

Looks good to me. Fixes the original issue, doesn't break the notifications, and is even more readable now.

Revision 1.68.2.31

Thanks for the patch, reviews, and testing.

hey where to add this patch in the sense in which file plz let me know this it will solve my problem....
becoz basically i dnt knowhow to add patches.