[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     |
-----------------------------+------------------------------
Changes (by boonebgorges):

 * keywords:  dev-feedback, commit => dev-feedback


Comment:

 Thanks for the patch, jjj. My gut reaction here is that we're either over-
 or under-engineering this solution.

 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:

 {{{
 function bp5170() {
     bp_has_members();
 }
 add_action( 'wp_head', 'bp5170' );
 }}}

 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()`.

 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?

 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 :)

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