BP_Core shouldn't run on 'bp_setup_components'

[r-a-y]

In bp-core-loader.php, the BP_Core class is responsible for includng all BP component code into WordPress.

The problem is we load up the BP_Core class on 'bp_setup_components' as well as any other components on the same action.

What we need to do is load BP_Core before 'bp_setup_components' so the other component's priority settings on 'bp_setup_components' will take effect.

Attached patch runs BP_Core on 'bp_loaded' at priority 0 since this runs before 'bp_setup_components' and fixes how 'bp_setup_components' is intended to run.

Also had to fix an issue with the members component's default_component.

[boonebgorges]

Cool. This looks less arbitrary than some of the other possibilities we discussed regarding the order of optional/required components. As you note, it makes total sense that bp-core would need to load first, since it's responsible for loading other components.

Let's put the default_component fix into another commit, but otherwise looks good to me.

[boonebgorges]

Crud, so this is going to be more complex than I'd thought.

Applying r-a-y's patch causes other problems. Now bp-members is running its setup_globals() before bp-xprofile, which means that the call to bp_core_get_user_displayname() is failing (eventually, it ends up in a query to a table name that does not exist)

I see two different paths we might pursue:

1. Settle on a component load order that we are happy with, and enforce it somehow
2. Make the necessary changes to prevent components from touching each other until they're fully loaded

Choice a is easier, but b seems better to me.

I've attached a patch that shows how we can easily do this in the case of the logged-in user fullname. Basically: use only WP data when setting up bp-members (because we can't be sure that xprofile is active or loaded). Then, when xprofile loads, overwrite the WP value (if necessary).

