#5103 closed defect (bug) (fixed)
Group slug and user's subnav parent_slug trouble
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (15)
#1
@
12 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
@
12 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.
#4
@
11 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.
#5
@
11 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
@
10 years ago
- Keywords needs-patch needs-refresh added; 2nd-opinion has-patch removed
- Severity changed from major to normal
group slug = activity