A new API to manage single items navigation

[imath]

We've discussed about it yesterday during dev-chat, this is the navigation part i had previously included in the last patch i've attached to #6026.

For the blogs single items, it's a kind of "blocker", so the soonest we have this in, the best it is, of course soon doesn't mean quick and dirty :)

I've included a unit test so that's easier to understand the API. In it, i'm simulating a blogs single item, i build a nav for the admin and for a regular user and i remove the nav.

How i use it in #6026

Into BP_Blogs_Component->setup_globals(), i init the nav this way

$this->nav = new BP_Single_Item_Navigation( array(
	'component_id'     => $this->id,
	'component_slug'   => bp_get_blogs_root_slug(),
	'single_item_name' => 'blog',
) );

Then into BP_Blogs_Component->setup_nav(), once i'm sure it's a blogs single item we're viewing i build the nav and subnav items to finally populate the blogs single item navigation this way :

$this->nav->setup_nav( $item_main_nav, $item_sub_nav );

Then in blogs/single/ templates i use these two template functions to display the nav and the subnav :

• bp_single_item_main_nav() a wrapper for BP_Single_Item_Navigation->display_main_nav()
• bp_single_item_sub_nav() a wrapper for BP_Single_Item_Navigation->display_sub_nav()

Outputing a single item nav thanks to an object ID

That was r-a-y's suggestion in slack. IMHO, this is quite complicated. If we take the groups single item, the different elements of its navigation are built once the current group is set and about to be displayed (I actually do the same for blogs single items). So i think we should first make sure the API will behave correctly for our current way of setting single items nav. And work on this need for a future release.

Improvements to the API

About the class, i think we should have specific methods or filters to control the access to a page attached to a nav or a subnav. So far i've mimicked how it was managed in bp_nav and bp_option_nav and adapted it to a single item (eg: blogs or groups, but that wouldn't match for a displayed user as the bp_current_component() is not the user).

I'd love to have your feedbacks, (in particular @boonebgorges and @dcavins 's ones)

[imath]

Another way to test the API is to first apply ​6026.split.01.patch and then apply ​6534.patch :)

[boonebgorges]

imath - thanks for getting this started! I think it's a great start and most of the concepts in the patch seem solid to me.

Regarding r-a-y's suggestion about "ids". I'm not sure if I understand it correctly, but if I do, then his concern is similar to mine.

We've had a couple of historical problems with our nav system.

1. The distinction between bp_nav and bp_options_nav, and the inconsistency in their use between members and non-members (such as groups).
2. Index clashes for nav/subnav items caused by conflicting slugs between single items (such as a member and a group both called 'foo').
3. The inconsistent context-sensitivity of nav items. When looking at a group, bp_options_nav refers mostly to the displayed content - that is, the single group. But bp_nav is still largely configured relative to the *logged-in* user.

The technique in 6534.patch goes some way toward fixing this, because navs are stored in their components - buddypress()->groups->nav, etc. I think we can make the schema a bit more flexible by allowing one more level of abstraction, by item id/slug. So:

buddypress()->
    members->nav = array(
        4 => array(
            'friends' => ...
            'groups' => ...
        ),
        134 => array(
            'friends' => ...
            'groups' => ...
        ),
    )
    ->groups->nav = array(
        54 => array(
            'members' => ...
            'send-invites' => ....
        )
    )