r-a-y, I know this all stems from my suggestion to use BP_Group_Member_Query in BP_Groups_Group::populate(), and that it results in a similar problem (because the current group gets loaded in bp-groups::setup_globals(), before the members component is loaded). I think we can do something similar there too. There's no need for user last_activity data when getting a group's admin mods, so we can just pass 'populate_extras=false' to BP_Group_Member_Query when populating the group object. This should prevent entanglement with the members component (and it'll prevent a needless query or two).

There may be other places where this kind of thing happens, and we'd have to handle them on a case-by-case basis. But I think this is the sound way to approach the problem.

To be clear, I think that r-a-y's original idea to load bp-core first still makes sense, even if we're going to make spot-fixes for individual component entanglements.

What do you think?

[r-a-y]

Basically: use only WP data when setting up bp-members (because we can't be sure that xprofile is active or loaded). Then, when xprofile loads, overwrite the WP value (if necessary).

I think 02.patch outlines more of a problem with bp_core_get_user_displayname() more than anything even though the patch makes logical sense.

We should add a filter to bp_core_get_user_displayname() and the xprofile component should filter this and add in its fullname when it's enabled.

Edit: I'm not fully awake! Me thinks we could also add a parameter so we can run the setup_globals() method based on a custom priority. This would be like the 'adminbar_myaccount_order' parameter in the component start() method. Does this sound like too much finagling?

There's no need for user last_activity data when getting a group's admin mods, so we can just pass 'populate_extras=false' to BP_Group_Member_Query when populating the group object.

Actually, we will need 'populate_extras' to be true because we will need to add in the is_admin / is_mod flags for each member in the query. This isn't being done yet in the BP_Group_Member_Query::populate_group_member_extras() method, but we will need it if we plan on emulating the $admin_mods variable in the populate() method.

Don't you love how one thing leads to another? :)

[boonebgorges]

Me thinks we could also add a parameter so we can run the setup_globals() method based on a custom priority. This would be like the 'adminbar_myaccount_order' parameter in the component start() method. Does this sound like too much finagling?

This sounds more like my suggestion 1 here ​https://buddypress.trac.wordpress.org/ticket/5436#comment:2 - enforcing a load order, rather than disentangling components. I'm not opposed to determining and enforcing the proper load order for other reasons, though FWIW I think it's not the most elegant workaround to the root problem of entanglement.

Actually, we will need 'populate_extras' to be true

Ugh, I guess that's right. An alternative is to do two separate BP_Group_Member_Query queries: one with 'role' => array( 'admin' ) and the other with 'role' => array( 'mod' ). This is not very beautiful. Another option is to rethink what 'populate_extras' means. Maybe setting is_admin/is_mod should not be an optional component of calling up BP_Group_Member_Query. This seems more elegant, though it would add a very small amount of overhead in some cases.

[r-a-y]

After thinking about this some more, I think 02.patch addresses the issue adequately though we have to do a spot check with our other components.

If we're talking of entangling, then we should also do what I initially suggested with bp_core_get_user_displayname() in comment:3.

---

I'm going to continue the discussion about BP_Groups_Group::populate() over on the other ticket (#5407).

[boonebgorges]

If we're talking of entangling, then we should also do what I initially suggested with bp_core_get_user_displayname() in comment:3.

Yeah, I agree that that's a good idea, but it won't fix the current problem. xprofile will catch the filter when bp-members is in its setup_globals(), but, as is the case now, the component won't be set up enough to replace the value. I've come up with a slightly modified (and, I think, more bulletproof) technique for the xprofile override. See 5436.03.patch

[boonebgorges]

(In .03.patch, ignore my changes to $bp->default_component. I was messing with something there, but the issue with default component setting is much more difficult because it's used immediately after it's set, which means there's no chance for another component to intervene. Maybe a problem to tackle in the future.)

[boonebgorges]

Circling back around to this, because I think that it's holding up a number of other items (#5407 in particular).

The basic problem is that some components are interdependent. In some cases, it makes sense to fix this by reconfiguring the load order of the components themselves. In others, the problem is that there's too much happening in a specific method, so that the interdependencies would be cleared up if the separate events were split into different methods that loaded on different actions.

In that spirit, 5436.04.patch builds on the previous patches, with some additional enhancements to straighten out additional errors I was getting in my testing. A summary:

• Introduce BP_Core_Component::setup_canonical_stack(), and move Groups and Members canonical_stack calculation here. Why do this? Canonical stack calculation requires the idea of a "default component". But default components are determined, in part, by looking at component slugs and $bp->pages. Both of these are set up during setup_globals() methods in various components. So, instead of juggling the order of Core/Activity/Members/Groups to make this sorta-kinda-work, we just do canonical stack calculation on a later priority.
• Move bp_setup_core() to fire at bp_loaded with priority 0. (Makes sense that core would need to run first.)
• Move xprofile overrides of loggedin_user and displayed_user display names into the xprofile component, so that BP_Members_Component::setup_globals() is no longer dependent on xprofile.
• Move bp_setup_xprofile() to priority 2. This ensures that it loads after core and members, but before everything else. I needed to do this to ensure that other components had access to xprofile display name data. It's not ideal, but it doesn't seem too arbitrary either - it makes sense that other components would need access to profile data at loadtime (but not necessarily the other way around).

In my light testing, this clears up all of the problematic dependencies. r-a-y and others, would really like your thoughts so we can get something into trunk early in this cycle.

[r-a-y]

I like the general idea behind the 'bp_setup_canonical_stack' hook, however I dislike the name for it because it's a little too specific. Other things could be used on this hook.

For example, the xprofile_override_user_fullnames() function in 04.patch could potentially be hooked to 'bp_setup_canonical_stack' within the BP_XProfile_Component class as a method, but the hook name doesn't make much sense.

Perhaps rename bp_setup_canonical_stack to bp_setup_late_globals?

[boonebgorges]

I think there's some benefit to our subhooks being specific. If someone doesn't like the specific name but wants to hook in at the same point, they could hook directly to 'bp_init'. You could make the same argument for, eg, 'bp_setup_nav'.

If we must go with a more general name, let's at least tie it somewhat to its intended functionality, rather than simply describing its load order as "late" (which is a bit of a misnomer anyway - it's pretty early in the BP load process). The key idea is that this is a chance for components to set up globals that (a) need to be run early in the load process, but (b) are dependent on all of the core components' being registered. So maybe something like 'bp_setup_secondary_globals'?

[johnjamesjacoby]

Boone hit the nail on the head here:

The basic problem is that some components are interdependent. In some cases, it makes sense to fix this by reconfiguring the load order of the components themselves. In others, the problem is that there's too much happening in a specific method, so that the interdependencies would be cleared up if the separate events were split into different methods that loaded on different actions.

I'm opposed to doing too much rearranging of actions or priorities until we can clearly identify the nuances that are causing issues.

The problem is we load up the BP_Core class on 'bp_setup_components' as well as any other components on the same action.

That's not actually a problem by itself. What actual problem(s) does this cause?

For predictability of use, the Core component should be subjected to all the same actions other components are (I'd marry it to the bp_include action if we could) so that it can easily be overridden, hooked into, etc...

Not saying this is the way we should go, but bbPress separates "common" code from "core" code (the same way we separate Member code from Core.) Common code includes API's used by m/any components, and Core code only includes the absolute bare minimum necessary to boot bbPress up and set it's hooks in.

Perhaps by simplifying what "Core" means to BuddyPress, we can better identify what needs to be loaded in what order, for what reasons.

[boonebgorges]

Thanks for chiming in, johnjamesjacoby, and thanks for the sanity check :)

I'm opposed to doing too much rearranging of actions or priorities until we can clearly identify the nuances that are causing issues.

I think that a fair amount of identification of nuances has happened in the discussion and patches above. In a nutshell: as BP has grown, we've lumped together the "component setup process" into chunks that are too large, and depending on accidental features of the WP plugin API (such as the fact that multiple callbacks attached to a single hook with the same priority of 10 will be fired in the order in which they were added) to paint over the fact that we're being too coarse-grained. This manifests itself in a couple different ways, but the overarching theme is that we're trying to do too much work in each component's setup_globals(): we set up basic info like slugs and database tables, then we determine stuff like whether we're looking at a specific user or group, then we do access control, then we do canonical URL setting, etc etc etc.

So, for example: if BP_Groups loads before BP_Members, it can't make reference to anything about the BP_Members component - even basic stuff like the members slug - when setting up its canonical URLs. This is a basic design failure. Each component should register its global values; *then* each component should determine necessary actions for the currently requested page; *then* each component should set up its default actions/canonical URLs; and so on. By breaking the setup routines into smaller chunks and then serializing them across components, we greatly decrease the potential for conflicts.

5436.04.patch is a very conservative attempt to address the worst parts of this problem. It takes one important part of component setup - setting up canonical URLs - and splits it into a separate BP_Component method that fires *after* 'setup_globals'. This fixes the worst problems. It also removes some of the hardcoded references to other components from setup_globals() into standalone filters. So, it doesn't solve all potential interdependence problems (and certainly doesn't break things up into the small chunks that I think would be ideal) but it solves all of the interdependency issues that actually plague BP at the current moment. I think that's enough for a first pass.

So, in the absence of argmuent to the contrary, I'm going to suggest that we go ahead with 5436.04.patch for 2.1, and we ramp up our vigilance regarding atomic methods in the future :)

Not saying this is the way we should go, but bbPress separates "common" code from "core" code (the same way we separate Member code from Core.)

For predictability of use, the Core component should be subjected to all the same actions other components are (I'd marry it to the bp_include action if we could) so that it can easily be overridden, hooked into, etc...

These distinctions are helpful, though I draw a different conclusion from them. In my view, what we call "BP_Core" is not really a "component" at all. It doesn't really set up navigation, or require its own tables, or do pretty much anything that the other components do. Nearly all of it counts as "common" in my view. And as such, it should *not* be subjected to the same actions as other components (and in fact, maybe it shouldn't use BP_Component at all). Probably a discussion for another day, and definitely needs more reseach (BP_Core is huge), but that's my initial impression.

[boonebgorges]

In 8610:

Untangle some load order and mutual dependency issues between component loaders

BuddyPress's components are dependent upon each other in numerous ways. All
components depend in one way or another on bp-core. Many components reference
bp-members functions and globals. References to bp-xprofile can be found in
a number of components. And so on. We have managed to make these dependencies
work on a case-by-case basis, though the system becomes less stable as it
becomes more complex. The fact that all components are loaded at
bp_setup_components introduces the possibliity of numerous race conditions that
may not cause problems on vanilla installations of BuddyPress, but can cause
fatal errors when trying to customize in various ways.

As part of an ongoing project to disentangle and regularize these dependencies
(see eg #5750), this changeset introduces a few enhancements that make the
dependencies between components more predictable, more serialized, and more
stable.

• In bp-members and bp-groups, move canonical_stack determination out of the main setup_globals() method and into its own setup_canonical_stack() method. This new method runs after all components have had a chance to run setup_globals(). As a result, the components are able to reference each other safely when determining default and fallback components for redirects (for example, the members component has access to the activity component's slug and other global data when determining whether example.com/members/boone/activity/ should resolve to example.com/members/boone/)
• In bp-members, use only WP core data to set up the loggedin_user and displayed_user global objects. Previously, these objects were configured by referenced to xprofile, which required that xprofile be loaded before members. Now, users' display names are first loaded from wp_users, and are overridden by the xprofile component via filter, only after xprofile has fully initialized.
• In bp-xprofile, move the Settings > Profile navigation setup routine out of the main setup_nav() task. bp-xprofile loads early, before bp-settings has had a chance to set up its top-level navigation. As a result, bp-xprofile must wait until the bp_settings_setup_nav action to add its Profile subnav item.
• Fine-tune the component load order. bp-core, which acts more like a library of common functionality than like a traditional component, is moved from bp_setup_components:10 to bp_loaded:0. This ensures that it is available to all other components. bp-xprofile, which must load after bp-members but at the same time is closely coupled to it, is moved to bp_setup_components:2, just after bp-members.

These changes are enough to make it possible to move forward on dependent
tickets; see, for example, #5407. It's likely that significant entanglement
and load-order issues remain. We'll continue to fix them as they arise, by
making load order more explicit, and by breaking the component bootstrap
process into increasingly discrete chunks.

Fixes #5436

Props boonebgorges, r-a-y