Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#6734 closed defect (bug) (wontfix)

`groups_get_groups()` doesn't distinguish between `0` and falsey values for `user_id`

Reported by: boonebgorges Owned by: boonebgorges
Milestone: Priority: normal
Severity: normal Version:
Component: Groups Keywords: 2nd-opinion needs-patch needs-unit-tests
Cc:

Description

The default value for user_id in groups_get_groups() is false, which, as expected, means to ignore group memberships when fetching groups. But the way this is implemented in BP_Groups_Group::get() is with ! empty( $user_id ) checks. This means that passing user_id=0 returns the same results. I would expect that user_id=0 would always return an empty array - the groups that user 0 is a member of. This would be useful in the following sorts of cases:

$groups_of_user = groups_get_groups( array
    'user_id' => bp_loggedin_user_id(),
) );

As things stand, it's necessary to do different logic if ( ! is_user_logged_in() ). (I found this out the hard way, because of some ugly access-related issues in a plugin I'm building.)

Do people agree with me that 0 ought to mean something different than false or null in this context? What kinds of backward compatibility concerns should we expect? (Especially in BP_Groups_Group::get(), which has the default user_id of 0.)

Change History (12)

#1 @DJPaul
4 years ago

  • Milestone changed from Awaiting Review to Under Consideration

This is tricky.

If I write code and try to use the current user, I think I would normally check to see if is_user_logged_in first, even if it's somewhere that ought to only get reached if the user is logged in.

Assuming the above, if I were writing a function like groups_get_groups today, would I support the argument change you propose? Probably I would, because it doesn't hurt, and especially if I were also supporting a negative integer (i.e. for groups this user ID is *not* a member of).

Backwards compatibility here would be my main concern. I think we'd need to do some research in plugins and see what would happen if the change were made.

#2 @henry.wright
4 years ago

My vote is to be as strict as possible when it comes to type. As Paul points out, backpat could be an issue here but I think the greater evil is to not make the change.

#3 @boonebgorges
4 years ago

Backwards compatibility here would be my main concern. I think we'd need to do some research in plugins and see what would happen if the change were made.

Definitely. This is change not worth making if it's going to break anything significant. To be clear, it should be possible to write in such a way that full backward compatibility is maintained for the following uses of groups_get_groups():

groups_get_groups(); // default behavior remains
groups_get_groups( array( 'user_id' => false ) ); // explicitly setting user_id to false

The only "breaking" change (or, I'd say, "fixing" change) is the following use:

groups_get_groups( array( 'user_id' => 0 ) );

Currently, this results in all groups being returned; I'm suggesting it should result in no groups being returned. I'll look through the plugin repo to see how/whether groups_get_groups() is being used, but it'd be helpful to get a gut check from others - do we think that anyone is passing 0 as a user ID and *expecting* all groups?

#4 @henry.wright
4 years ago

Just my two cents' worth:

I'll look through the plugin repo to see how/whether groups_get_groups() is being used

It might also be worth checking the forum for any code snippets that use groups_get_groups()

do we think that anyone is passing 0 as a user ID and *expecting* all groups?

Probably, but I don't think you should hold back with a change such as this (even if it may be a 'breaking' change) because it's actually fixing a problem.

#5 @r-a-y
4 years ago

I did a quick search on Github under the wp-plugins user and the only plugin that could be affected is BuddyDrive:
https://github.com/wp-plugins/buddydrive/blob/35df1f7a015459837f91c8e3a16c5f0419bfbb35/includes/buddydrive-item-template.php#L132-L142

Note: Github code search is a little finicky, so it might not include all wp.org plugins.

#6 @boonebgorges
4 years ago

Thanks, @r-a-y ! My guess is that that code from BuddyDrive will never be fired for non-logged-in users, and if it were, it would be unexpected behavior for all groups to appear in the list :)

#7 @DJPaul
4 years ago

  • Milestone changed from Under Consideration to Future Release

#8 @boonebgorges
3 years ago

  • Milestone changed from Future Release to 2.7
  • Owner set to boonebgorges
  • Status changed from new to assigned

See #6734 for a related issue. I'm moving to 2.7 for consideration (I need to remember to do a plugin search to accompany @r-a-y's GitHub search).

#9 @dcavins
3 years ago

I searched through the WP Plugin Directory Slurper and came up with only a few instance where plugins were using groups_get_groups() with the user_id argument. In all cases, they were passing a user ID to get the user's groups, and I think would have been surprised to get a list of all groups. Here are the references:

Topic Mover (and a fork)
https://plugins.trac.wordpress.org/browser/buddypress-topic-mover/trunk/topic-mover.php#L43

WP Idea Stream
https://plugins.trac.wordpress.org/browser/wp-idea-stream/trunk/includes/buddypress/groups.php#L3875

BP Group Hierarchy
https://plugins.trac.wordpress.org/browser/bp-group-hierarchy/trunk/extension.php#L724

JSON API for BP
https://plugins.trac.wordpress.org/browser/json-api-for-buddypress/trunk/controllers/BuddypressRead.php#L292

BuddyDrive and two forks, BuddyDrive S3 and BuddyBox
https://plugins.trac.wordpress.org/browser/buddydrives3/trunk/includes/buddydrive-item-template.php#L113

BP XML RPC Receiver and a fork
https://plugins.trac.wordpress.org/browser/buddypress-xmlrpc-receiver/trunk/bp-xmlrpc.php#L894
https://plugins.trac.wordpress.org/browser/buddypress-xml-rpc-receiver/trunk/bp-xmlrpc.php#L894

I think your proposed change would actually help these plugins do their jobs. If they're trying to find a user's groups and pass in a 0 because the current user isn't logged in, then the response should be no groups, because the current user doesn't belong to any.

Last edited 3 years ago by dcavins (previous) (diff)

#10 @boonebgorges
3 years ago

Thanks for doing the research, @dcavins! I agree that, in each of these cases, we'd be fixing a bug by making the change. Let's do it.

#11 @boonebgorges
3 years ago

  • Milestone 2.7 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I started writing a patch and realized that this won't work. The default value of 'user_id' set in bp_has_groups() is bp_displayed_user_id(). When there's no displayed user, bp_displayed_user_id() returns 0. In this case, the expectation is that all groups will display. There are almost certainly people with similar expectations, so I don't think we can make the change.

The workaround is to pass array( 0 ) to user_id.

#12 @dcavins
3 years ago

That's so sad, but you're absolutely right.

Note: See TracTickets for help on using tickets.