In other words: index by component, and within each component, support the building of nav arrays on a per-item basis. (This could also be done like this: buddypress()->nav->members = array(), instead of keeping with the component. Doesn't matter much.)

In the long run, this would provide perhaps the most flexibility in building nav arrays. In the short run, for example, imagine you are logged in as user 4, and are viewing the profile of user 134. Using the schema above, it'd be possible for nav arrays to be registered both for you and for the displayed user.

On this scheme, functions like bp_core_new_nav_item() would need to accept additional params for component and item_id. Defaults would be based on the currently displayed user.

Backward compatibility with members (bp_nav) and groups (bp_options_nav[ $group_slug ]) could work by setting buddypress()->bp_nav = new BP_Nav_Backward_Compatibility() etc, a class that would implement ArrayAccess and would have __isset(), etc methods that would do the necessary lookups in the new storage locations. In this way, plugins that directly access buddypress()->bp_nav and buddypress()->bp_options_nav would continue to work.

Then something like what imath suggests in 6534.patch could be used as the primary interface for building and maintaining nav items.

This solves the above problems 1-3. 1 would go away because all items would be stored in the same component-id schema. 2 would go away because navs would be split between components, and single items within a component must have unique IDs or slugs. 3 would go away because we could build multiple navs in parallel (and we could bring back stuff like bp_get_loggedin_user_nav()!!).

Is this too ambitious? Is it even desirable?

[DJPaul]

I like what you've suggested here, Boone.

[imath]

boonebgorges, thanks a lot for your feedback.

We're BuddyPress, we can be ambitious :)

I think, i can see right away an interest for 2 "objects" : the logged in user and the displayed user. Because these 2 objects are set quite early in the process.

But for the blogs single item or the groups one, only one is set early the displayed one. There's no logged in blog or group. Both navs are set only if we are about to see a blog or a group + we do some checks like bp_is_item_admin() to include or not some nav items.
So what i was thinking about r-a-y's point (unless i misunderstood) is.. Let's take an example. I'd like to display the nav of a group directly into the front page of my blog. I think the way it's built in the component's loader it would really be a massive change to let's say have a function like this bp_single_item_display_nav( 45, 'groups' ) to display the nav.

But why not :) So i've edited the patch (see 6534.item_id.patch) to build a nav the way you suggested:

• $bp->component_id->nav->main[ $item_id ] = array( $nav_items )
• $bp->component_id->nav->sub[ $item_id ] [ $main_nav_item ] = array( $sub_nav_items )

+ i remembered some plugins like BP Groups Hierarchy (i think) can generate group permalinks like this > site.url/groups/group_parent_slug/group_slug so i've added a mechanism to eventually use bp_get_group_permalink()

+ i think we'll need an extra property in the BP_Groups_Group and BP_Blogs_Blog objects because the id in this last one is not the blog_id but the id of an association between a user and a blog. So i've used item_id.

Then i've tested it with the unit test and an adapted version of 6026.split.01.patch (i will add the version 6026.split.02.patch to #6026).

It's working fine. But a feature like: bp_single_item_display_nav( 45, 'groups' ) is imho complicating a lot.

[boonebgorges]

But a feature like: bp_single_item_display_nav( 45, 'groups' ) is imho complicating a lot.

Yeah. But if we're going to break everything and build a compatibility layer, let's try to build a system that's maximally flexible.

Plus, I do think that we should have intelligent defaults that make this schema invisible to existing plugins. So, eg, bp_core_new_subnav_item(), if called without an item_type and item_id, should have internal logic that looks schematically like this:

if ( ! $item_type ) {
    $item_type = bp_current_component();
}

if ( ! $item_id ) {
    $item_id = apply_filters( 'bp_nav_item_id_fallback', null );
}

$nav = new BP_Single_Item_Navigation( array(
    'item_type' => $item_type,
    'item_id' => $item_id,
    // etc
) );

It looks like you have some of this fallback logic happening in your patch, but I got a little lost with what appears to be the half-implemented buddypress->single_item_id stuff.

Do we need to maintain the distinction between bp_nav (main in your patch) and bp_options_nav (sub in your patch)? Can't we put them all in a single place, like WP_Admin_Bar does, and then identify top-level items as those without a 'parent_slug'? It seems to me that it would help a lot to simplify our system if we had a single interface for *all* nav items.

Speaking of interfaces, I wonder if we might talk broadly about separating *item nav* from *item nav items* in the code. We'd be able to enforce better principles of encapsulation if we had a class BP_Item_Nav with a property nodes that was an array of BP_Item_NavItem objects. (I know that you haven't done this in part because you've copied and pasted from existing nav_item() functions.)

imath, what do you think of the above? Let's talk about it a little bit before you go and whip up another patch :) I want to dig in myself and start working with code, but I want to be clear on the high-level strategy first.

[imath]

oh yeah sorry about buddypress->single_item_id i should have explained this a bit more. It's the way i've used to build the title_parts in bp_modify_page_title() as the nav is now relative to an item id.

About the above, i'm okay with your plan, as you said:

let's try to build a system that's maximally flexible.

So a big +1

