Skip to:
Content

Opened 3 years ago

Closed 9 months ago

Last modified 9 months ago

#6431 closed defect (bug) (fixed)

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

Reported by: lenasterg Owned by: dcavins
Milestone: Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch
Cc:

Description

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 ) ) );
  1. 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

Attachments (6)

class-bp-groups-group.php.patch (3.0 KB) - added by lenasterg 3 years ago.
6431.diff (4.9 KB) - added by Mamaduka 2 years ago.
6431-tests.diff (1.4 KB) - added by Mamaduka 2 years ago.
6431.2.diff (2.7 KB) - added by Mamaduka 18 months ago.
6431.2-tests.diff (1.2 KB) - added by Mamaduka 18 months ago.
6431.3.patch (10.3 KB) - added by dcavins 12 months ago.
Fix up legacy forum functions to avoid use of get_group_extras().

Download all attachments as: .zip

Change History (31)

#1 @boonebgorges
3 years ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 2.4

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.

#2 @DJPaul
2 years ago

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.

#3 @DJPaul
2 years ago

  • Keywords early added
  • Milestone changed from 2.4 to 2.5

@Mamaduka
2 years ago

@Mamaduka
2 years ago

#4 @Mamaduka
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.


2 years ago

#6 @DJPaul
2 years ago

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?

#7 @Mamaduka
2 years ago

@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.

This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.


2 years ago

#9 @boonebgorges
2 years ago

  • Keywords needs-patch added; has-patch early removed

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.

#10 @DJPaul
2 years ago

  • Milestone changed from 2.5 to 2.6

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


20 months ago

#12 @dcavins
20 months ago

  • Milestone changed from 2.6 to Future Release

@Mamaduka
18 months ago

#13 @Mamaduka
18 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 2.7

@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.

#14 @r-a-y
17 months ago

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

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

Last edited 17 months ago by r-a-y (previous) (diff)

#15 @mercime
16 months ago

  • Milestone changed from 2.7 to Future Release

Punting.

#16 @DJPaul
15 months ago

  • Milestone changed from Future Release to 2.8

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

#17 @boonebgorges
13 months ago

  • Keywords needs-patch added; has-patch removed

@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).

#18 @dcavins
13 months ago

  • Owner set to dcavins
  • Status changed from new to accepted

#19 @dcavins
12 months ago

  • Keywords has-patch added; has-unit-tests needs-patch removed

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
12 months ago

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

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


12 months ago

#21 follow-up: @boonebgorges
12 months ago

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

#22 in reply to: ↑ 21 @dcavins
12 months ago

  • Milestone changed from 2.8 to Future Release

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. :)

#23 @dcavins
12 months ago

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.

#24 @dcavins
9 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

#25 @r-a-y
9 months ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.