get_group_extras function doesn't "respects" the user_id variable from groups_get_groups() function

[lenasterg]

Hi.
I noticed the following:

1. I was tried to get the groups details for a specific user ex. user_id=26 by using the function groups_get_groups() .

Example:

 $user_id = 26;
    echo '<pre>';
    var_dump( groups_get_groups( array( 'user_id' => $user_id, 'show_hidden' => true ) ) );

2. In another browser tab I was logged in with the user_id=35 (I just forget to log out from a previous test).

Result:
The var_dump return the groups of user_id=26 but the fields
["is_member"] ["is_invited"] ["is_pending"] ["is_banned"] are containing info about the loggedin user (user_id=35).

This seems like a bug to me.

If it is a bug, attached is a suggested patch which passes the $user_id variable from BP_Groups_Group::get() function (which is used in groups_get_groups()) into the get_group_extras() function.

If this an expected behavior, what is the "proper" way to get the details for each group the user is member of?. (My initial intension was to submit an enhancement which adds the is_admin, is_mod into the get_group_extras() function)

Thanks in advance
Lena

[boonebgorges]

Good catch. This does seem like a bug. I'm not sure how our unit test coverage is for get_group_extras() in general, but we will definitely need a test describing this bug.

[DJPaul]

This will probably miss 2.4 due to no tests (something we require for any SLQ changes), but leaving this here for a little longer.

[Mamaduka]

6431.diff​ includes refresh for the previous patch also optimization for BP_Groups_Group::get_group_extras() method. Previously it was triggering two DB queries which now is combined into one, also remove few extra loops.

Not sure why values where assigned this way before, compared to BP_Group_Member_Query::populate_group_member_extras() where values as casted integers. Later makes more sense and is much nicer way to handle.

P.S. $type arg was never used in this method, at least since it was introduce r2533 (this is as far as I was able to trace down in Git logs). Maybe it would be better to deprecate it.

[DJPaul]

Not tested yet, but quick comments for you @Mamaduka:

• You remove AND is_banned = 0 from the query. Is this to do with removing that second query by that is_banned check you added? If so, nice.
• In the Mock defaults values for extras lines where you moved those variable declarations outside the for loop -- are you sure? Aren't those inside so they are reset for each group in the query? Not just reset if is_banned?

[Mamaduka]

@DJPaul yes I removed AND is_banned = 0 to use only one query.

Don't see reason to reset default values on every iteration. Only keeping those for backwards compatibility, because someone might be doing strict comparisons. We have totally different behavior in BP_Groups_Members where we just cast returned values to integers. I would be nice to have same standard return types, but not sure if it's worth to break BC.

Regarding why I reset everything on $is_banned check, it's to match original behavior, moving defaults inside the loop won't give us any changes.

[boonebgorges]

Thanks for the patch, Mamaduka.

In get_group_extras(), you have:

$user_id = empty( $user_id ) ? bp_loggedin_user_id() : $user_id; 

This makes it impossible to explicitly request group extras for user 0 when the current user is logged in. Let's do something more specific than an empty() check - maybe false === $user_id (though we may also need to support null because of upstream functions; please check this).

I think DJPaul is right that we need to reset the default values in each iteration of the foreach loop. Say you have three groups 3, 6, and 10. And assume that is_banned = true for group 3. Because the defaults are not reset, the user will show as banned for 6 and 10 as well. This probably ought to be demonstrated and guarded against with a unit test :)

Combining the is_banned query is clever. +1 from me.

[Mamaduka]

@boonebgorges I tried adding better logic to handle difference between falsey values and '0'. Unfortunately without fixing #6734, change breaks things.

Current patches handle expected behavior of groups_get_groups() and similar methods.

Moving ticket into 2.7 milestone.

[r-a-y]

FYI, in r11065, I made a change to the get_group_extras() so all properties except is_banned returns an integer.

Let me know if that causes a problem here or whether I should revert.

[mercime]

Punting.

[DJPaul]

This is a small patch and let's look at getting it in 2.8 early so we don't waste the previous effort invested in it.

cc @Mamaduka

[boonebgorges]

@Mamaduka Could you take another look at this patch? Since the group queries were rewritten in 2.7, 'populate_extras' has been largely eliminated. So the part of the patch that deals with the get() method needs a rethink (or maybe just a refresh).

[dcavins]

I revisited this ticket and the provided patches, and the changes in BP2.7 (removing the populate_extras flag from BP_Groups_Group::get()) largely solved this problem. Now, groups that are fetched via BP_Groups_Group::get() are real BP_Groups_Group objects and support lazy loading the "extras" that used to be handled by BP_Groups_Group::get_group_extras(). However, some one-off methods that directly query the groups table still do not return BP_Groups_Group objects, and rely on get_group_extras():

• BP_Groups_Group::get_by_most_forum_topics()
• BP_Groups_Group::get_by_most_forum_posts()
• BP_Groups_Group::get_by_letter()
• BP_Groups_Group::get_random()

For the legacy-forum-related functions, I'd like to propose an approach where we return real BP_Groups_Group objects so we can forget about get_group_extras(). See attached patch.

See #7419 where I suggest we change BP_Groups_Group::get_by_letter() and BP_Groups_Group::get_random() to use BP_Groups_Group::get().

[dcavins]

Fix up legacy forum functions to avoid use of get_group_extras().

[boonebgorges]

This kind of change seems fine to me. Ideally it will be supported by tests, but I say go ahead with it :)

[dcavins]

Replying to boonebgorges:

This kind of change seems fine to me. Ideally it will be supported by tests, but I say go ahead with it :)

Ha, all of the changes other than the two legacy forum-related functions changes (in 6431.3.patch) have tests. I'm fine with leaving the legacy forum stuff alone, since I'm unfamiliar with it, and there don't appear to be any tests of that part of the codebase.

They're the last two uses of get_group_extras() if #7419 goes in, so I'd love to see them change, but it's not high-pressure. :)

[dcavins]

I'm considering closing this ticket, because the uses of get_group_extras() are limited to legacy forum functions. Let me know if that makes sense. Or, I can make the changes so that get_group_extras() can be deprecated. I'm not sure what the best answer is.