Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5720 closed defect (bug) (fixed)

bp_core_new_subnav_item() only barely works for non-user nav

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

Description

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.

Attachments (2)

5720.break-up-bp_core_new_subnav_item.patch (5.2 KB) - added by boonebgorges 5 years ago.
5720.patch (5.6 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (11)

#1 @boonebgorges
5 years ago

In 8539:

Save a variable when parsing args in bp_core_new_subnav_item()

See #5720

#2 @boonebgorges
5 years ago

In 8540:

Some tests for bp_core_new_subnav_item().

See #5720

#3 follow-up: @boonebgorges
5 years ago

  • Keywords dev-feedback added

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?

#4 @imath
5 years ago

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

#5 in reply to: ↑ 3 @imath
5 years ago

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 ;)

#6 @boonebgorges
5 years ago

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

#7 @boonebgorges
5 years ago

In 8545:

Add unit tests for bp_core_maybe_hook_new_subnav_screen_function()

See #5720

#8 @boonebgorges
5 years ago

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
5 years ago

#9 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.