Skip to:
Content

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#5103 closed defect (bug) (fixed)

Group slug and user's subnav parent_slug trouble

Reported by: imath Owned by: boonebgorges
Milestone: 2.6 Priority: low
Severity: normal Version:
Component: Navigation Keywords: needs-patch needs-refresh
Cc:

Description

Hi,

I was testing next version of BuddyDrive and then i've created a group with the name of BuddyDrive. Once the group was created the slug of the group is 'buddydrive'. Then i've noticed that the BuddyDrive user's subnav was showing on the group nav just after the Home one.

I then thought, no problem i'll add buddydrive to the groups forbidden names. Then i ran some other tests by creating groups with these names :

  • Profile
  • friends
  • activity

Once these groups were created, i have the component user's subnav in the group nav just like my BuddyDrive test (you can see screencaps for the 3 groups).

The problem is that as the parent_slug of for instance the Edit profile subnav is profile and match the group slug, then if the loggedin user == the group creator (or if he's the superadmin), he has in the group nav his profile subnav etc..

I guess a quick fix would be to add components slugs to the groups forbidden name array. On the other hand, i'm not sure it's a great solution as in the case of plugin's component, if the name of the group match the plugin's component slug before it has been installed, filtering the groups forbidden names will have no effect as at least one group were created with the parent_slug..

What do you think ?

Attachments (4)

activity.png (39.0 KB) - added by imath 5 years ago.
group slug = activity
friends.png (35.0 KB) - added by imath 5 years ago.
group slug = friends
profile.png (53.0 KB) - added by imath 5 years ago.
group slug = profile
5103.patch (731 bytes) - added by imath 4 years ago.

Download all attachments as: .zip

Change History (15)

@imath
5 years ago

group slug = activity

@imath
5 years ago

group slug = friends

@imath
5 years ago

group slug = profile

#1 @boonebgorges
5 years ago

  • Priority changed from normal to low

I've noticed this problem before as well.

The root issue is the way the bp_options_nav array is indexed. User profile-related nav is keyed by component: 'profile', 'activity', etc. Group-related nav is keyed by the group slug (since BP 1.5). So, when there's a match between the group slug and a component, everything gets jumbled. It's a little bit more complicated than this, but basically, the following two things happen:

// in the activity component
bp_core_new_subnav_item( array(
    'slug' => 'favorites',
    'parent_slug' => 'activity', // $bp->activity->slug
) );

// in the group component
bp_core_new_subnav_item( array(
    'slug' => 'members',
    'parent_slug' => 'activity', // $bp->groups->current_group->slug
) );

Messing around with the forbidden names array seems hackish to me. It doesn't address the root issue, which is that bp_options_nav is built in an inconsistent way across object types. Also, it wouldn't be effective anyway, for reasons like what imath suggests.

If we didn't have to worry about backward compatibility, I'd say that we should rethink the way that bp_options_nav works. There are a couple possibilities.

a) Add a new parameter to bp_core_new_nav_item(), something like 'context', which could be 'group', 'user', etc. That'd be stored in the bp_options_nav array. Then, when the nav is built (in bp_get_options_nav(), I think), you'd check to make sure that you're only loading context-appropriate items before spitting them out. This would solve the problem on the surface, but the global data object would still be pretty messed up. That said, it wouldn't be too hard to manage backpat for this strategy.

