WP pages as component slugs causes redirect problems

[boonebgorges]

In troubleshooting some issues on buddypress.org I came across the following issue.

In 1.3 trunk, you can designate existing WP pages as BP pages. If those WP pages are child pages, then the slug of the corresponding activity item is a relative link rather than a simple slug. For example, if your blog is at ​http://example.com, and you enter the existing page ​http://example.com/awesome/activity as the Activity home page during the BP setup process, $bp->activity->slug will be set as 'awesome/activity' instead of 'activity'.

Usually, this is necessary, especially when "normal" URLs are being concatenated with the slug, where by "normal" I mean URLs where the component slug comes right after the siteurl (eg ​http://example.com/awesome/members/boonebgorges). But in cases where the slug is used in the *middle* of a url, it breaks.

The example I found is in the Activity component (thus the component of this ticket). In the schema described above, activity permalinks (eg ​http://example.com/awesome/activity/p/12345) redirect, via bp_activity_action_permalink_router() in bp-activity.php, to
{{$redirect = bp_core_get_user_domain( $activity->user_id, $activity->user_nicename, $activity->user_login ) . $bp->activity->slug . '/' . $activity->id . '/';}}
But that comes out to:
​http://example.com/awesome/members/boonebgorges/awesome/activity/12345
instead of the correct
​http://example.com/awesome/members/boonebgorges/activity/12345

There might be other places where this happens, but this is the one I found.

Possible fixes:

• Hardcode the 'slug' when it appears mid-URL, eg

{{$redirect = bp_core_get_user_domain( $activity->user_id, $activity->user_nicename, $activity->user_login ) . 'activity/' . $activity->id . '/';}}. Downside: Doesn't translate

• Use $bp->activity->name instead. Downside: doesn't translate
• Refactor the activity permalink catcher to look for the true activity slug. In particular, bp_activity.php, bp_activity_screen_single_activity_permalink() checks for

{{ if ( !$bp->displayed_user->id

$bp->current_component != $bp->activity->name )

return false;}}

but maybe it should check for

{{ if ( !$bp->displayed_user->id

$bp->current_component != $bp->activity->slug )

return false;}}

Downside: Having the word 'awesome' (or whatever it is!) multiple times through the URL is a bit strange

• Create something like $bp->activity->short_slug = BP_ACTIVITY_SLUG, so that the slug can remain the same, but the short_slug could be used in situations like this.

(Sorry for the longwindedness, it's a complex problem!)

[boonebgorges]

Bumping this up in priority because it's one of the issues from bp-pages that absolutely must be resolved for 1.3. (It was brought up briefly by me and jjj at the most recent dev chat but I couldn't find the ticket at the time.)

[boonebgorges]

2600-1.patch is a proof of concept for the third option I mentioned above, done for the groups component. I have chosen to call the new value a 'mid_slug', because it appears in the middle of paths rather than toward the beginning as in the case of the top-level directories, but I am certainly welcome to better suggestions.

Most of it is straightforward. The only bit of trickiness is in the definition of $bp->groups->mid_slug. If BP_GROUPS_SLUG has no slashes in it (ie is a top-level page) then it gets copied over to mid_slug. Otherwise it defaults to 'groups'.

One (perhaps minor) concern is that some components do not have top-level directories associated with them (friends, messages). If this proposal is applied to those components, then things like $bp->friends->slug will, in effect, never be used. Alternatively, since those components don't have top-level pages associated with them, maybe it makes sense to handle them separately.

Another concern is that this change will almost certainly break some plugins. I don't see a way around that. If we wanted a potentially more consistent and less harmful solution, we could instead keep using $bp->[component]->slug for middle-of-path uses, and instead make the directory-level slug the new piece of data. Eg, $bp->groups->directory_slug would be set by looking at $bp_pages, and $bp->groups->slug would be set in the same way as I'm setting mid_slug in this patch. Actually, the more I think about it, the more I think that this solution will seem more consistent to plugin devs.

Thoughts?

[boonebgorges]

I had a chat with Paul in IRC and we agreed that the directory_slug route was better, but after doing another proof-of-concept I am not so sure. As you'll see in 2600-2.patch (again, a fix for the groups component only, just as a test), I had to swap out every check of $bp->current_component out with directory_slug. This is likely to break even more plugins than the other route.

I'm going to mull this over. Feedback welcome.

[boonebgorges]

I should note btw that 2600-2.patch does not take into account any of the current_component checks outside of bp-groups.php. I'm sure there are lots of them.

[boonebgorges]

I spent some time this morning to sketch out a mostly complete patch for the groups component, along the lines of what was discussed yesterday in the dev chat. A brief intro to 2600-3.patch:

• It takes the general direction of 2600-2.patch, which is to say that it creates a new root_slug that is used for top pages
• It adds a bp_core_is_root_component() function that can be used to test for current_component in a way that is flexible.

The patch is, as you can see, enormous (and not even complete with respect to groups - there are some finer points having to do with $bp->bp_nav that I have not solved yet). In order to make groups tabs work, I had to rejigger the kinds of arguments that go into bp_core_new_subnav_item in some cases - $root_slug instead of $slug for $parent_slug.

The bottom line is that the current system - where top-level nav (like /groups/group-name/) is one and the same with profile subnav (like /members/boonebgorges/groups/) - simply does not cooperate when you want the first kind of slug (root_slug) to be different from the regular slug. No matter what we do, we can't guarantee backward compatibility for plugins that add nav and subnav pages - they will break when root_slug and slug diverge.

I can go ahead and complete a full patch along the lines of 2600-3.patch, but I would like some feedback first.

[boonebgorges]

2600-4.patch is a patch that is more or less complete for the Groups and the Activity components. Begging for feedback :)

I played around with our idea of setting the $bp->current_component to always be the short slug. But that move wreaked all sorts of havoc with the way that the options_nav is built and displayed. Writing this patch has made me realize that those parts of BP sorely need rebuilding, but that's out of scope for this dev cycle.

I'm asking for two kinds of feedback:
1) Apply the patch to your local installation and let me know whether the thing actually works with activity and groups
2) Have a look at my methods and let me know what you think. As discussed above, this patch will break some plugins, but the introduction of a very generous bp_core_is_current_component() should make the process of updating plugins very easy for plugin authors (I did a grep search and replace in just a few minutes). I'm not very happy about the technique I use at the beginning of bp_get_options_nav() to decide which nav items to load, but again, given the way that BP currently works, this may be the best we can do.

Quick feedback would be much appreciated, as this kind of extensive patch will go stale very quickly, and I'll need to extend the technique to the remaining components for a full patch (though groups was the hard one).

[johnjamesjacoby]

(In [3728]) Code normalization and whitespace clean-up. Introduce bp-core-deprecated.php. Introduce root_slug globals into components with directories, to help with WP page slugs. Fixes #2600. Optimus Props boonebgorges