Skip to:

Opened 6 years ago

Closed 6 years ago

#7677 closed defect (bug) (fixed)

Group with no admin or mod causes heavy user query

Reported by: cyclic's profile cyclic Owned by: boonebgorges's profile boonebgorges
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.2
Component: Groups Keywords: needs-patch


As mentioned in

When a BP group has no admins or mods, BP_Groups_Group->set_up_admins_and_mods() runs a user query with an empty array for 'include' which ends up fetching all users instead of none. This negatively affects performance up to and including memory exhaustion fatal errors on sites with lots of users (I see extra memory usage of roughly 300MB on a site with 12000 users).

This is the line that runs the query:

Pasted here in case that link goes stale:

$admin_mod_users = get_users( array(
        'include' => array_merge( $admin_ids_plucked, $mod_ids_plucked ),
        'blog_id' => null,
) );

I'm interested in implementing a patch for this. Something like this?

$admin_mod_ids = array_merge( $admin_ids_plucked, $mod_ids_plucked );
$admin_mod_users = [];

if ( ! empty( $admin_mod_ids ) ) {
        $admin_mod_users = get_users( array(
                'include' => $admin_mod_ids,
                'blog_id' => null,
        ) );

Attachments (3)

7677.01.patch (1.7 KB) - added by cyclic 6 years ago.
patch including unit test
7677.02.patch (1.8 KB) - added by cyclic 6 years ago.
updated patch with test to check group->mods as well as group->admins
7677.03.patch (1.8 KB) - added by cyclic 6 years ago.
removed extraneous debug docblock group

Download all attachments as: .zip

Change History (16)

#1 @DJPaul
6 years ago

Any idea how exactly this group ended up with no admins or moderators? Have you any custom code, plugins, run any scripts that might have affected this?

Are you able to check the row in the bp_groups and groups member tables, and see what this group looks like there?

#2 @cyclic
6 years ago

It's likely that the only admin was deleted in a nonstandard way. We run Commons In A Box with a ton of custom code and integrate with a Shibboleth-based identity management stack to deal with authentication and user management, and there are scenarios where a WordPress user could be removed and might not run all relevant hooks, so the broken state of the data is probably related.

Here's what the database says for one affected group:

> select count(*) from wp_bp_groups_members where group_id = 288\G
*************************** 1. row ***************************
count(*): 20
1 row in set (0.00 sec)

> select * from wp_bp_groups_members where group_id = 288 and ( is_admin = 1 or is_mod = 1 )\G
Empty set (0.00 sec)

#3 @cyclic
6 years ago

  • Component changed from Core to Groups
  • Version set to 2.9.2

#4 @boonebgorges
6 years ago

  • Keywords needs-patch added

Thanks, @cyclic. If you discover that there's a way to delete the sole admin of a group in the standard BP interface, please let us know, as that'd count as a separate bug.

Since it's easy to introduce a failsafe here, let's go ahead and do so. Something like what you've suggested above should work. If you can generate a patch (and ideally, a unit test), that'd be great. Thank you!

#5 @DJPaul
6 years ago

  • Milestone changed from Awaiting Review to 3.0

#6 @cyclic
6 years ago

I may have found how the admins were removed: as part of an integration with an external membership database, we call BP_Groups_Member->demote() on users when certain conditions are met. It doesn't appear to me that this tries to manage the fact that the group may be left admin-less afterwards. Did I miss where that's handled or is this the aforementioned separate bug?

groups_demote_member() doesn't modify the user if they're an admin.

Last edited 6 years ago by cyclic (previous) (diff)

#7 @boonebgorges
6 years ago

It looks like our protection happens at a higher level. We don't allow users themselves to trigger a group-leave in the normal way (by clicking Leave Group) if they're the only admin. See There's nothing deeper in our API that prevents a group from being made admin-less or member-less. And, generally speaking, I don't think there needs to be - it doesn't cause any logical problems or anything if a group doesn't have members.

So I think what we need here is a fix for the runaway query, and possibly for any other untenable weirdness that might arise in cases where all members have been removed from a group. (where "untenable" means things like fatal errors or fundamentally broken parts of the interface)

#8 @hnla
6 years ago

Why do we not simply add a site admin as group admin if we query no admins or mods, a group generally has the group creator set as primary group admin if all members demoted then a site admin would still be able to administer the group so replace them as prime admin or even group creator?

#9 @DJPaul
6 years ago

One reason is that doing database writes from the front-end when loading a page (that’s not the result of some action a user specifically invoked, such as submitting a post) does not scale and is a bad idea.

The main point as Boone described is that while this is an unusual circumstance and not a feature we intend to support from the interface, our plugin shouldn’t fall apart given an unexpected set of data.

#10 @hnla
6 years ago

got ya.

6 years ago

patch including unit test

6 years ago

updated patch with test to check group->mods as well as group->admins

6 years ago

removed extraneous debug docblock group

#11 @cyclic
6 years ago

I think solves the issue and tests that both $group->admins & $group->mods are empty when the last admin is demoted. Is there anything I should change?

#12 @boonebgorges
6 years ago

This looks perfect - thanks for the patch!

#13 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 11838:

Groups: Admin/mod queries should respect when a group has no admins/mods.

Without this patch, groups without admins or mods can trigger a lookup of
every user on the installation.

Props cyclic.
Fixes #7677.

Note: See TracTickets for help on using tickets.