bp_has_members() in widgets stomps $members_template global

[johnjamesjacoby]

bp_has_members() uses the $members_template, similar to the way query_posts() uses $wp_query. As such, subsequent calls to bp_has_members() leave the $members_template global in a polluted state that does not reflect the actual main query loop.

This results in some craziness, again, similar to WordPress core. Template functions tied to the $members_template global (bp_get_member_user_id(), bp_get_member_user_nicename(), et all) may return unexpected results when more than one call to bp_has_members() occurs on the same page output, and also leaves no way to get back the main query loop data.

To dance around this issue, WordPress uses the $wp_the_query variable, and uses wp_reset_query() to swap $wp_query back to it's natural state. My proposed patch does something similar, introducing a few static methods to the BP_Core_Members_Template class, and a bp_reset_members_query() helper function, which is patched to be used in our core widgets.

Worth noting, all of our _Template classes and helper function sets exhibit this same problem. Groups, Activity, Blogs, Messages, Settings, and XProfile, will all need similar attention. As the members component is always on, and the widget is frequently used, I'm using it as my first example.

[johnjamesjacoby]

set_main_query() is more flexible when not a static method

[boonebgorges]

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

[johnjamesjacoby]

Replying to 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.

[DJPaul]

This was also reported in #5134

[DerekFoulk]

Is there a fix for this. I see the fix above, but @boonebgorges made a comment that is making me nervous to use the patch... Will this be fixed soon in BP core?

If you guys say that the patch above is working and awesome, then I will use it. How do I use the patch attached above by @johnjamesjacoby ?

[boonebgorges]

DerekFoulk - If you need to run multiple queries on a single page, I'd suggest that you zero out the global yourself.

$GLOBALS['members_template'] = null;

or stash it somewhere if you think you'll need it later. Fixing this in BP is going to require more architectural work than we can use now. It's likely that jjj's patches will solve the immediate problem, but might have unexpected side effects.

[johnjamesjacoby]

Replying to boonebgorges:

$GLOBALS['members_template'] = null;

Is there value is doing this after the widget code is put out? I would advocate that it's currently unexpected behavior to have the $members_template global populated on any page other than the members directory just because there's a widget in a sidebar somewhere.

[hnla]

This appears to have possibly presented as an actual issue with group create button on twentyfifteen theme, details in #6096

[boonebgorges]

Is there value is doing this after the widget code is put out? I would advocate that it's currently unexpected behavior to have the $members_template global populated on any page other than the members directory just because there's a widget in a sidebar somewhere.

In the case of the widgets specifically, I think we can do something like this:

global $members_template;

$old_members_template = null;
if ( ! empty( $members_template ) ) {
    $old_members_template = clone $members_template;
}

if ( bp_has_members() ) {
// ...
}

if ( $old_members_template ) {
    $members_template = $old_members_template;
} else {
    unset( $GLOBALS['members_template'] );
}

It's kinda ugly, but it's pretty foolproof. And in the case of widgets, we can be confident (as suggested by johnjamesjacoby) that no one is expecting global pollution.

We probably need something more sophisticated for the more general problem.

[boonebgorges]

In 9327:

Explicitly declare block_self=false for group and blog create buttons.

The block_self parameter, which defaults to true, is designed to prevent things
like 'Add Friend' buttons appearing when viewing oneself in a member loop. In
the case of group and blog create buttons, the parameter is not really
meaningful. But in certain cases of global variable population (such as when
a members loop is run on the page before the group/blog buttons are generated),
block_self=true can cause these buttons to be improperly suppressed.

See #5170 for background on the problem.

Fixes #6096.
Props hnla.

[boonebgorges]

Just came across this again (using Twenty Fifteen, which renders sidebars before the main content). It's a very annoying problem. I'd like to think more about johnjamesjacoby's suggestions about compartmentalization of our template loops. But for 2.5, we can at least clean up after ourselves in the widgets.

[boonebgorges]

In 10553:

Restore template globals after widget loops.

BP template loop functions, like bp_has_members(), are responsible for
populating corresponding template globals. When this happens inside of a
"secondary" loop - such as a sidebar widget - the loop should clean up after
itself by resetting the relevant template global to whatever it previously was
before the secondary loop had run. (This is a rough approximation of WP's
wp_reset_postdata() for BP loops.)

Fixes #5170.