Skip to:
Content

Opened 7 months ago

Last modified 4 weeks ago

#5170 new defect (bug)

bp_has_members() in widgets stomps $members_template global

Reported by: johnjamesjacoby Owned by: johnjamesjacoby
Milestone: 2.1 Priority: high
Severity: major Version: 1.0
Component: Members Keywords: dev-feedback
Cc:

Description

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.

Attachments (2)

5170.patch (3.9 KB) - added by johnjamesjacoby 7 months ago.
5170.2.patch (4.3 KB) - added by johnjamesjacoby 7 months ago.
set_main_query() is more flexible when not a static method

Download all attachments as: .zip

Change History (7)

johnjamesjacoby7 months ago

johnjamesjacoby7 months ago

set_main_query() is more flexible when not a static method

comment:1 follow-up: boonebgorges7 months ago

  • Keywords commit removed

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

comment:2 in reply to: ↑ 1 johnjamesjacoby7 months ago

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.

comment:3 DJPaul6 months ago

  • Milestone changed from Awaiting Review to 2.0

comment:4 DJPaul4 months ago

This was also reported in #5134

comment:5 boonebgorges4 weeks ago

  • Milestone changed from 2.0 to 2.1
Note: See TracTickets for help on using tickets.