Targeting the 'groups_leave_group' action can be difficult

[thebrandonallen]

While working on ​#BB2488, I discovered that the groups_leave_group action can be difficult to target. Prior to 2.3, groups_leave_group() removed a member by "uninviting" them. Since 2.3, this is now done by groups_remove_member.

The groups_uninvite_user and groups_leave_group hooks are dissimilar enough that the potential for conflict is minimal. However, the groups_leave_group and groups_remove_member are very similar. The only difference being that one is initiated by the individual user, while the other is initiated by a group/site admin. This problem presents itself in two ways:

1) Functions hooked to both groups_leave_group and groups_remove_member are called twice, with groups_remove_member being the first action called. This occurs twice in BP core with bp_groups_leave_group_delete_recent_activity() and groups_clear_group_user_object_cache(). Technically, it should happen three times, but the bp_core_clear_cache action for groups_remove_member is missing. In each case, the function runs once on groups_remove_member, then again for groups_leave_group, but by the second time, the task is already complete.

2) If you're trying to target the "leave" and "remove" actions similarly, but not exactly the same, there's no way to do this, per point one above. In the case of ​#BB2488, an example use case would be to allow developers to treat the user initiated action of leaving a public group differently than the admin initiated action of removing the same user from a public group. Since the leave action is typically done via UI, we can do a bp_current_action check, but it's not 100% reliable, as groups_leave_group() may not always be called via UI event.

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

[r-a-y]

However, the groups_leave_group and groups_remove_member are very similar. The only difference being that one is initiated by the individual user, while the other is initiated by a group/site admin.

Good catch, thebrandonallen.

groups_remove_member_tweaks.patch is an attempt to consolidate all these different hooks under the groups_remove_member() function. It's not pretty by any means, but it should separate the use of the 'groups_remove_member' and 'groups_leave_group' hooks.

It does this by creating a third parameter for groups_remove_member() -- $context.

If passed, it adds a hook named after $context that is fired after a group member is removed successfully. This is to emulate how 'groups_leave_group' hook works at the moment. As I said, this isn't pretty ;)

Feedback appreciated.

[thebrandonallen]

This was, more or less, the idea I had as well. My brain took me one step further, and ended with groups_leave_group() being deprecated. Your "trimmed down" approach probably makes more sense ;)

After spending some time thinking about this, how about the below patch as an iteration? The dynamic action probably isn't needed, since it really only has one purpose. The result is functionally the same, but with less code, and we have the added benefit that the groups_leave_group action stays in it's original location.

[r-a-y]

The real problem is wanting to stick to our wrapper functions where they sound correct to use, but end up creating problems.

The more I'm thinking about it, the more I like your initial solution, thebrandonallen:

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

However, BP_Groups_Member::remove() is too lenient, which is probably why groups_uninvite_user() was used in the first place because it has more checks on is_confirmed and inviter_id before removing the user from a group.

Our patches in this ticket are both trying to work around the hook-firing issue, which to me sounds wrong.

Ugly as it may sound, reverting back to groups_uninvite_user() from r9811 might be the best course of action.

Or perhaps we change the groups_remove_member() call to BP_Groups_Member::remove_invite(), as the static method will bypass another hook.

@dcavins and others - Want to chime in?

[dcavins]

Replying to r-a-y:

The more I'm thinking about it, the more I like your initial solution, thebrandonallen:

The easiest solution would be to replace the groups_remove_member() call with a call to BP_Groups_Member::remove(). The problem with that is that we then have two nearly identical functions.

I'm working to extricate invitations and membership requests from the group membership table, so using uninvite_user to remove a member from a group doesn't look good to me.

Could we determine if the user was being removed or leaving a group by comparing the passed $user_id to the current user id? If they're the same, it was self-initiated; if they're different, we know that someone else initiated the action.

I think being able to target those two actions separately is useful. The question to me is whether it makes sense to continue having both functions or to roll them into one that's a bit more context aware.

[DJPaul]

It looks like this ticket needs more time and love to resolve properly.

As @r-a-y suggested inline, do we want to revert r9811 for 2.4?

[dcavins]

Some thoughts:

• Reverting r9811 as the complete solution wouldn't fix @thebrandonallen's original problem: separating the hooks. Using groups_uninvite_user() within groups_leave_group() calls two hooks: groups_uninvite_user as well as groups_leave_group.
• Using groups_remove_member() within groups_leave_group() was probably one of my many bad ideas.
• I'm feeling like remove is applied to a user by an admin while leave is a user-initiated action.
• In BP_Groups_Member there's a delete() that looks like an all-purpose, no-intent-implied function.

My preference is to basically revert r9811 (undoing the changes to groups_remove_member()) and use BP_Groups_Member::delete() within groups_leave_group(). See 6597.03.patch.

@r-a-y what do you think?

[dcavins]

Use BP_Groups_Member::delete in groups_leave_group().

[thebrandonallen]

This was my original proposal, so I'm game. Just wasn't sure how important it was to stick to the wrapper structure.

Sidenote, I just noticed that @r-a-y isn't correctly parsed in Trac comments, which means there's no Slack notifications when he's at-mentioned. Should this get reported to Meta, or is it something on our end?

[r-a-y]

No need to report to Meta.

It looks like I get notified properly; I just missed these comments!

I think 03.patch works as well. Go ahead and commit, dcavins.

[dcavins]

In 10278:

Modify delete method in groups_leave_group().

Using groups_remove_member() in
groups_leave_group() led to confusion
in the action hooks that are called.
This change reverts the change in r9811
to avoid this problem.

Props thebrandonallen, r-a-y.

Fixes #6597.

[dcavins]

Thanks for the comments!