[buddypress-trac] [BuddyPress Trac] #4954: Migrate BP's custom URI parser to use WP's Rewrite API

[buddypress-trac]

#4954: Migrate BP's custom URI parser to use WP's Rewrite API
---------------------------+------------------------------
 Reporter:  boonebgorges   |       Owner:  johnjamesjacoby
     Type:  task           |      Status:  new
 Priority:  high           |   Milestone:  2.0
Component:  Rewrite Rules  |     Version:
 Severity:  major          |  Resolution:
 Keywords:                 |
---------------------------+------------------------------

Comment (by r-a-y):

 Replying to [comment:18 johnjamesjacoby]:

 > That way (if I'm thinking about this correctly) any old third-party
 components without rewrite rules will still continue to function the same
 way they always have, with the URI router still catching them
 appropriately.

 If you're talking about any third-party root components, I'm not sure any
 exist, but you'd be right that they might not work.

 Plugins that hook onto existing member and group components are the
 plugins we have to worry the most about and the ones I have tested so far
 work (bbPress, BP Follow and BP Docs).  More testing is needed of course.

 > Thoughts on renaming bp_path to bp_action to retain the existing
 component/action/variables naming and relationships we're so used to now?

 Our current_X items are inconsistently named across different pages.

 For example, if you're on a single members component page -
 `/members/admin/activity/groups/`:

 * The directory page is 'members' also is used for
 bp_is_current_component() checks
 * $bp->current_item will return 'admin'
 * $bp->current_component will return 'activity'
 * $bp->current_action will return 'groups'
 * $bp->action_variables will return anything after 'groups' if set

 In my patch, `bp_path` returns 'admin/activity/groups' and that is later
 broken down into the respective current_X items in the
 BP_Members_Component::parse_query() method.

 If you're on a single groups component page - `/groups/test-
 group/members/`:

 * The directory page and $bp->current_component is 'groups'
 * $bp->current_item will return 'test-group'
 * $bp->current_action will return 'members'
 * $bp->action_variables will return anything after 'members' if set

 In my patch, `bp_path` returns 'test-group/members' and that is later
 broken down into the respective current_X items in the
 BP_Groups_Component::parse_query() method.

 If you're on a directory action page - `/groups/create/`:

 * The directory page and $bp->current_component is 'groups'
 * $bp->current_action will return 'create'
 * $bp->action_variables will return anything after 'create' if set

 In my patch, `bp_path` returns 'create'.  This is set in the main
 BP_Rewrite_::parse_query() method by default.

 So my thoughts on literally renaming `bp_path` to `bp_action` in my patch
 doesn't really make sense.  If we were following another approach, then,
 I'd be for it.

 Because our old current_X system is pretty inconsistent, I ended up doing
 the current_X shuffling just like our old system for backpat.  It's
 definitely not ideal, but I'd be interested in a better way to handle
 this.

 > Or, rather than having BuddyPress act as a central router again (with
 standard component/path/action vars) if maybe they should always be
 unique.

 My patch allows the component to set their own query variables.

 For example, your `bp_member_id` suggestion is already set as my
 `bp_user_id` in my patch during the BP_Members_Component::$query_vars
 array and parsed in the parse_query() method.  But, I still use the old
 component/action method.  I'd be interested to see what ideas you have for
 this.

 I was thinking of renaming my `bp_component` query variable to
 `bp_directory` or `bp_page`.

 >  We also should stick to using WordPress's wrappers instead of combining
 the rewrite rules array. Something like what bbPress does now would be
 best, though we may even want to have our own wrappers for easily
 registering component/action/variable style pages. See:
 https://bbpress.trac.wordpress.org/browser/trunk/src/bbpress.php#L728

 Yeah, the bbPress method is better than combining the rewrite rules.

 I can look into using `add_rewrite_tag()` and `add_rewrite_rule()` in a
 second patch, but I would still use `bp_path` to break down the current X
 items.

 I'm definitely not set with the approach used in the first patch, so I'm
 interested in alternative ideas while keeping backwards compatibility with
 the old URI system.

 ----

 Replying to [comment:19 boonebgorges]:

 > Some scenarios: root profiles; multiblog mode; username compat mode; MS
 subdomains; MS subdirectories; WP installed in a subdirectory;
 page_on_front; nested bp-pages

 I've tested username compat, MS subdirectories, WP installed in a
 subdirectory, page_on_front lightly and it works.  Needs more thorough
 testing though.

 > Test compatibility with some popular plugins. It seems to me that the
 approach in r-a-y's patch is actually pretty conservative with respect to
 backward compatibility, but it would be helpful to pick maybe a dozen
 different kinds of plugins to manually test that this is actually the
 case.

 As mentioned above, I've tested on bbPress, BP Follow and BP Docs.

 > We should automate the testing of as much of this stuff as possible. The
 routing stuff can be pretty icky to write unit tests for, but I think it
 will be worth our effort to do so.

 For sure.

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