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

[boonebgorges]

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

[DJPaul]

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.

[henry.wright]

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.

[boonebgorges]

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?

[henry.wright]

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.

[r-a-y]

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.

[boonebgorges]

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

[boonebgorges]

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

[dcavins]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[dcavins]

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