Opened 10 years ago
Closed 10 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)
Change History (20)
#2
@
10 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
@
10 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
@
10 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).
#5
@
10 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?
This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.
10 years ago
#7
@
10 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
@
10 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:
- The fact that it's only a problem for existing installations makes it less severe, but still pretty severe. Many thousands of installations.
- 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 itsbp_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:
↓ 10
@
10 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.
#10
in reply to:
↑ 9
@
10 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 usebp_core_load_template( 'members/single/home' )
instead ofbp_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:
- members/single/home.php
- members/single/settings.php (https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-templates/bp-legacy/buddypress/members/single/home.php#L51)
- members/single/plugins.php (https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-templates/bp-legacy/buddypress/members/single/settings.php#L40)
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:
- members/single/home.php
- members/single/plugins.php (https://buddypress.trac.wordpress.org/browser/tags/2.0.2/bp-templates/bp-legacy/buddypress/members/single/home.php#L54)
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
@
10 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.
#13
@
10 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
@
10 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.
Yeah, I've run into this problem in the past and filtered
'bp_active_components'
to remove the custom component from passing thebp_is_current_component_core()
check. This is obviously a workaround.