[buddypress-trac] [BuddyPress Trac] #7614: Group member count routine is bad

[buddypress-trac]

#7614: Group member count routine is bad
-------------------------------------------------+-------------------------
 Reporter:  boonebgorges                         |       Owner:
                                                 |  espellcaste
     Type:  enhancement                          |      Status:  assigned
 Priority:  normal                               |   Milestone:  10.0.0
Component:  Groups                               |     Version:  1.2.3
 Severity:  normal                               |  Resolution:
 Keywords:  has-unit-tests dev-reviewed needs-   |
  refresh                                        |
-------------------------------------------------+-------------------------
Changes (by imath):

 * keywords:  good-first-bug has-patch has-unit-tests => has-unit-tests dev-
     reviewed needs-refresh


Comment:

 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.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7614#comment:4>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac