Separate functions for creating a new nav link and registering a screen function.

[dcavins]

The functions bp_core_new_subnav_item() and bp_core_new_nav_item() each do two things: add a link to the navigation menu and register the screen function that should be fired when that link is used. Sometimes, it would be handy to do one without the other. For instance, in a group, you have a pane that isn't important enough to warrant its own navigation itemin the group nav (the navigation to that page is handled in some other way), but you do need to register the screen function. BP_Group_Extension allows such an arrangement, but the underlying functions are all-or-nothing.

The attached patch separates the action into two pieces: add the link to the nav, register the screen function.

See #6301 for more discussion.

Thanks for your feedback.

[dcavins]

Minimum change to separate the functions.

[boonebgorges]

Thanks for getting this conversation started, dcavins.

It'd be helpful (both for review and for committing) to get the whitespace/extract() changes into a separate patch.

Instead of swapping out current core uses of bp_core_new_subnav_item(), why don't we refactor bp_core_new_subnav_item() so that it wraps the two new functions?

[imath]

Just fyi, for #6026 i also had to play in this area. i'm currently working on a new patch where I've decided to leave these functions the way they are because they are too tight to user and i didn't want to add some more risks of slug collisions like there is today. Instead, I'm using a new Class to build the navigation for any component's single items. So far it's working nice on my tests and i'm almost pretty sure the groups single items could benefit from it.
I have some details to finish, but i'll soon update the ticket about the blogs single items.

PS: I agree with boonebgorges about the extract part changes :)

[DJPaul]

Big +1 from me.

(Let's get the smaller unrelated changes split into its own patch.)

[dcavins]

Here are two new patches.

In 6503-remove-extract.patch I've removed the extract()s from bp_core_new_nav_item() and bp_core_new_nav_default() and done a bracket and whitespace cleanup on those functions plus bp_core_sort_nav_items().

In 6503-refactor-bp_core_new_subnav_item.patch I've added the new functions bp_core_create_subnav_link() and bp_core_create_subnav_link() and changed bp_core_new_subnav_item() to use the new functions. The group extension use of bp_core_new_subnav_item() has been switched to use the new functions directly, because it can take advantage of the new granularity.

I had previously touched the use of bp_core_new_subnav_item() in BP_Component and BP_XProfile_Component, but, since those uses don't currently need the granularity, I've left them as is.

Thanks for your feedback!

[dcavins]

Remove extract() from bp_core_new_nav_item.

Also formatting clean up in bp_core_new_nav_item(), bp_core_new_nav_default() and bp_core_sort_nav_items() : bracket if statements, normalize white space.

[dcavins]

Refactor bp_core_create_subnav_link().
Use new functions bp_core_create_subnav_link() and bp_core_register_subnav_screen_function() within bp_core_create_subnav_link(). Switch groups extension usage to take advantage of new granularity.

[boonebgorges]

dcavins - Awesome. If you're happy with 6503-remove-extract.patch, and you're satisfied with test coverage for it, commit at will.

The approach in the refactoring patch looks good to me. It strikes me that if we're going to make this change to subnav registration, then we should probably do it for top-level nav registration as well. What do you think?

[dcavins]

In 9996:

Remove extract() from bp_core_new_nav_item.

Remove extract() and other formatting clean up
in bp_core_new_nav_item(),
bp_core_new_nav_default() and
bp_core_sort_nav_items(). Bracket if statements,
normalize white space.

See #6503.

[dcavins]

In 9997:

Refactor bp_core_new_subnav_item().

Use new functions bp_core_create_subnav_link()
and bp_core_register_subnav_screen_function()
within bp_core_new_subnav_item().
Switch group extension usage to take advantage of new granularity.

See #6503.

[dcavins]

In 10001:

Finish removing extract in bp_core_new_subnav().

Fixes if condition for a subnavigation item that
is currently being requested.

See #6503.

[dcavins]

Separate the link creation from registering the screen function in bp_core_new_nav_item().

[dcavins]

I've added a patch for separating the addition of the nav link from registering the screen function in bp_core_new_nav_item(). In working through the changes, I had a few questions:

1. This check occurs in the original file _after_ the link is added to the $bp->bp_nav global

   if ( empty( $r['show_for_displayed_user'] ) && ! bp_user_has_access() ) {
   	return false;
   }
   

Can anyone think of a reason for this order of operations? I moved it before things happen, so if the old order of operations is important, please let me know.

2. There's a do_action( 'bp_core_new_nav_item' ) at the end of bp_core_new_nav_item(). I had to choose where that should go, and left it after the screen function is registered. Is that appropriate?

3. bp_core_new_nav_item () is only called in BP_Component::setup_nav() which doesn't currently take advantage of the split, so I've left that call as is for now.

Thanks for your feedback! I think there could be some more unit tests, so if anyone has ideas for cases to test, please let me know that as well, and I'll write some tests.

[boonebgorges]

Can anyone think of a reason for this order of operations? I moved it before things happen, so if the old order of operations is important, please let me know.

See #1263 [2048]. I'm assuming it just happened to be added there - I don't see evidence that it was on purpose. I can't think of any reason why your change would break anything, so let's go with it. (I only see the green in 6503.split-new-nav-item.patch. Make sure you remove the existing check too.)

There's a do_action( 'bp_core_new_nav_item' ) at the end of bp_core_new_nav_item(). I had to choose where that should go, and left it after the screen function is registered. Is that appropriate?

I'm not seeing this in the patch, but I'd leave it where it is. Leave it in bp_core_new_nav_item(), after the screen function is hooked, to ensure compatibility with the current behavior.

bp_core_new_nav_item () is only called in BP_Component::setup_nav() which doesn't currently take advantage of the split, so I've left that call as is for now.

+1. This seems fine to me. We're not deprecating the existing function, just converting it to a wrapper.

[dcavins]

Thanks Boone. I'll give this patch another look over (and move the do_action( 'bp_core_new_nav_item' ) back into the parent function ) and commit it today. In the future, when the patch isn't very helpful to seeing the changes, I'll attach an excerpt of the actual file so anyone can see the result.

[dcavins]

Excerpt of bp_core_buddybar

[dcavins]

Second approach of splitting up new nav item creation.

[dcavins]

I've added a new version that moves do_action( 'bp_core_new_nav_item' ) back to the end of the wrapper function. In doing so, I also needed to move the wp_parse_args() call back out to the wrapper function. In order to avoid parsing the args again, I removed the wp_parse_args() calls in the new helper functions, but this means that they can't be used directly.

I'd like some advice on the best way to handle this. Do we ultimately want to use the new functions directly? (If you did, then the bp_core_new_nav_item action wouldn't fire either.) Maybe not? I'm not sure.

