[buddypress-trac] [BuddyPress] #5170: bp_has_members() in widgets stomps $members_template global

[buddypress-trac]

#5170: bp_has_members() in widgets stomps $members_template global
-----------------------------+------------------------------
 Reporter:  johnjamesjacoby  |       Owner:  johnjamesjacoby
     Type:  defect (bug)     |      Status:  new
 Priority:  high             |   Milestone:  Awaiting Review
Component:  Members          |     Version:  1.0
 Severity:  major            |  Resolution:
 Keywords:  dev-feedback     |
-----------------------------+------------------------------

Comment (by johnjamesjacoby):

 Replying to [comment:1 boonebgorges]:

 Note: I'm still half thinking this all through, so any/all feedback is
 encouraged, especially since I'm accidentally proposing a large change in
 BuddyPress's behavior.

 > Thanks for the patch, jjj. My gut reaction here is that we're either
 over- or under-engineering this solution.
 Thanks for looking at it. You're not wrong about either case imo.

 > For one thing, what counts as "main" here is not clear. In the case of
 WP and $wp_the_query, *the* query stands apart from all others: it's the
 one determined by parsing the URL, it's the one that determines the top-
 level templates that are loaded, and all other queries on the page are
 essentially nested within it. On the other hand, in 5170.2.patch, the
 "main" query ends up being whichever one gets run first. For example, if
 you drop the following in a plugin, you'll see that *it* ends up being the
 main query:

 BuddyPress has never needed to use `parse_query` because of
 `bp_core_set_uri_globals` short circuiting it. Now that we're
 incorporating WordPress's Rewrite Rules API, which integrates directly
 with a `parse_query` router instead of our custom one, the concept of a
 "main query" could (should?) exist at the `parse_query` stage (similar to
 WordPress) rather than after `bp_core_set_uri_globals` and/or template
 output starts.

 > So, if you're running any members operations before the page loads, or
 if you have a members widget on the *left* (ie higher in the page
 rendering than the "main" loop), it'll end up being the main query. In
 this sense, the approach in 5170.2.patch doesn't do enough: if we wanted
 to keep track of the main query, we might need something like an
 'is_main_query' parameter for `bp_has_members()`.

 Right, that makes sense. In this first pass, I made the assumption that
 the first members query is always the main one, just to get it out of the
 door. We'll likely need to mirror the behavior of the `WP` class (maybe
 with a variant of `bp_core_set_uri_globals`) to do the routing and run the
 "main query" for whatever the context is. Your approach makes sense here.

 > On the other hand, I do wonder whether any of this is necessary (this is
 the "overengineered" part!). In the case of `$wp_the_query`, the main
 query is kept around for a bunch of reasons - eg, so that you can do
 `is_single()` etc checks later in the page. But what's the purpose of
 keeping around old `$members_template` values? It's not as if we ever have
 nested `bp_has_members()` queries (and if we did, I don't think this patch
 in itself would be enough to make them work). And our own functions like
 `bp_is_members_component()` work differently from WP's `is_` functions. So
 why not just chuck the queries? At the beginning of each call to
 `bp_has_members()`, why not just zero out the global? Or, keep
 `bp_reset_members_query()`, but just have it as a wrapper for `unset(
 $members_template )`, and then in the widget classes call it *before*
 running the widget's loop?

 Good questions, that come down to Rewrite Rules and `parse_query`. By the
 time `parse_query` is finished, BuddyPress should have all of it's `_is_`
 functionality filled in and ready to go, with additional logic built into
 them to check the main `$wp_query` global for the query-variables we set,
 based on the rewrite rule matches each BuddyPress component will set.
 (bbPress user rewrite rules is a good example of how it all comes
 together.)

 > So, if we're going to keep the "main" query stuff, we should be able to
 distinguish clearly which query is the "main" one, and we should be able
 to articulate why we'd ever need to make reference back to old
 Members_Template objects. Otherwise, we should just go with the simplest
 possible solution, which is to `unset()` it and forget it :)

 Hmmm... Yeah; it maybe isn't needed. Since BuddyPress does not combine the
 main `$members_template` to our `_is_` functions the way the main
 `$wp_query` global does, the separation may actually be a good thing. That
 said, the multiple calls to `bp_has_members()` problem still exists, with
 search and cookie parameters bleeding into subsequent calls. I'll open a
 new ticket for that, if there isn't one already.

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