b) Rethink the way that bp_nav and bp_options_nav are structured, and make it more consistent across contexts. We currently consider the current group to be the top-level nav item when creating the group nav (and, oddly, we call that nav item "Memberships", though it's not displayed anywhere), and all of the "main" nav items under it (Members, Admin, etc) are subnav items. In contrast, the "main" nav items (Profile, Activity, etc) under a user are *top-level* nav items (stored in bp_nav) and the second row of nav items (Edit Profile, Change Avatar, etc) are stored in bp_options_nav. I know why it works this way from a legacy point of view, and from the point of view of what counts as a "component" in BuddyPress, but it doesn't make a whole lot of sense when looked at with a fresh pair of eyes. There are various ways we might think about changing this arrangement, but all of them are likely to break backward compatibility in many ways.

Anyway, those are my initial thoughts. I'm anxious to hear what ideas other people have.

(I'm marking this ticket Low priority, because in the end, I think we might wait until we do a major overhaul of navigation building before attempting to fix it. Also, the case that imath raises is kind of an edge case, annoying as it may be, so it's not urgent to fix it. That said, if we come up with a good idea for fixing the issue, I'm glad to move it into 1.9.)

#2 @johnjamesjacoby
5 years ago

  • Milestone changed from Awaiting Review to 1.9

Related to #4986. Moving to 1.9 so we can give the navigation some real attention.

#3 @boonebgorges
5 years ago

  • Milestone changed from 1.9 to Future Release

#4 @imath
4 years ago

  • Keywords has-patch added; needs-patch removed

In 5103.patch, i suggest to check the context without adding a new argument but by checking the parent_url one as it should contain the groups component's directory root slug.

@imath
4 years ago

#5 @boonebgorges
4 years ago

imath, your patch makes my eyes burn :) I know that the problem you're trying to solve is an annoying one, but the fact remains that it's an edge case. And blocking the creation of nav items based on parent_url is likely to cause backward compatibility issues in cases where people are doing creative things building the nav items.

I had to make a long drive yesterday after reading this patch, so had a chance to think a lot about option (b) :) I think that it'd be possible to refactor some of this in the way I suggest *without* breaking backward compatibility too much, through the creative use of the __get() magic method and/or some by-reference markers where we're moving stuff around. Let me think about this a bit more and work up a proof of concept. I think it's doing this the right way if we're going to do it at all.

#6 @DJPaul
3 years ago

  • Keywords needs-patch needs-refresh added; 2nd-opinion has-patch removed
  • Severity changed from major to normal

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


2 years ago

#8 @boonebgorges
2 years ago

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

In 10745:

Introduce new API for BuddyPress navigation.

The new BP_Core_Nav overhauls the way that BuddyPress registers, stores, and
renders navigation items. Navigations are now component-specific, eliminating
the potential for confusion and conflict between navigation items with similar
names in different components, and opening the possibility of generating navs
for separate objects of the same type on a single pageload.

The new nav API replaces the old bp_nav and bp_options_nav system, which
dates from the earliest days of BuddyPress. These global properties were
responsible for handling nav and subnav across all of BP's components. The data
structure of bp_nav and bp_options_nav was simultaneously too opaque (in the
sense of being difficult to approach for developers and not having a complete
interface for programmatic modification) and too transparent (forcing devs to
manipulate the global arrays directly in order to customize navigation). The
new system eliminates most of these problems, by removing direct access to the
underlying navigation data, while providing a full-fledged API for accessing
and modifying that data.

An abstraction layer provides backward compatibility for most legacy uses of
bp_nav and bp_options_nav. Plugins that read data from the globals, modify
the data (eg $bp->bp_nav['foo']['name'] = 'Bar'), and unset nav items via
bp_nav and bp_options_nav should all continue to work as before. Anyone
accessing the globals in this way will see a _doing_it_wrong() notice. This
backward compatibility layer requires SPL (Standard PHP Library), which means
that it will not be enabled on certain configurations running PHP 5.2.x. (SPL
cannot be disabled in PHP 5.3+, and is on by default for earlier versions.)

The new system breaks backward compatibility in a number of small ways. Our
research suggests that these breaks will affect very few customizations, but
we list them here for posterity:

  • Some array functions, such as sort(), will no longer work to modify the nav globals.
  • Subnav items added to nonexistent parents can no longer be successfully registered. Previously, they could be loaded into the global, but were never displayed on the front end.
  • Manual management of group navigation items previously worked by passing the group slug as the parent_slug parameter to the bp_core_*_subnav_item() functions. The new API requires specifying a $component ('members', 'groups') when accessing nav items. To provide compatibility with legacy use - where $component was not required - we make some educated guesses about whether a nav item is "meant" to be attached to a group. This could result in unpredictable behavior in cases where a group slug clashes with a Members navigation item. This has always been broken - see #5103 - but may break in different ways after this changeset.

Props imath, boonebgorges, r-a-y.
Fixes #5103. Fixes #6534.

#9 @boonebgorges
2 years ago

  • Milestone changed from Future Release to 2.6

Fixed as part of work on #6534.

#10 @boonebgorges
2 years ago

  • Component changed from Component - Core to API - Nav

#11 @johnjamesjacoby
2 years ago

In 10873:

Navigation: Update phpdoc paramters for delete_nav().

Also make $slug parameter optional to piggy-back the empty( $slug ) check.

See #5103.

Note: See TracTickets for help on using tickets.