Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#3652 closed defect (bug) (fixed)

bp_core_new_nav_default() doesn't really work

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 1.6 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch dev-feedback
Cc:

Description

bp_core_new_nav_default() is supposed to let you set a default subnav for a nav item other than the one originally registered with the nav item.

It doesn't really work, because of this check: http://buddypress.trac.wordpress.org/browser/tags/1.5/bp-core/bp-core-buddybar.php#L116 The !$bp->current_action check will never return true, because bp_core_new_nav_item() will always set a current_action (http://buddypress.trac.wordpress.org/browser/tags/1.5/bp-core/bp-core-buddybar.php#L82)

Have a feeling this is going to be tricky to fix, because with $bp->current_action set, I don't think there's an easy way for BP to know the difference between, eg
http://example.com/members/boone/settings/
and
http://example.com/members/boone/settings/general/

Obviously, when you change the default, you only want to change the behavior of the former URL, not the latter.

Maybe when we manually set $bp->current_action in bp_core_new_nav_item(), we should set a flag in the global that says that we did so? Have to think about it. Really we should also think about content duplication issues here too; if /members/boone/settings/ shows General, then we shouldn't have another URL /members/boone/settings/general/. See #1741.

Attachments (1)

3652.01.patch (2.7 KB) - added by boonebgorges 9 years ago.

Download all attachments as: .zip

Change History (6)

#1 @boonebgorges
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed
  • Severity changed from minor to normal

This is indeed tricky. I can't figure out a way to fix this while maintaining any semblance of backpat with previous uses of bp_core_new_nav_default(). In a nutshell, here's how the current system works:

  • you add a nav item, including info about the default subnav, at bp_setup_nav, using bp_core_new_nav_item()
  • That function looks to see whether you need to be redirected to the default subnav; if so, it redirects you, and if not, it hooks the proper screen function.

So, in short, the redirection, etc happens *immediately after* the nav item is added, which leaves no obvious place to change the behavior. You need to have access to the nav item in order to change the default subnav (so you need to hop in after it's been added), but you need to alter it before the redirect happens, or you'll be too late to modify the nav default.

3652.01.patch is a proposed solution. Here's how it works:

  • The plugin developer sets up a function that calls bp_core_new_nav_default() inside of a function hooked *before* the nav item is added. Eg,
function bbg_set_settings_subnav(){
	bp_core_new_nav_default( array(
		'parent_slug' 	  => bp_get_settings_slug(),
		'screen_function' => 'bp_core_screen_notification_settings',
		'subnav_slug'     => 'notifications'
	) );
}
add_action( 'bp_setup_nav', 'bbg_set_settings_subnav', 5 );
  • bp_core_new_nav_default() adds this array to a nav_default_resets stack for later use.
  • A new hook has been added between the time when the nav item is added and the time when the redirect occurs (it's called bp_core_nav_default, and it appears in the middle of bp_core_new_nav_item())
  • A new function bp_core_reset_nav_default() is hooked to bp_core_nav_default. It looks to see whether, for that slug, any plugins have attempted to change the default subnav, and if so, it swaps out the information accordingly.

So, it's not totally backward compatible - those who are currently using bp_core_new_nav_default() will have to change their priority so that it happens *before*, rather than *after*, the nav item is added. And it's not particularly elegant. But it works. The only other alternative I see is to take the redirecting stuff and to put it much later in the BP loading process - which is fine, except that it will make the redirects happen more slowly by introducing more processing before they occur, which is IMO a bad thing.

Would be happy to get some feedback on this issue. Thanks.

#2 @boonebgorges
9 years ago

This probably works as expected again, given the way that the canonical redirect has been reordered. Needs testing.

#3 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#4 @boonebgorges
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [5559]) Fixes bp_core_new_nav_default() to work with the new canonical redirect. Fixes #3652

#5 @DennisSmolek
8 years ago

Anyone that comes across this in frustration, you have to add the 5 priority tag for this to fire correctly, otherwise it gets ignored.. I'm writing a codex page for this now...

Note: See TracTickets for help on using tickets.