[buddypress-trac] [BuddyPress Trac] #6534: A new API to manage single items navigation

[buddypress-trac]

#6534: A new API to manage single items navigation
------------------------------------+------------------
 Reporter:  imath                   |       Owner:
     Type:  enhancement             |      Status:  new
 Priority:  high                    |   Milestone:  2.6
Component:  API                     |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  dev-feedback has-patch  |
------------------------------------+------------------

Comment (by boonebgorges):

 OK, another round of review. Advance apologies for what is going to be a
 long comment :)

 [attachment:6534.2.diff] is my current proposed patch. I'll explain its
 partner [attachment:6534.diff] below.

 > Do we really need to reset $bp->options_nav as it was already set in the
 members component ?

 No. In fact, I think we should do it once, in `BP_Core`. My new patch does
 this.

 > About the deprecated functions, what is the reason that makes these
 functions different than other deprecated functions since r-a-y and you
 are adding a trace to the deprecated notice ?

 I have mixed feelings about the utility of full stack traces (anyone who
 needs one is probably already using something like xdebug). But in any
 case, I agree that it's probably not appropriate to sneak it into this
 ticket. @r-a-y, if you think it would be a good improvement throughout BP
 - or perhaps just for those deprecated notices that show up most
 frequently - let's talk about it in a separate ticket.

 In [attachment:6534.2.diff], I undeprecate `bp_core_new_nav_item()` and
 `bp_core_new_subnav_item()`. @imath, I'm guessing that you deprecated them
 because, on the new system, adding a nav item (specifically, a subnav
 item) requires the specification of a `$component`. But I think that the
 deprecation notices are overkill. Falling back on 'members' works fine in
 most cases. The only problematic case is where someone is using
 `bp_core_new_subnav_item()` with 'parent_slug' set to a group slug to add
 a subnav item. This is my point 5 in [comment:25 comment 25]. As suggested
 there, I think we can and should provide BC for this use case, and I think
 I have a pretty good heuristic for doing so (see the patch, and my comment
 below, for more details). So it's extremely likely that in nearly every
 single case, calling the existing functions will continue to work as
 expected. For this reason, I don't think there's a good reason for us to
 fill everyone's error logs with deprecation notices. (I had to turn hack
 around WP_DEBUG just to test the original patches, given all the plugins
 that use these functions!) For the same reason, I think we can probably
 undeprecate the `_remove_` functions, though IMO it's less critical. (The
 `_doing_it_wrong()` notices for touching `bp_nav` and `bp_options_nav`
 should stay.)

 I feel pretty strongly that we should *not* deprecate these functions, but
 if everyone disagrees with me, [attachment:6534.diff] is a version of
 [attachment:6534.2.diff] that retains the deprecation.

 On the subject of 'groups' and `bp_core_new_subnav_item()` backward
 compatibility. A plugin of mine (and @r-a-y's) uses the same trick to add
 group subnav items (sub-subnavs!) as BP core uses for the Manage subnav: a
 parent nav item of `{$group_slug}_manage` (or, in our case,
 `{$group_slug}_events`). See eg https://github.com/cuny-academic-commons
 /bp-event-organiser/blob/master/bp-event-organiser-groups.php#L140. Let's
 set aside for a moment the fact that this is a terrible technique, and
 something we should fix down the road by making group navigation top-
 level. We can do this in the future. For now - if @r-a-y and I are doing
 this, it's likely someone else is. So I added some horrendous logic that
 says: this is probably a group navigation item if (a) the parent slug ===
 the current group slug, OR (b) the current group slug appears at the very
 beginning of the `parent_slug`, and there is no Members nav item with the
 `parent_slug` (to avoid the very slim possibility of false positives).
 It's pretty ugly, but it works.

 I did some juggling to the way that you reorganized the `_create_link()`
 and `_hook_screen_function()` stuff, so that return values and
 `do_actions()` made a bit more sense.

 I crawled my way through the unit test suite to ensure that everything
 passes. This means deleting the `bp_core_sort_nav_items()` tests - this
 function no longer does anything. Working through the tests showed a
 couple more minor BC breaks that should be noted. The most significant is
 that it's no longer possible to add subnav items with a `parent_slug` that
 points to a non-existent nav item. I'm guessing that no one is actually
 doing this in production, since it results in nav items that don't render
 on the front end, but we were doing it in a couple places in the tests in
 order to "fake" the navigation.

 I did some documentation tidying throughout.

 An earlier patch had some modification to the way
 `bp_nav_menu_get_loggedin_pages()` works, in order to account for the fact
 that nav items are now classes rather than arrays. This was pretty
 confusing to me, since `_loggedout_` didn't require the same treatment.
 Since I implemented ArrayObject for nav items anyway, I standardized the
 treatment.

 ==

 I feel pretty good about [attachment:6534.2.diff]. @imath and others, I'm
 especially interested in your thoughts about deprecated functions. My
 inclination is to undeprecate the `_remove_` functions as well. Once we
 make a decision about this, I think we are close to having something ready
 to be committed.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6534#comment:27>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac