Group extension access settings disrespected by bp_core_new_subnav_item()

[dcavins]

In our new group extension access settings (see r8605), we've allowed users to independently set who can see the extension's navigation tab and who can visit the extension's display pane. However, because of a limitation in bp_core_new_subnav_item(), if who can visit the tab is more permissive than who can see the tab (for example, you create an extension that doesn't need a navigation tab, but does need to display the contents to anyone, like 'show_tab' => 'noone', 'access' => 'anyone') bp_core_subnav_item() is never called. This prevents the navigation link from being created, but it turns out that bp_core_subnav_item() must be called in order for the screen function to be caught. (See bp_core_maybe_hook_new_subnav_screen_function() in bp-core-buddybar.php.) And there's no way currently to call bp_core_subnav_item() without actually creating the navigation link.

I've attached a patch (6301.01) that adds another argument, user_can_see_nav_item, to bp_core_new_subnav_item() that controls whether the link is created in bp_get_options_nav(). user_has_access is still used to determine whether the screen function should be hooked or not.

In patch 6301.02, I used the new argument to call bp_core_new_subnav_item() if the user can see the nav tab or access the extension's display pane.

I'm unfamiliar with the navigation creation functions, so would appreciate feedback.

[dcavins]

Add 'user_can_see_nav_item' argument to bp_core_new_subnav_item()

[dcavins]

Use the 'user_can_see_nav_item' argument to call bp_core_new_subnav_item() if the user can see the nav tab or access the extension's display pane.

[boonebgorges]

The idea of having an argument in bp_core_new_subnav_item() that would essentially cause a subnav item not to be created seems pretty unappealing. The root problem here is that we hook the screen function and create the nav item inside of the same function. A better solution, IMO, would be to break these two tasks apart.

What do you think of something like this: have separate functions for registering a subnav item and for attaching a screen function, then swap out all BP core usage for these new functions. The old bp_core_new_subnav_item() (and bp_core_new_nav_item()) would stay as-is for backward compatibility.

[dcavins]

Yes.

I would also like it (a lot) if somewhere down the road we simplified/unified the process of creating navigation in groups and member profiles. Adding the ability to even reorganize navigation links (being able to group several typically top-level navigation items under a new navigation items is an obvious example--moving group invites into the members tab, for instance) via filters would be pretty sweet. (I'm thinking a bit of how when declaring a custom post type or taxonomy, you can specify that it be added as a subnav item of an existing menu item.)

Separating the navigation link creation from the screen hook seems like a good start. I'm surprised at how few times bp_core_new_subnav_item() is called in the code; it's used once each in buddybar, component, groups-classes and xprofile-loader.

Thanks for the feedback!

[DJPaul]

@dcavins do you want to create a new ticket for that suggestion, so we can close this ticket? Or would we still want this change but implemented differently after the aforementioned split?

[dcavins]

Closing in favor of #6503.