[buddypress-trac] [BuddyPress Trac] #921: bp_group_has_members() - "order" parameter request

[buddypress-trac]

#921: bp_group_has_members() - "order" parameter request
-----------------------------------------------+---------------------------
 Reporter:  r-a-y                              |       Owner:
     Type:  enhancement                        |      Status:  new
 Priority:  normal                             |   Milestone:  Future
Component:  Groups                             |  Release
 Severity:  normal                             |     Version:
 Keywords:  has-patch needs-testing early-2.0  |  Resolution:
-----------------------------------------------+---------------------------
Changes (by boonebgorges):

 * keywords:  has-patch needs-testing 2nd-opinion early-2.0 => has-patch
     needs-testing early-2.0


Comment:

 921.03.diff is imath's .02.diff, but updated for the current trunk. There
 were a couple of random bugfixes and improvements mixed up in the original
 patch, which I separated into different commits. Also, .02.diff had some
 changes to parameter order in various functions that would've broken
 backward compatibility, so I thought this would be a good time to convert
 to using the parameters-as-array format.

 Specific thoughts on imath's approach:

 - Let's not pass parameters to
 BP_Group_Member_Query::get_group_member_ids(). Just get the value out of
 $this->query_vars.
 - I know we use the 'type' param (like type=last_modified) throughout
 BuddyPress, but I'm not a huge fan of it. It doesn't provide very much
 flexibility; 'order' and 'orderby' are much better, with 'type' being
 translated into 'order' and 'orderby' if necessary. Of course, now that I
 type this, I see that BP_User_Query doesn't support this syntax, so maybe
 it's an area for future enhancement.
 - Maybe instead of setting the $type in `get_group_member_ids()`, it'd be
 possible to do it in `setup_hooks()`. I don't totally remember the logic
 behind setting it in `setup_hooks`, but it looks like it might be
 reproducing (or reversing) some logic to do it later on as well. Play with
 it and see what you find.
 - The changes to `BP_Group_Member_Query` really need unit tests.
 - The special case for the Group Members page in
 `bp_legacy_theme_ajax_querystring()` is really ugly. Isn't there another
 way we can handle this? Why is 'object' not set properly by passing
 'group_members' to `bp_ajax_querystring()` in groups/single/members.php?
 I'm not sure about the `exclude_admins_mods` item - maybe we could set
 this in the JS somewhere (like you're doing with `template`)? or via a
 hidden input in the template that's then slurped up by the JS? or maybe in
 the AJAX handler? `bp_legacy_theme_ajax_querystring()` is really meant to
 be a generic utility function, so I'd really like to keep special cases
 out of it if possible.

 The rest of the approach looks good. The technique for determining member
 sort in BP_Group_Member_Query looks pretty elegant, and I like the change
 to `bp_legacy_theme_ajax_querystring()` and `bp_filter_request()` that let
 you pass a custom template. If we address some of the items I list above
 (especially unit tests! no messing with queries without them!) I think
 this'd be ready for 2.0.

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