[buddypress-trac] [BuddyPress] #3554: Inverted component root page craziness

[buddypress-trac at lists.automattic.com]

#3554: Inverted component root page craziness
-------------------------------------+------------------
 Reporter:  johnjamesjacoby          |       Owner:
     Type:  defect                   |      Status:  new
 Priority:  normal                   |   Milestone:  1.5
Component:  Core                     |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  needs-patch 2nd-opinion  |
-------------------------------------+------------------

Comment (by boonebgorges):

 I've done some investigation, and my theory about the root cause of the
 problem is essentially correct.

 However, it's not as simple as just swapping the order. The function
 bp_is_current_component() is written with multiple fallbacks in mind, so
 that you can feed different values to the function. Let's say that you
 have the following setup:
 $bp->members->root_slug = 'users';
 $bp->members->slug = 'dudes';

 The function is currently written so that the following three checks will
 all return true:
 bp_is_current_component( 'members' );
 bp_is_current_component( 'users' );
 bp_is_current_component( 'dudes' );

 We did it this way for backpat reasons. Previously, the standard
 throughout BP was (for the most part - we were inconsistent) to check
 $bp->current_component == $bp->members->slug. Some plugins followed suit.
 But sometimes we (and plugins) checked for $bp->current_component ==
 $bp->members->id or simply $bp->current_component == 'members'. The
 introduction of root_slugs in 1.5 makes this even more complicated. So
 when we introduced bp_is_current_component() for 1.5, we wanted to provide
 maximum flexibility to plugin/theme authors.

 This is fine and dandy, until we start having crazy setups like the one
 you suggest. When
 $bp->members->root_slug = 'groups';
 the current component checks become, essentially, a race to see which
 screen function gets fired first. That's because bp_is_members_component()
 and bp_is_groups_component() will *both* return true;
 bp_is_current_component( 'members' ) is true because of the check against
 canonical IDs, while bp_is_current_component( 'groups' ) is true because
 of the check against root_slugs/$bp->pages.

 I think that there are three possible strategies for fixing the problem:
 1) Ignore it as an extreme edge case.
 2) Remove some of the fallback logic, thereby enforcing best practices for
 bp_is_current_component(). Presumably, the recommended practice will be to
 use $bp->{$component}->id, or the hardcoded canonical component name, when
 checking your current component (this is what we do in the
 bp_is_x_component() functions).
 3) Add a special case to bp_is_current_component() that checks for these
 kinds of cases. In other words, we still allow different kinds of inputs
 for bp_is_current_component(), but when we detect a clash like the one
 you've outlined, we fall back on strict $bp->pages/root_slug matches.

 IMO: (3) kinda stinks. Seems ad hoc and inelegant, plus it will result in
 somewhat unpredictable behavior for bp_is_current_component(), as your
 plugin won't be able to accurately predict what the user's setup will be.
 Between (1) and (2), I lean toward (2); bp_is_current_component() is new
 anyway, and we can recommend to plugin authors that they use the wrapper
 functions bp_is_groups_component() etc when available, and otherwise to
 use canonical names with custom components, eg bp_is_current_component(
 'achievements' ) and not bp_is_current_component( $bp->achievements->slug
 ). 3554.01.patch is a proposal for the revised logic (along with a
 corresponding logic change in bp_get_name_from_root_slug()).

 My only misgiving is that it's very late in the dev cycle to be making
 such substantial changes to a function like this, when there's little time
 for testing, and little time for reaction by plugin developers. So I would
 be OK with punting the issue for the moment if others think it would be
 appropriate.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/3554#comment:2>
BuddyPress <http://buddypress.org/>
BuddyPress