[DJPaul]

@imath, I am assuming this won't be finished for 2.4 and have moved to 2.5 milestone.

[espellcaste]

Just to reinforce on the debate, as I've had a few problems with the BuddyPress navigation in the past, #6085. There are some areas I see could be improved. Firstly let me say that I find @boonebgorges idea/suggestions better as recreating the way he outlines will bring better organization, clarity and maybe will avoid and fix current problems with the BuddyPress navigation.

The areas I think could be improved are:

• Item creation both from top level items as well as for sub-items;
• Menu order modification - Maybe make it easier to change the order of appearance of items in the nav and subnav. The problem that I had on #6085 was partially because I removed and change names and order from the navigation and sub-navigations;
• Removal of items and sub-items.

Mainly and basically, make it easier for developers to remove, change, add items and sub-items without having much conflict. As I had to remove, add, change names and orders. Creating a custom organization, that's what I found difficult and had to deal with some problems.

[DJPaul]

I'm guessing that a 9 month old patch needs a refresh.

@r-a-y You resurrected this ticket. Do you intend to work on this for 2.6?

[imath]

I've been thinking about this ticket and i'd like to suggest another approach. I think the main problem is arrays are not "enough" to store our nav items. So instead i'm suggesting to use a new object that will build the nav for a given item id (group or member).

6534.02.patch is using a new class BP_Core_Item_Nav to build navs for the displayed user and another one for the displayed group so that we avoid slug collisions. Patch is fixing #5103 (you can test by creating a group named "profile" before & after applying the patch)

As the Group has now the full control over the main nav, i guess we could use it for the the future release in a more interesting way i'm doing in the patch (i mimic the old bp_options_nav).

So i think unless somebody is directly setting/playing/editing $bp->bp_nav or $bp->bp_options_nav this patch shouldn't break anything and it will allow me to carry on working on #6026 ;)

Feedbacks welcome :)

[imath]

6534.03.patch is trying to take in account most of the points @boonebgorges and I discussed about (wednesday after the dev-chat on slack) and tries to come closer to what he an I are dreaming of about the BuddyPress Navs :)

1. It improves some names (but as i'm not the best in this area i guess it can still be improved)
2. It now uses a single object to store the navs (primary & secondary) and uses the parent_slug to get the children of the primary nav.
3. Some functions has been added to the BP_Core_Nav class so that it's easier to add/edit/delete any item of the navs. There's also a function i've specially created for something i wasn't aware about > bp_get_nav_menu_items This function gets the primary nav and nests the secondary one as a children argument of the primary.
4. It's also easier to request the nav items according to their parameters. For instance for bp_get_nav_menu_items i was able to directly remove the nav that were not 'user_has_access' === true
5. I've made sure everything was ok with the document's title part (<title>)
6. The Group's has its own nav making sure to fix #5103
7. Backward compatibility for users directly accessing to buddypress()->bp_options_nav or buddypress()->bp_nav is done thanks to the Backcompat class which implements ArrayAccess (By the way it's a real pain to manage multidimensional arrays with it!) If a user does, i'm sending a ​`_doing_it_wrong`
8. I've deprecated some nav functions and introduced new ones.

I still need to edit the unit tests, but before doing so, i'd like to make sure we're on a good road! Feedbacks welcome :)

I think it's a nice improvement and i really hope we'll have new BuddyPress Navs soon!

On a side note, i'd like to understand why The Groups Extension API uses a specific way for adding its navigations (in the display method). I was a bit surprised about it, but left it the way it was to avoid breaking things in this part..

[r-a-y]

Great work, @imath!

For point 7, I made a few changes to ensure that devs accessing the $bp->bp_nav and $bp->bp_options_nav globals manually should still work. In order to do this, I made changes to the BP_Core_BackCompat_Nav and BP_Core_BackCompat_Options_Nav classes. The code is a little ugly because I needed to create a few $bp singleton markers to keep track of the current component and action - buddypress()->backpat_nav_component and buddypress()->backpat_nav_action. See the set_component_props() method for more details.

This ensures that something like this still works:

// The old way of manually changing BP nav menu props.
add_action( 'get_header', function() {
        // User page.
        buddypress()->bp_nav['activity']['name'] = 'Ray Activity';
        buddypress()->bp_nav['friends']['name'] = 'Ray Friends';
        buddypress()->bp_options_nav['activity']['mentions']['name'] = 'Ray Mentions';

        // Single group page.
        $group_slug = bp_current_item();
        buddypress()->bp_options_nav[$group_slug]['home']['name'] = 'Ray Activity';
} );

Hopefully, the set_component_props() technique can be removed with something a little more elegant.

I also fixed a bug with the user's "Profile" nav not showing up and added a debug trace to the deprecated nav functions so devs can easily find out where the deprecated nav calls are coming from.

[imath]

Thanks a lot @r-a-y for your feedback.

About point7: your version of the patch is only making sure editing the nav is working, because if i add this line to your snippet:

var_dump( buddypress()->bp_nav['activity']['link'] );

I'm getting a null value although it's set. In previous patch, i worked on setting new navs directly and your version removed it :(

More globally point 7 will be a real pain, because ArrayAccess *is not designed* to take care of multidimensional arrays. So i feel this backcompat class will be a never ending story. So i doubt we'll ever have this ticket fixed if we need to take care of any backcompat issue for people playing directly with vars instead of using available functions or filters...

But so be it! 6534.04.patch should fix the issue you shared without creating new buddypress() global vars ;)

About:

I also fixed a bug with the user's "Profile" nav not showing up and added a debug trace to the deprecated nav functions so devs can easily find out where the deprecated nav calls are coming from.

It's only happening when viewing another user's profile. Good catch. I've included your fix in 04.patch (i've spent an hour finding it as svn is formatting the diffs differently than git!)

