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

 I know that @imath is concerned about backward compatibility holding up
 this ticket, so I decided to dive in and do a BC review.
 [attachment:6534.05.patch] contains a number of items that improve (and
 demonstrate) backward compatibility. (I have not done a full patch review
 yet - I will do so in the upcoming days, but I wanted to get this part of
 it up for review first.) A summary:

 I wrote a bunch of unit tests that describe various ways one can interact
 with the `bp_nav` and `bp_options_nav` global, and got them to pass
 against the current BP trunk. See the `globalAccess.php` file in
 [attachment:6534.05.patch] (they probably won't run on trunk because of
 some mods I made during development, but with a bit of massaging they
 could be made to do so). I also tested using the following function on the
 front end:
 https://gist.github.com/boonebgorges/6025ad2e0f7ab8b42b26c965773d8805

 I think I managed to synthesize the approaches made by @imath and @r-a-y.
 @imath's patch wasn't extensive enough to provide BC for the most common
 mods to the global, while @r-a-y's patch broke in a number of ways for me,
 and I think was overly complex because it was trying to be truly
 recursive. Figuring this out took me most of a day, and would've taken
 much longer without looking at your clever patches, so thanks :)

 The gist linked above, and the unit tests, show the extent of the BC that
 is covered by the patch. Here are some uses that are *not* covered:

 {{{
 // Unsetting a single property of a nav item. The new nav API doesn't
 support malformed items very well.
 unset( $bp->bp_nav['foo']['name'] );

 // Sorting. This will probably give you a fatal error.
 sort( $bp->bp_nav );

 // Looping using foreach.
 foreach ( $bp->bp_nav as $key => $value ) { ...
 }}}

 There may be others. It's likely that people are doing these things, but
 I'd wager that it's very rare compared to the basic set/get/isset/unset
 actions that are covered by [attachment:6534.05.patch]. I guess it's worth
 looking through the plugin directory to see if unsupported mods are in
 widespread use; it's possible to support them with some fancy use of
 `ArrayObject`, but I don't want to go to the trouble for extreme edge
 cases.

 A couple other notes on my BC implementation:

 1. I renamed the BC classes and put them into separate files

 2. I added a check that ensures that they're only loaded when
 `ArrayAccess` is available. SPL can be disabled on PHP 5.2.x (though it is
 enabled by default), so we want to be sure not to cause fatals. On these
 installations, there will simply be no backward compatibility support for
 bp_nav and bp_options_nav - a fair compromise, I think.

 3. Another BC syntax break I noticed was that it's no longer possible to
 do this:

 {{{
 $nav_item = $bp->bp_options_nav['foo']['bar'];
 $css_id = $nav_item['css_id'];
 }}}

 because the `$nav_item` is a `stdClass`. Even if we're no longer going to
 use this syntax in BP, I think it's wise to support its legacy use in
 plugins/themes. So I introduced `BP_Core_Nav_Item`, which extends
 `ArrayObject` when available, and used that class in `BP_Core_Nav` instead
 of `$nav_item = (object) $args`. This seems like a small and easy gesture
 we can make for BC. Having a separate class for nav items also opens the
 door for other OOP techniques in the future.

 4. Seems like if we're going to throw `_doing_it_wrong()` notices, we
 should do it whenever the global is touched, not just when an offset is
 provided. So I made that change. If I'm missing something, go ahead and
 change it back.

 5. Another BC break that we should probably document or mitigate is that
 this will no longer work:

 {{{
 bp_core_new_subnav_item( array(
     'name' => 'Foo',
     ...
     'parent_slug' => bp_get_current_group_slug(),
     ...
 ) );
 }}}

 Currently, this will add a subnav to the current group. But the new
 `bp_core_add_subnav_item()` will only know to add to a group nav (rather
 than members) if 'groups' is passed to the `$component` parameter. In
 `BP_Core_BP_Nav_BackCompat::get_component()`, I do this: if
 `bp_is_group()`, and the nav item slug matches the slug of the current
 group, I assume that you intend to be modifying the group nav. (This won't
 be true in absolutely every case, but 99.999% of the time.) I see that
 something similar is happening in the deprecated `_remove_` functions. I'd
 suggest adding it to `bp_core_new_subnav_item()` too.

 Phew - I think that's it for the BC stuff. Once we address the above
 points, I think we'll be in a good place from the compatibility point of
 view.

 I will come back soon with a more complete review, but first a small note,
 before I forget: The function names `bp_core_add_nav_item()` and
 `bp_core_add_subnav_item()` were once in use in BP, and removed in [2168].
 They were replaced by the current functions for the same reason we're
 using the names to replace what we've currently got - because we need easy
 synonyms for changed syntax. Probably not worth worrying about, but I
 thought I'd throw it out there :)

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