Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#6503 closed enhancement (fixed)

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

Reported by: dcavins Owned by:
Milestone: 2.4 Priority: normal
Severity: blocker Version: 1.2
Component: Core Keywords: has-patch commit
Cc: espellcaste@…

Description

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.

Attachments (10)

6503.01.patch (18.0 KB) - added by dcavins 5 years ago.
Minimum change to separate the functions.
6503-remove-extract.patch (10.6 KB) - added by dcavins 5 years ago.
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.
6503-refactor-bp_core_new_subnav_item.patch (8.5 KB) - added by dcavins 5 years ago.
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.
6503.split-new-nav-item.patch (6.4 KB) - added by dcavins 5 years ago.
Separate the link creation from registering the screen function in bp_core_new_nav_item().
bp_core_new_nav_item_excerpt.php (9.8 KB) - added by dcavins 5 years ago.
Excerpt of bp_core_buddybar
bp_core_new_nav_item_excerpt.02.php (9.1 KB) - added by dcavins 5 years ago.
Second approach of splitting up new nav item creation.
6503-split_new_nav_item.02.patch (8.8 KB) - added by dcavins 5 years ago.
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.
bp_core_new_nav_item_excerpt.03.php (11.5 KB) - added by dcavins 5 years ago.
6503.fix-bp-nav.patch (634 bytes) - added by dcavins 5 years ago.
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().
6503.fix-bp-nav-w-test.patch (3.9 KB) - added by dcavins 5 years ago.
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().

Download all attachments as: .zip

Change History (40)

@dcavins
5 years ago

Minimum change to separate the functions.

#1 @espellcaste
5 years ago

  • Cc espellcaste@… added

#2 @boonebgorges
5 years ago

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?

#3 @imath
5 years ago

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

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


5 years ago

#5 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Big +1 from me.

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

#6 @dcavins
5 years ago

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

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

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.

#7 @boonebgorges
5 years ago

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?

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#9 @dcavins
5 years ago

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.

#10 @dcavins
5 years ago

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.

#11 @dcavins
5 years ago

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

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

#12 @dcavins
5 years ago

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.

  1. 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?
  1. 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.

#13 @boonebgorges
5 years ago

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.

#14 @dcavins
5 years ago

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

Excerpt of bp_core_buddybar

@dcavins
5 years ago

Second approach of splitting up new nav item creation.

#15 @dcavins
5 years ago

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.

#16 @boonebgorges
5 years ago

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

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.

#17 @dcavins
5 years ago

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.

#18 @dcavins
5 years ago

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.

#19 @dcavins
5 years ago

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

#20 @r-a-y
5 years ago

  • Milestone changed from Future Release to 2.4
  • Version changed from 2.3.1 to 1.2

@dcavins
5 years ago

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().

#21 @dcavins
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#22 @DJPaul
5 years ago

  • Severity changed from normal to blocker

#23 @boonebgorges
5 years ago

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

#24 @dcavins
5 years ago

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

#25 @DJPaul
5 years ago

  • Keywords needs-testing added

@dcavins
5 years ago

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().

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


5 years ago

#27 @boonebgorges
5 years ago

  • Keywords commit added; needs-testing removed

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.

#28 @dcavins
5 years ago

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.

Last edited 5 years ago by dcavins (previous) (diff)

#29 @dcavins
5 years ago

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.

#30 @dcavins
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.