About the trace thing. I haven't included it in 04.patch. Although I think it's interesting, why doing this for these specific deprecated functions and not the others ?
(ideally i think it should live in an upstream ticket)

[boonebgorges]

I know that @imath is concerned about backward compatibility holding up this ticket, so I decided to dive in and do a BC review. 6534.05.patch​ contains a number of items that improve (and demonstrate) backward compatibility. (I have not done a full patch review yet - I will do so in the upcoming days, but I wanted to get this part of it up for review first.) A summary:

I wrote a bunch of unit tests that describe various ways one can interact with the bp_nav and bp_options_nav global, and got them to pass against the current BP trunk. See the globalAccess.php file in 6534.05.patch​ (they probably won't run on trunk because of some mods I made during development, but with a bit of massaging they could be made to do so). I also tested using the following function on the front end: ​https://gist.github.com/boonebgorges/6025ad2e0f7ab8b42b26c965773d8805

I think I managed to synthesize the approaches made by @imath and @r-a-y. @imath's patch wasn't extensive enough to provide BC for the most common mods to the global, while @r-a-y's patch broke in a number of ways for me, and I think was overly complex because it was trying to be truly recursive. Figuring this out took me most of a day, and would've taken much longer without looking at your clever patches, so thanks :)

The gist linked above, and the unit tests, show the extent of the BC that is covered by the patch. Here are some uses that are *not* covered:

// Unsetting a single property of a nav item. The new nav API doesn't support malformed items very well.
unset( $bp->bp_nav['foo']['name'] );

// Sorting. This will probably give you a fatal error.
sort( $bp->bp_nav );

