Group member count routine is bad

[boonebgorges]

There are in fact two bad things going on here, but they're closely related, so I'm lumping them together.

1. Group member count is stored in groupmeta for performance. This count is refreshed every time a group's Members page is viewed. ​https://buddypress.trac.wordpress.org/browser/tags/2.9.1/src/bp-groups/bp-groups-screens.php?marks=585#L573 [2858] This is bad. This cached value only needs updating when a member joins or leaves.

2. The method used to query the group member count is groups_get_total_member_count(), which calls BP_Groups_Group::get_total_member_count(), which makes a direct SQL query. This query doesn't take into account whether users are activated in WP (see user_status or spam in the wp_users table, it ignores whether the users in question even exist in wp_users, it is not filterable, and it is not cached. We should not use it. This should be switched to a regular group member query, with 'count' functionality added to that query class if it's not already there.

[espellcaste]

Good points and I noticed that as well when working on the REST API endpoints. There are actually other examples in the code base where direct DB queries are performed without cache.

[espellcaste]

Here is a patch which does a few things:

• Updates groups_get_total_member_count to grab from group cache if available
• Fallbacks to BP_Groups_Group::get_total_member_count
• BP_Groups_Group::get_total_member_count was updated to use groups_get_group_members so that only active users are considered (banned users are excluded)
• The "Refresh the group member count meta." on the group member template was removed. This is not necessary since there is already code to update this cache when a user leaves/join/remove from a group or when a user is deleted.
• Ticket version was updated to 1.2.3, this is when the groups_get_total_member_count function was introduced.
• Also, minor tweaks were added.

[imath]

Hi @espellcaste

Thanks a lot for your work on this. The first thing I noticed was the following code you are using :

	$bp    = buddypress();
	$group = bp_get_group( $group );

	if (
		empty( $group->id )
		&& (
			! empty( $bp->groups->current_group )
			&& is_numeric( $group ) // **This cannot be true.**
			&& $group === $bp->groups->current_group->id
		)
	) {
		$group = $bp->groups->current_group;
	}

1. I'm wondering how the $group variable can be numeric as the bp_get_group() function is returning a Group object or false.
2. I believe checking the current group should happen before using the bp_get_group() function to potentially avoid a DB query if it matches the group id.
3. I was a bit confused to see changes about things that don't relate directly to the subject of the ticket. But let's keep these improvements and maybe make sure to split the changes in two commits: one to improve groups_get_slug() (<- side note: why this one is not using bp_get_group()?), groups_leave_group(), groups_join_group(), groups_update_last_activity() ; and one other for what's directly about the Group member count routine.

Instead of the above code, I'd probably do something like this:

	$group_id      = 0;
	$current_group = null;

	if ( is_numeric( $group ) ) {
		$group_id      = (int) $group;
		$current_group = groups_get_current_group();

		if ( $current_group instanceof BP_Groups_Group && (int) $current_group->id === $group_id ) {
			$group = $current_group;
		}
	}

	if ( ! isset( $group->id )  ) {
		$group = bp_get_group( $group );
	}

Remember BuddyPress can still be used with PHP 5.6 😉: the null coalescing operator (??) is not present in PHP version 5.6 or earlier. (I advise you to run grunt commit to see the phpcompat warning.)

I confirm the PHP Unit tests are OK (with or without my suggestion).

I believe we shouldn't change the kind of returned value for refresh_total_member_count_for_group() we just need to know if the count was refreshed, not the new member count.

[espellcaste]

@imath

1 - Good catch! The lack of test for this use case made me miss it.
2 - Your suggestion is good. Going to apply it.
3 - I understand the confusion and have no trouble in adding two different commits.
4 - Another good catch!
5 - Just to note it was already returning total count before. So it was not a new change but I can certainly remove that.

I'll send two new patches with the suggestions. :)

[imath]

Hi @espellcaste

Once I've seen the changes about checking the current group, I realized we were duplicating 3 times the same code. Sorry for realizing it late. I believe we should improve bp_get_group() instead. That's mainly what I'm suggesting into ​7614.4.recommandations.patch. I've moved the template's global check earlier to always try to use a group object we already got before requesting it via the DB. I've noticed some template functions were using 0 as the parameter, eg: bp_core_get_iso8601_date( bp_get_group_last_active( 0, array( 'relative' => false ) ) ) In this case 0 is numeric and the template's global was not used with the previous code of bp_get_group().

You might wonder why I'm introducing the 'bp_groups_set_current_group' hook. That's to:

• take in account early BP Rewrites need (the current group is set later during bp_parse_request. So it's a way to avoid the BP Rewrites plugin to generate doing it wrong notices.
• make sure the current group had a chance to be set before checking it.

Otherwise, the rest of the patch looks good. That's why I applied your patch and generated the .recommandations.patch after

You'll probably need to reference #6749 if you agree with these changes.

[espellcaste]

In 13103:

Improving the group member count routine and the function helper.

The group member count routine was updated to avoid direct, uncached, SQL query and unnecessary cache refresh when a group's Members page was viewed.

is now being used to get the group member count which takes into account users' existence in the site,

the query is now cached and filterable.

was also updated to get the current group from if available.

Props imath
Fixes #7614 and see #6749

[espellcaste]

In 13104:

Update group-related functions so that they use the new bp_get_group helper.

groups_get_slug, groups_leave_group, groups_join_group, and groups_update_last_activity were updated
to get a group using the recently added function bp_get_group, allowing them to be fetched with more than just IDs.

See #7614