Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5552 closed defect (bug) (fixed)

bp_is_current_component_core() breaks subnav for plugins that register themselves in component array

Reported by: boonebgorges Owned by:
Milestone: 2.1 Priority: normal
Severity: major Version:
Component: Core Keywords:
Cc:

Description

There's a check in the /members/single/plugins.php template https://buddypress.trac.wordpress.org/browser/tags/2.0/bp-templates/bp-legacy/buddypress/members/single/plugins.php#L13 that checks bp_is_current_component_core() before outputting the subnav (via bp_get_options_nav()). Introduced in r6783 (see #4772), this was intended to prevent duplicate navigation from showing up when subnavs were added to core items (like when a plugin adds a subnav item to Settings). But it appears to have had the side effect of hiding the subnav of *non-core* items.

So, for instance, take a plugin like Invite Anyone or BuddyPress Docs. They both add top-level nav items, and then add subnav items under them. In both cases, the subnav items do not show when using theme compatibility.

I think there are some semantic issues at play here. bp_is_current_component_core() doesn't actually check to see whether a component is a core component. It checks to see whether bp_is_active( $component ) is true. But this is true for any component that's bothered to register itself in the active_components array, which is any plugin that's "doing it right".

I don't have any definitive ideas about how to fix this. One brute force method is to remove the problematic check, and then to put a flag in bp_get_options_nav() that prevents it from firing twice on a single page. Another is to put a filter on bp_is_current_component_core() so that, at the very least, plugins can manually short-circuit the check when they know that they need the nav. Neither of these seem very elegant, so ideas welcome.

Attachments (3)

5552.patch (3.2 KB) - added by boonebgorges 5 years ago.
5552-plugins.php (2.0 KB) - added by boonebgorges 5 years ago.
5552.bp_is_current_component_core.patch (1.3 KB) - added by r-a-y 5 years ago.

Download all attachments as: .zip

Change History (20)

#1 @r-a-y
5 years ago

Yeah, I've run into this problem in the past and filtered 'bp_active_components' to remove the custom component from passing the bp_is_current_component_core() check. This is obviously a workaround.

#2 @boonebgorges
5 years ago

  • Priority changed from high to highest
  • Severity changed from normal to major

Just banged my head against the desk for another hour with this problem. I'm bumping the priority of this ticket to ensure it doesn't get lost.

#3 @DJPaul
5 years ago

  • Milestone changed from 2.1 to 2.2

Kind of reluctant to move to 2.2 but doesn't look like anyone has made progress on a patch yet.

#4 @boonebgorges
5 years ago

  • Keywords has-patch dev-feedback added
  • Milestone changed from 2.2 to 2.1

5552.patch is my suggested fix. It rolls back the bp_is_current_component_core() check in r6783 and then implements an improved version of my initial suggestion in https://buddypress.trac.wordpress.org/ticket/4772#comment:19 Summary:

  • In bp_core_load_template(), stash the requested template in the buddypress() object for later reference
  • In members/single/home.php, put a check at the top of the big if/elseif/elseif block that checks to see whether the requested template is 'members/single/plugins'. If it is, then load plugins.php

This fixes both problematic cases: 1. the original issue described in #4772, and 2. the missing subnav described here.

Note that this does involve a template-level change. For those themes that have overridden this template, the bug will persist. But this technique won't introduce any new regressions in that kind of theme.

It's not the most beautiful fix in the world, which is why I'm posting for feedback before committing. But I do think that we should go ahead with it for 2.1, as the status quo is much worse (and this is an underlying issue we can look at more with a template overhaul).

@boonebgorges
5 years ago

#5 @r-a-y
5 years ago

I appreciate the patch, boonebgorges. However, I don't think this is a problem any more on fresh installs. Just tested a fresh install myself with Invite Anyone and BP Docs and the subnav items show up properly.

In bp_is_current_component_core(), we grab the 'bp-active-components' DB option and then do component checks against this. This option is only supposed to have core BP components in its array.