// Looping using foreach. 
foreach ( $bp->bp_nav as $key => $value ) { ...

There may be others. It's likely that people are doing these things, but I'd wager that it's very rare compared to the basic set/get/isset/unset actions that are covered by 6534.05.patch​. I guess it's worth looking through the plugin directory to see if unsupported mods are in widespread use; it's possible to support them with some fancy use of ArrayObject, but I don't want to go to the trouble for extreme edge cases.

A couple other notes on my BC implementation:

1. I renamed the BC classes and put them into separate files

2. I added a check that ensures that they're only loaded when ArrayAccess is available. SPL can be disabled on PHP 5.2.x (though it is enabled by default), so we want to be sure not to cause fatals. On these installations, there will simply be no backward compatibility support for bp_nav and bp_options_nav - a fair compromise, I think.

3. Another BC syntax break I noticed was that it's no longer possible to do this:

$nav_item = $bp->bp_options_nav['foo']['bar'];
$css_id = $nav_item['css_id'];

because the $nav_item is a stdClass. Even if we're no longer going to use this syntax in BP, I think it's wise to support its legacy use in plugins/themes. So I introduced BP_Core_Nav_Item, which extends ArrayObject when available, and used that class in BP_Core_Nav instead of $nav_item = (object) $args. This seems like a small and easy gesture we can make for BC. Having a separate class for nav items also opens the door for other OOP techniques in the future.

4. Seems like if we're going to throw _doing_it_wrong() notices, we should do it whenever the global is touched, not just when an offset is provided. So I made that change. If I'm missing something, go ahead and change it back.

5. Another BC break that we should probably document or mitigate is that this will no longer work:

bp_core_new_subnav_item( array(
    'name' => 'Foo',
    ...
    'parent_slug' => bp_get_current_group_slug(),
    ...
) );

Currently, this will add a subnav to the current group. But the new bp_core_add_subnav_item() will only know to add to a group nav (rather than members) if 'groups' is passed to the $component parameter. In BP_Core_BP_Nav_BackCompat::get_component(), I do this: if bp_is_group(), and the nav item slug matches the slug of the current group, I assume that you intend to be modifying the group nav. (This won't be true in absolutely every case, but 99.999% of the time.) I see that something similar is happening in the deprecated _remove_ functions. I'd suggest adding it to bp_core_new_subnav_item() too.

Phew - I think that's it for the BC stuff. Once we address the above points, I think we'll be in a good place from the compatibility point of view.

I will come back soon with a more complete review, but first a small note, before I forget: The function names bp_core_add_nav_item() and bp_core_add_subnav_item() were once in use in BP, and removed in [2168]. They were replaced by the current functions for the same reason we're using the names to replace what we've currently got - because we need easy synonyms for changed syntax. Probably not worth worrying about, but I thought I'd throw it out there :)

[imath]

Hi @boonebgorges

Thanks a lot for your great improvements. I confirm the unit tests and the front end tests you shared are working fine for me.

I've noticed you iterated from r-a-y's version of the patch. So i have questions... Why defining the $bp->options_nav again inside the groups component? If i comment this part in bp-groups/classes/class-bp-groups-component.php, your patch is still working:

// Backward compatibility for plugins modifying the legacy bp_options_nav global property.
/*if ( buddypress()->do_nav_backcompat ) {
	$bp->bp_options_nav = new BP_Core_BP_Options_Nav_BackCompat();
}*/

Do we really need to reset $bp->options_nav as it was already set in the members component ?

About the deprecated functions, what is the reason that makes these functions different than other deprecated functions since r-a-y and you are adding a trace to the deprecated notice ? My point is why doing it here and not for all deprecated functions ?

[boonebgorges]

OK, another round of review. Advance apologies for what is going to be a long comment :)

6534.2.diff​ is my current proposed patch. I'll explain its partner 6534.diff​ below.

Do we really need to reset $bp->options_nav as it was already set in the members component ?

No. In fact, I think we should do it once, in BP_Core. My new patch does this.

About the deprecated functions, what is the reason that makes these functions different than other deprecated functions since r-a-y and you are adding a trace to the deprecated notice ?

I have mixed feelings about the utility of full stack traces (anyone who needs one is probably already using something like xdebug). But in any case, I agree that it's probably not appropriate to sneak it into this ticket. @r-a-y, if you think it would be a good improvement throughout BP - or perhaps just for those deprecated notices that show up most frequently - let's talk about it in a separate ticket.

In 6534.2.diff​, I undeprecate bp_core_new_nav_item() and bp_core_new_subnav_item(). @imath, I'm guessing that you deprecated them because, on the new system, adding a nav item (specifically, a subnav item) requires the specification of a $component. But I think that the deprecation notices are overkill. Falling back on 'members' works fine in most cases. The only problematic case is where someone is using bp_core_new_subnav_item() with 'parent_slug' set to a group slug to add a subnav item. This is my point 5 in comment 25. As suggested there, I think we can and should provide BC for this use case, and I think I have a pretty good heuristic for doing so (see the patch, and my comment below, for more details). So it's extremely likely that in nearly every single case, calling the existing functions will continue to work as expected. For this reason, I don't think there's a good reason for us to fill everyone's error logs with deprecation notices. (I had to turn hack around WP_DEBUG just to test the original patches, given all the plugins that use these functions!) For the same reason, I think we can probably undeprecate the _remove_ functions, though IMO it's less critical. (The _doing_it_wrong() notices for touching bp_nav and bp_options_nav should stay.)

I feel pretty strongly that we should *not* deprecate these functions, but if everyone disagrees with me, 6534.diff​ is a version of 6534.2.diff​ that retains the deprecation.

On the subject of 'groups' and bp_core_new_subnav_item() backward compatibility. A plugin of mine (and @r-a-y's) uses the same trick to add group subnav items (sub-subnavs!) as BP core uses for the Manage subnav: a parent nav item of {$group_slug}_manage (or, in our case, {$group_slug}_events). See eg ​https://github.com/cuny-academic-commons/bp-event-organiser/blob/master/bp-event-organiser-groups.php#L140. Let's set aside for a moment the fact that this is a terrible technique, and something we should fix down the road by making group navigation top-level. We can do this in the future. For now - if @r-a-y and I are doing this, it's likely someone else is. So I added some horrendous logic that says: this is probably a group navigation item if (a) the parent slug === the current group slug, OR (b) the current group slug appears at the very beginning of the parent_slug, and there is no Members nav item with the parent_slug (to avoid the very slim possibility of false positives). It's pretty ugly, but it works.

I did some juggling to the way that you reorganized the _create_link() and _hook_screen_function() stuff, so that return values and do_actions() made a bit more sense.

I crawled my way through the unit test suite to ensure that everything passes. This means deleting the bp_core_sort_nav_items() tests - this function no longer does anything. Working through the tests showed a couple more minor BC breaks that should be noted. The most significant is that it's no longer possible to add subnav items with a parent_slug that points to a non-existent nav item. I'm guessing that no one is actually doing this in production, since it results in nav items that don't render on the front end, but we were doing it in a couple places in the tests in order to "fake" the navigation.

I did some documentation tidying throughout.

An earlier patch had some modification to the way bp_nav_menu_get_loggedin_pages() works, in order to account for the fact that nav items are now classes rather than arrays. This was pretty confusing to me, since _loggedout_ didn't require the same treatment. Since I implemented ArrayObject for nav items anyway, I standardized the treatment.

==

I feel pretty good about 6534.2.diff​. @imath and others, I'm especially interested in your thoughts about deprecated functions. My inclination is to undeprecate the _remove_ functions as well. Once we make a decision about this, I think we are close to having something ready to be committed.

[imath]

Thanks a lot @boonebgorges for these new improvements!!

I 100% agree with you about "undeprecating" the functions, i did this in the first place because of one of our talk after a dev chat. So i'm ok to keep the add_new / _remove_ functions.

About

by making group navigation top-level. We can do this in the future.

Yes first i think it's best to make sure the new nav is working great and in next release(s), we'll be able to enjoy the "primary nav" for groups. Because i think this will probably require some new edits of how we control the access to screens...

I also feel pretty good about 6534.2.diff :)

[boonebgorges]

Great!

I think we should build on 6534.2.diff​, remove the deprecation of the _remove_ notices, and get this into trunk. Do you want to do the honors?

[r-a-y]

Looks good! Thanks for all your work on this, boonebgorges and imath.

Please do remove my debug trace stuff. I can turn that on manually if needed.

[boonebgorges]

In 10743:

Add tests for bp_core_remove_nav_item().

See #6534.

[boonebgorges]

In 10744:

Add tests for bp_core_remove_subnav_item().

See #6534.

[boonebgorges]

In 10745:

Introduce new API for BuddyPress navigation.

The new BP_Core_Nav overhauls the way that BuddyPress registers, stores, and
renders navigation items. Navigations are now component-specific, eliminating
the potential for confusion and conflict between navigation items with similar
names in different components, and opening the possibility of generating navs
for separate objects of the same type on a single pageload.

The new nav API replaces the old bp_nav and bp_options_nav system, which
dates from the earliest days of BuddyPress. These global properties were
responsible for handling nav and subnav across all of BP's components. The data
structure of bp_nav and bp_options_nav was simultaneously too opaque (in the
sense of being difficult to approach for developers and not having a complete
interface for programmatic modification) and too transparent (forcing devs to
manipulate the global arrays directly in order to customize navigation). The
new system eliminates most of these problems, by removing direct access to the
underlying navigation data, while providing a full-fledged API for accessing
and modifying that data.

An abstraction layer provides backward compatibility for most legacy uses of
bp_nav and bp_options_nav. Plugins that read data from the globals, modify
the data (eg $bp->bp_nav['foo']['name'] = 'Bar'), and unset nav items via
bp_nav and bp_options_nav should all continue to work as before. Anyone
accessing the globals in this way will see a _doing_it_wrong() notice. This
backward compatibility layer requires SPL (Standard PHP Library), which means
that it will not be enabled on certain configurations running PHP 5.2.x. (SPL
cannot be disabled in PHP 5.3+, and is on by default for earlier versions.)

The new system breaks backward compatibility in a number of small ways. Our
research suggests that these breaks will affect very few customizations, but
we list them here for posterity:

• Some array functions, such as sort(), will no longer work to modify the nav globals.
• Subnav items added to nonexistent parents can no longer be successfully registered. Previously, they could be loaded into the global, but were never displayed on the front end.
• Manual management of group navigation items previously worked by passing the group slug as the parent_slug parameter to the bp_core_*_subnav_item() functions. The new API requires specifying a $component ('members', 'groups') when accessing nav items. To provide compatibility with legacy use - where $component was not required - we make some educated guesses about whether a nav item is "meant" to be attached to a group. This could result in unpredictable behavior in cases where a group slug clashes with a Members navigation item. This has always been broken - see #5103 - but may break in different ways after this changeset.

Props imath, boonebgorges, r-a-y.
Fixes #5103. Fixes #6534.

[boonebgorges]

I used the changeset above to close this ticket. The discussion above is mainly architectural, so I think it's appropriate to mark the current ticket fixed, and to handle regressions and other issues in separate tickets.

Thanks for playing along, everyone. I think this is the biggest internal mod we've ever made to Modern BuddyPress (at least since BP_Component), and I think we've pulled it off pretty nicely.

[boonebgorges]

In 10752:

Nav backcompat: Check the existence of an offset before fetching it.

Helps avoid PHP notices when accessing the bp_options_nav backcompat layer
after calling bp_core_remove_nav_item().

The error only appears when running versions of PHP earlier than 7.0, which is
why it was inadvertently introduced in [10745]. See #6534.

Fixes #7062.

[boonebgorges]

In 10753:

Tests: Don't use assertArrayHasKey() on bp_nav.

The nav backward compatibility layer does not support this method for all
versions of PHP.

See #6534.

[boonebgorges]

In 10874:

Use correct 'link' for top-level items in bp_get_nav_menu_items().

The incorrect variable name was used when the new nav system was introduced
in [10745]. See #6534.

Fixes #7110.

[dcavins]

I just noticed a bug when attempting to call
bp_core_remove_subnav_item( $parent_slug, 'send-invites' );
for a group-related sub-navigation item.

The underlying issue is that the when $component isn't specified in bp_core_remove_subnav_item(), and we think it's a group-related item, we call bp_core_remove_subnav_item() again, specifying the $component as groups. The parameters were being sent back to the function in the wrong order ($parent_slug and $slug were reversed).

6534.param-order-fix.patch fixes the parameter order problem.

I'm wondering if it wouldn't be more efficient to just set $component and continue, though, rather than starting the function over with the $component set (which means buddypress() will be called again). So the other patch, 6534.set-component-instead.patch takes the low road and simply sets the $component.

[dcavins]

Correct the parameter order in the 'groups' backcompat case of bp_core_remove_subnav_item().

[dcavins]

In the 'groups' backcopat case for bp_core_remove_nav_item() and bp_core_remove_subnav_item(), set the component and continue.

[imath]

@dcavins good catch, i think i'd go with 6534.set-component-instead.patch but i'd like @boonebgorges 's confirmation ;)

[boonebgorges]

I started writing tests to demonstrate the bug, but got a bit confused. What is the purpose of the "backward compatibility" block in bp_core_remove_nav_item()? Was it previously possible to remove a group nav item using this function? I thought you had to do bp_core_remove_subnav_item( array( 'parent_slug' => $group_slug ) ).

[imath]

@boonebgorges you're right, it's a bit confusing. Before all group nav items were subnavs of the group slug main nav
​https://buddypress.trac.wordpress.org/browser/tags/2.5.3/src/bp-groups/classes/class-bp-groups-component.php#L488

Meaning if you did bp_core_remove_nav_item( $group_slug ) it would remove the group slug nav item and all group's subnav items.

In 2.6, we don't "fully" enjoy yet the primary nav for the group, we just set one nav that is the group's slug this way even if it's a different object it still looks like before, all group nav items are in the secondary nav of the group's slug primary nav.

So i guess the test for bp_core_remove_nav_item( $group_slug ) is to check $bp->groups->nav is empty.

That's probably the reason why you added this backward compat block in the first place.

Now if you only need to remove a specific nav of a group (which was a subnav and is now a secondary nav), you use bp_core_remove_subnav_item( $parent_slug, $slug, 'groups' ) where the parent slug is the group slug and the slug is the one of the group nav (eg: members).

The problem @dcavins found is that you were using bp_core_remove_subnav_item( $slug, $parent_slug, 'groups' ); instead. So a way to resolve is to change the order of parameters (eg: bp_core_remove_subnav_item( $parent_slug, $slug, 'groups' )) that's what did @dcavins in 6534.param-order-fix.patch or a simpler way is to set the $component to 'groups', that's what he did in 6534.set-component-instead.patch.

For the bp_core_remove_subnav_item i guess the test is more to check if the 'members' secondary nav is the only one deleted. I'm not sure to be clear, am I ?

[dcavins]

Hi @boonebgorges- I think you're right in that the only backcompat case is in bp_core_remove_subnav_item() for the case used in "Invite Anyone" with the group slug being passed as the $parent_slug. I've written a test that I think should show this case and will attach it.

[dcavins]

Add test for bp_core_remove_subnav_item() backcompat.

[boonebgorges]

Thanks, @imath and @dcavins. My takeaway from the above is that the only real bug is in bp_core_remove_subnav_item(). But if we're going to change that logic to set $component = 'groups', then we may as well change it in both places.

That's probably the reason why you added this backward compat block in the first place.

Calling bp_core_remove_nav_item( 'foo' ), where 'foo' is a group slug, will pretty much completely break your site. This is true both in 2.5 and in 2.5. I guess it makes sense to carry over this 2.5 behavior, though I think it doesn't matter much. I don't recall why I added that block - I have a feeling it was either copypasta, or it was something that @imath added in an original patch - but I guess it's fine to keep it.

[boonebgorges]

In 10876:

Fix syntax error in back compat block of bp_core_remove_subnav_item().

The arguments were reversed, causing a compatibility break with plugins
using bp_core_remove_subnav_item() to remove group nav items.

We simplify the logic by setting $component to 'groups' in thes cases, and
letting the rest of the function handle the nav removal.

To maintain the parallel construction, a similar syntax change has been
introduced to bp_core_remove_nav_item().

Props dcavins, imath.
Fixes #6534.

[r-a-y]

In 10979:

Core: Fix 404 errors on single BuddyPress pages when HHVM is in use.

Debugging on a HHVM install revealed a bug with our create nav link
functions during the implementation of the new navigation API. We
shouldn't be returning the nav API methods in the nav link functions; we
should be returning the nav array item instead.

See #6534.

Fixes #7197 (2.6-branch).

[r-a-y]

In 10980:

Core: Fix 404 errors on single BuddyPress pages when HHVM is in use.

Debugging on a HHVM install revealed a bug with our create nav link
functions during the implementation of the new navigation API. We
shouldn't be returning the nav API methods in the nav link functions; we
should be returning the nav array item instead.

See #6534.

Fixes #7197 (trunk).

[r-a-y]

In 11814:

Navigation: Restricted subnav menu pages should redirect instead of throwing a 404.

Back in BuddyPress 2.6.0, we refactored the way we register menu items with
the BP Navigation API (see #6534).

The problem is we missed an instance during the refactor. This caused
restricted subnav pages (those set with the 'user_has_access' parameter
to false in bp_core_new_subnav_item()) to 404 instead of being redirected
away.

Commit fixes this problem.

See #7659.