[boonebgorges]

In doing so, I also needed to move the wp_parse_args() call back out to the wrapper function. In order to avoid parsing the args again, I removed the wp_parse_args() calls in the new helper functions, but this means that they can't be used directly.

Ah. It's a bit annoying, but I'd say that we should parse the args twice. In bp_core_new_nav_item(), be sure to stick with the unfilterable wp_parse_args(). If we decide to introduce bp_parse_args() to this stack, it'll be in the more specific functions.

Do we ultimately want to use the new functions directly?

I'd suggest introducing new hooks at the end of bp_core_create_nav_link() and bp_core_register_nav_screen_function(), and then making a note in the docs for 'bp_core_new_nav_item' that the more specific hooks should be used, because the wrapper may not always be used. In BP, we should feel free to call the bp_core_new_nav_item() wrapper function, but we should avoid using the 'bp_core_new_nav_item' hook, unless we want to fire a callback specifically when screen functions + nav items have been registered as a package deal.

[dcavins]

Refactor bp_core_new_nav_item().
Use new functions bp_core_create_nav_link() and bp_core_register_nav_screen_function() within bp_core_new_nav_item(). Also introduces two new action hooks: bp_core_create_nav_link and bp_core_register_nav_screen_function.

[dcavins]

Thanks very much for your feedback. I think the newest version accomplishes these goals. I'll plan on committing it after I take another look at it later today.

[dcavins]

In 10003:

Refactor bp_core_new_nav_item().

Use new functions bp_core_create_nav_link() and
bp_core_register_nav_screen_function() within
bp_core_new_nav_item(). Also introduces two new
action hooks: bp_core_create_nav_link and
bp_core_register_nav_screen_function.

See #6503.

[dcavins]

Fix error introduced in r10003 that prevents access-protected nav items from being added to the bp_nav array. The access protection should happen at access time in bp_core_register_nav_screen_function().

[dcavins]

I introduced an access-protection logic error in r10003. The symptom of this error is that if you’re not logged in and try to visit a user profile protected screen ​_without specifying a subnav slug_​, you get dumped to a 404 rather than a login and redirect screen. For instance: ​http://bpdev.local/members/cassie/messages/ yields a 404, but ​http://bpdev.local/members/cassie/messages/inbox follows the correct behavior and drops you to the login screen. The underlying issue is that a show_for_displayed_user check was preceding (and preventing) the addition of the nav item to the bp_nav array. (That nav element needs to exist even if the user can’t visit it.)

Fix is included in 6503.fix-bp-nav.patch, above.

[boonebgorges]

Is it possible to write a test that demonstrates the regression?

[dcavins]

Yes, thanks for asking. This test feels like a bit of a hack, but bp_nav is a slippery little feller to test. I've attached a new patch that

• adds a test that shows an example failure (I'd love advice on how to make this test better.)
• ensures that bp_core_new_nav_item() and bp_core_new_subnav_item() return false in certain cases where the screen function registration fails. (Removing the early access check demonstrated this problem.)

[dcavins]

Fix error introduced in r10003 that prevents access-protected nav items from being added to the bp_nav array. The access protection should happen at access time in bp_core_register_nav_screen_function().

[boonebgorges]

Thanks, @dcavins. I've spent a bit of time with your patch, and I think it's the right way to go. The return false stuff is really goofy, but I think that's a price we pay for full backward compatibility.

The test technique seems OK to me. Ideally, our nav/page registration system would have a simple has_access() method that we could use for unit testing. See #6534.

A note that the change will result in a very small change in behavior. In 2.3, registering a navigation item with show_for_displayed_user=false would fail completely when viewing another user's profile. After this change, the nav item will be registered into the global, but it'll never be rendered. See bp_get_displayed_user_nav(). So if someone is doing some weird stuff the globals, they'll see something there that they didn't expect to see. I don't really care about these scenarios - and it's something I'd like to address in #6534 anyway - but I wanted to put it out there.

Let's go ahead with this for 2.4.

[dcavins]

Thanks for your feedback @boonebgorges.

In 2.3, registering a navigation item with show_for_displayed_user=false would fail completely when viewing another user's profile. After this change, the nav item will be registered into the global, but it'll never be rendered.

I think the nav item needs to be registered in the global (and I think it is in 2.3)--because it's used if a non-logged in user tries to access a protected item. For instance, clicking on a link to a private message in an email will drop a 404 if the item isn't in the global. If it is, you get the login redirect behavior. This is my understanding of the situation, at least.

OK, I'll merge this in tonight. Thanks again. And yes to the goofiness of all the return false stuff.

[dcavins]

In 10326:

Allow access-protected bp_nav items to exist.

Fix error introduced in r10003 that prevents
access-protected nav items from being added to the
bp_nav array. The access protection should
happen at access time in
bp_core_register_nav_screen_function().

See #6503.