Somewhere in an older version of BP (haven't looked), it was possible for BP to save 3rd-party components into this DB option as well. I'm guessing we were just saving anything that was registered into $bp->active_components into the bp-active-components DB option. This led to the problem as bp_is_current_component_core() shouldn't be checking 3rd-party components at all.

To be extra safe as well as for backward compatibility, we could:

  • Add a core function to return an array of BP core components.
  • Reference this function in bp_is_current_component_core() and do proper core component checks.

This should fix the problem. boonebgorges, what do you think?

Also, to test, can you dump bp_get_option( 'bp-active-components' ) on the problematic install and see if Invite Anyone or BP Docs is in that array?

Last edited 5 years ago by r-a-y (previous) (diff)

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.


5 years ago

#7 @DJPaul
5 years ago

  • Milestone changed from 2.1 to 2.2

So I am moving this back to 2.2, again, in light of R-a-y's comments. I don't mind this getting into 2.1 if the issue is still confirmed, but that investigation would need to happen very soon.

#8 @boonebgorges
5 years ago

Somewhere in an older version of BP (haven't looked), it was possible for BP to save 3rd-party components into this DB option as well.

Thanks for the research, r-a-y. I have come to the same conclusion. 5552-plugins.php demonstrates the problem - it only exists when the current component is in the active_components array.

I'm starting to think that my suggested solution is maybe not the best one. But still, I want to try to make the case one more time for a more substantial fix, at least for 2.2. Two considerations:

  1. The fact that it's only a problem for existing installations makes it less severe, but still pretty severe. Many thousands of installations.
  2. The current setup results in pretty wacky behavior, that avoids duplicated menu items only by accident. In 5552-plugins.php, Test1 (subnav of Settings) gets its bp_options_nav() from settings.php, while Test2 (subnav of Test2 Parent) gets its bp_options_nav() from plugins.php. This, despite the fact that plugins.php is used to render the template content in each case.

The more I think about it, the more I think that this is a problem that pervades our core template logic. The fact that we are required to load plugins.php in settings.php is a sign that something is wrong. Something like my original patch should be part of a future version of BP, but it should probably accompany a rethink of the way our template structure works. All calls to plugins.php should go through a single point in the stack, and that is not currently the case.

So, I guess I'm fine to punt this and do nothing for now, seeing as the solutions are almost as ugly as the problem. I can build workarounds into Docs and Invite Anyone, and I doubt many others are having the problem at the moment.

#9 follow-up: @r-a-y
5 years ago

When I use 5552-plugins.php in theme compat, the subnav for "Test 3 Parent" does not show up.

bp_is_current_component_core.patch follows what I stated in comment:5.

bp_core_get_components() is a new function that returns a hardcoded array of core components. This is a little ugly, but addresses the issue of the subnav for "Test 3 Parent" not appearing.

The fact that we are required to load plugins.php in settings.php is a sign that something is wrong.

bp_core_load_template( $whatever_is_here ) is for bp-default themes only.

When theme compat is triggered, theme compatibility correctly loads up 'members/single/home' on user pages:
https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-members/bp-members-screens.php#L433

bp-legacy, then, uses members/single/plugins.php, which is used as a template part.

Whereas for bp-default, plugins.php is a full page template. That's why we call bp_core_load_template( 'members/single/plugins' ). In an ideal world, bp-default's 'members/single/plugins.php' would be a template part like bp-legacy, then you could use bp_core_load_template( 'members/single/home' ) instead of bp_core_load_template( 'members/single/plugins' ) in plugins. However, it is what it is I'm afraid.

Last edited 5 years ago by r-a-y (previous) (diff)

#10 in reply to: ↑ 9 @boonebgorges
5 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from 2.2 to 2.1

Replying to r-a-y:

bp_core_load_template( $whatever_is_here ) is for bp-default themes only.

When theme compat is triggered, theme compatibility correctly loads up 'members/single/home' on user pages:
https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-members/bp-members-screens.php#L433

bp-legacy, then, uses members/single/plugins.php, which is used as a template part.

Whereas for bp-default, plugins.php is a full page template. That's why we call bp_core_load_template( 'members/single/plugins' ). In an ideal world, bp-default's 'members/single/plugins.php' would be a template part like bp-legacy, then you could use bp_core_load_template( 'members/single/home' ) instead of bp_core_load_template( 'members/single/plugins' ) in plugins. However, it is what it is I'm afraid.

I don't dispute this distinction between plugins.php for bp-default and bp-legacy. But that's not what I'm referring to when I say that something is wrong.

When you load Test1, here's the chain of events:

The displayed_user_nav() comes from home.php, options_nav comes from settings.php, and the tab content comes from plugins.php

In contrast, when you load Test2, the chain goes like this:

displayed_user_nav() comes from home.php and options_nav comes from *plugins.php*.

As a plugin author, I find this quite counterintuitive. In the case of Test1, it seems clear that my intent is to use plugins.php, *not* settings.php. Checking against a whitelist of core components is effective at making sure that we don't get odd duplicate (or missing) nav items, but it doesn't address the underlying illogic.

All of this points to a deeper problem, which is that it's problematic for template routing - the selection of templates based on the current URL/component/action - to be happening inside of templates. Templates are loaded from the outside in (home.php contains settings.php contains plugins.php), and when the routing logic is inside of the templates, by the time you get to settings.php it's not possible to say "whoops, this actually isn't a settings page". Contrast this with what good WP themes do: get_template_part( 'content', get_post_format() ); is a single line that leaves the routing logic out of the template, where it can be filtered/modified independent of the template load order. Obviously, moving toward this kind of system is a ton of work, but I do think it's at the root of the trouble we're currently having.

For 2.1, I think that 5552.bp_is_current_component_core.patch is better than nothing, and it will be fairly easy to roll back in case we decide to go a different direction in the future. So let's go with it.

#11 @r-a-y
5 years ago

In the case of Test1, it seems clear that my intent is to use plugins.php, *not* settings.php.

Ahh, didn't see the bp_get_template_part( 'members/single/plugins' ) call in members/single/settings.php. I see what you're getting at. Sorry for the misinterpretation!

You basically want to drop the middle-man in Test1. I can see the benefits of this for advanced plugins. Trying to think of a sane way to do this without adding a ton more file_exists() checks via bp_get_template_part() :)

When this is looked at again, could also look at what you mentioned in ticket:4855#comment:28.

#12 @r-a-y
5 years ago

In 8830:

Fix bp_is_current_component_core() to reference BP core components.

bp_is_current_component_core() is a function that is meant to check
whether the current component is a BP bundled core component. This is done
by grabbing bp_get_option( 'bp-active-components' ), which is supposed to
hold only BP core components. However, if a plugin manually saves its
component in the 'bp-active-components' DB option, our function will
now check this 3rd-party component as well, which is incorrect.

To address this issue, this commit introduces bp_core_get_components(),
which is a hardcoded array of BP core components, and references this
function in bp_is_current_component_core(). This ensures that only
BP core components are checked.

See #5552.

#13 @r-a-y
5 years ago

  • Keywords has-patch commit removed
  • Milestone changed from 2.1 to 2.2
  • Priority changed from highest to normal

For 2.1, I think that 5552.bp_is_current_component_core.patch is better than nothing, and it will be fairly easy to roll back in case we decide to go a different direction in the future. So let's go with it.

I've committed the fix.

boonebgorges - I'm going to move this to 2.2 due to comment:8 and comment:10. Or feel free to close altogether and create a new ticket.

#14 @johnjamesjacoby
5 years ago

r-a-y: thoughts on renaming bp_core_get_components() to bp_core_get_packaged_component_ids() or something more descriptive? get_components() seems ambiguous to me given the clarity we are trying to provide here.

#15 @r-a-y
5 years ago

thoughts on renaming bp_core_get_components() to bp_core_get_packaged_component_ids(

Works for me. I was kinda thinking the same thing.

#16 @djpaul
5 years ago

In 9012:

Core: only return forums from bp_core_get_packaged_component_ids() if using bbPress 1.

This otherwise causes the bbPress 2's subnav bar to not be rendered in templates due to the behaviour of members/single/plugins.php.

We would normally consider using bp_is_active for this kind of check, but because both bbPress 1 and (possibly, in the future) 2 could return true, we need a more reliable check for bbPress 1 only; see the ticket for full details.

See #5552. Fixes #5873, props r-a-y.

#17 @r-a-y
5 years ago

  • Milestone changed from 2.2 to 2.1
  • Resolution set to fixed
  • Status changed from new to closed

The main issue for this was addressed in 2.1.

I've split the other issue brought up by boonebgorges about our single page template routing to #6084.

Note: See TracTickets for help on using tickets.