[buddypress-trac] [BuddyPress] #4482: Better member type support in bp_group_has_members()

[buddypress-trac]

#4482: Better member type support in bp_group_has_members()
--------------------------+---------------------------
 Reporter:  boonebgorges  |       Owner:  boonebgorges
     Type:  enhancement   |      Status:  assigned
 Priority:  high          |   Milestone:  1.8
Component:  Groups        |     Version:
 Severity:  major         |  Resolution:
 Keywords:  needs-patch   |
--------------------------+---------------------------

Comment (by boonebgorges):

 trishasalas - Thanks for the patch. This is moving in the right direction,
 but there are some limitations in your approach. Mainly, if we're going to
 be able to query by a *single* role, it'd also be nice to query by
 *multiple* roles (eg, give me all admins + mods). Also, you're reproducing
 a big failing of the current implementation, which is JOINing against WP's
 `wp_users` table. This causes problems on certain sorts of installations,
 and it's something I'd like to factor out of BP to whatever extent
 possible.

 4482.patch is a first pass at the approach discussed above. As often
 happens, it ended up being a bigger job that I'd anticipated. Here's a
 brief summary:

 - Introduces `BP_Group_Member_Query`, which extends `BP_User_Query`. This
 required modding `BP_User_Query` in a couple of ways, mainly by way of
 abstracting certain bits so that they could be overridden by specific bits
 in the new class.
 - Deprecates `BP_Groups_Member::get_all_for_group()`, and replaces it in
 the `bp_group_has_members()` stack with `BP_Group_Member_Query` (with
 legacy support via filter, just like with `bp_core_get_users()` and
 `BP_User_Query`
 - Unit tests to make sure that the swap-out described above does not break
 backward compatibility.
 - Added a `'group_role'` param to the `bp_group_has_members()` stack,
 which supports multiple roles being passed.
 - Adds unit tests for all of the role combinations.

 Overall, I think the changes are pretty good. By eliminating the use of
 `get_all_for_group()` we are starkly decreasing the amount of SQL required
 (since we use `BP_User_Query` for limit/pagination and exclude arguments),
 while greatly increasing the functionality of `bp_group_has_members()`
 (since it'll now accept any `BP_User_Query` parameters. This is definitely
 moving in the right direction.

 I have mixed feelings about the technique I've chosen to build
 `BP_Group_Member_Query`. On the one hand, it'll make our life easier down
 the road if we use real OOP techniques, like I've suggested with
 `get_include_ids`. On the other hand, it's not very WordPressy - what I've
 done is pretty close to being a regular WP filter. The problem with WP
 filters, though, is that they're hard to juggle: you have to have a place
 for hooking them in the first place (see my `setup_hooks()` method), and
 then you have to remove them later on, because there might be more than
 one instance of a `BP_User_Query` on a page. The OOP technique is much
 cleaner in this sense. At the same time, we're already using a regular WP
 filter for `'bp_user_query_populate_extras'`. So what you see in my
 implementation is a mix of the two techniques: class inheritance with
 get_member_ids(), and WP filters with `populate_group_member_extras()`. On
 balance, when I think about the different techniques I could have used
 (and I have thought about a lot of them....), I think this is the best
 one.

 Any feedback welcome. If people like this technique, it can be extended to
 create `BP_Friend_Query` very easily, giving us a uniform approach for
 user queries throughout BP.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/4482#comment:13>
BuddyPress <http://buddypress.org/>
BuddyPress