bp_core_new_subnav_item() only barely works for non-user nav

[boonebgorges]

bp_core_new_subnav_item() is designed for use with the individual member nav (see the references to 'displayed_user' throughout). Yet we use it in BP_Group_Extension to add group navigation items. This works fine for normal cases. But when you get into situations where you're doing funny things with redirects and user_has_access, things break. (See #4785 for some background.)

For the moment, I'm going to use this ticket as a collection place for writing unit tests for this and related functions. Then I may start breaking them into smaller bits, so that we can make the API more predictable and consistent across components.

[boonebgorges]

In 8539:

Save a variable when parsing args in bp_core_new_subnav_item()

See #5720

[boonebgorges]

In 8540:

Some tests for bp_core_new_subnav_item().

See #5720

[boonebgorges]

The last half of bp_core_new_subnav_item() is basically impossible to write unit tests for. It mixes up a bunch of display and hooking logic, and performs some redirects. I'm proposing that a helper function be written, which will perform the hooking + generate arguments for a redirect if necessary, and will then return this value back to bp_core_new_subnav_item() to perform the redirect. This should make it possible to write unit tests, and then to start hacking at the function for broader support.

See 5720.break-up-bp_core_new_subnav_item.patch. Could I get another dev to take a look to make sure this technique makes sense?

[imath]

I'll have a look at it soon, but before i forget, i think this problem #5103 might be related.

[imath]

Replying to boonebgorges:

See 5720.break-up-bp_core_new_subnav_item.patch. Could I get another dev to take a look to make sure this technique makes sense?

Just checked and tested, it's ok for me ;)

[boonebgorges]

In 8544:

Break redirect/display hook logic out of bp_core_new_subnav_item()

By moving some of the critical logic into bp_core_maybe_hook_new_subnav_screen_function(),
it becomes possible to write unit tests for it.

See #5720

[boonebgorges]

In 8545:

Add unit tests for bp_core_maybe_hook_new_subnav_screen_function()

See #5720

[boonebgorges]

After looking at this for a while, I think the best way forward is to add a new argument to bp_core_new_subnav_item(): no_access_url, which, if present, will be used to perform the redirect. See 5720.patch, which implements this new argument, and reorganizes the logic that tries to determine the redirect URL for user pages so that it makes a bit more sense.

Back in #4785, my proposal will then be to pass the following argument when calling bp_core_new_subnav_item() in BP_Group_Extension:

    'no_access_url' => bp_get_group_permalink( groups_get_current_group() ),

(It's a bit hard to supply a separate patch for this, as it overlaps with some other changes, but the logic should be easy enough to see.)

[boonebgorges]

In 8556:

Introduce 'no_access_url' param to bp_core_new_subnav_item()

bp_core_new_subnav_item() allows access to be controlled using the
'show_for_displayed_user' parameter. When this param is false for a given user,
the function attempts to determine a proper redirect URL. But this URL
determining logic assumes that we are looking at a user profile, while BP uses
bp_core_new_subnav_item() for group navigation as well. Rather than attempting
to bake more URL determination logic into bp_core_new_subnav_item() itself, we
introduce a 'no_access_url' parameter that allows calling functions to specify
dynamically the URL that users should be directed to when they do not have
permission to access a requested subnav item.

Fixes #5720