Skip to:
Content

Opened 2 years ago

Last modified 3 months ago

#4954 new task

Migrate BP's custom URI parser to use WP's Rewrite API

Reported by: boonebgorges Owned by: johnjamesjacoby
Milestone: Future Release Priority: high
Severity: major Version:
Component: API - Rewrite Rules Keywords:
Cc: david@…, vivek@…, sascha@…

Description

BP's custom URI parser (living mostly in bp_core_set_uri_globals()) is slow, error-prone, non-extensible, non-testable, and out of step with WP best practices. This ticket is a master ticket for the task of migrating from our current URI parser to the WordPress Rewrite API.

JJJ will be leading this task. Here's an outline of what will need to be covered, which I'm sure he and others will improve:

  • Port some of the basic rewrite architecture from somewhere like bbPress
  • Generalize some of the logic, to account for BP's system of current_component, current_action, action_variables
  • Ensure backward compatibility with plugins that add arbitrary sub-paths to BP addresses, whether it be via BP_Group_Extension, BP_Component, or manual detection of $bp->current_component etc
  • Add administation pages for the manual modification of slugs (see eg #2086)
  • Maintain backward compatibility with slugs set via constant (eg BP_GROUPS_SLUG)
  • Develop a tool for automatic migration from bp-pages to new slug system. In addition to the rewrite rules themselves, migration tool should be sensitive to Menus that include reference to bp-pages
  • Unit tests as appropriate

There is a fairly large number of tickets, both in and out of the 1.8 milestone, that will become irrelevant after the bp-pages schema has been abandoned. (Some will not necessarily be irrelevant, but they will require a totally different approach, and so should not be addressed until after the rewrite stuff is at least basically in place.) For the moment, I'm leaving those tickets open, in case the rewrite stuff were to take more than one dev cycle. An incomplete list from the 1.8 milestone: #2086 #4367 #4630 #4726 #4882 #4884 #4913 #4917 #3998

Attachments (3)

4954.ray.01.patch (54.8 KB) - added by r-a-y 15 months ago.
Patch created off r7859.
4954.ray.01b.ajax.patch (3.1 KB) - added by r-a-y 15 months ago.
Requires 4954.ray.01.patch.
4954.ray.01b.path.patch (511 bytes) - added by r-a-y 15 months ago.
Requires 4954.ray.01.patch - Fixes calculating path for backpat globals

Download all attachments as: .zip

Change History (33)

comment:1 @johnjamesjacoby2 years ago

In 6954:

Introduce bp_add_rewrite_tags() and connect bp_generate_rewrite_rules() sub-action. See #4954.

comment:2 @boonebgorges23 months ago

  • Milestone changed from 1.8 to 1.9

comment:3 @ddean21 months ago

  • Cc david@… added

comment:4 @johnjamesjacoby20 months ago

In 7367:

Rewrite Rules:

Add sub-actions for registering rewrite tags, rules, and permalink structures. See #4954.

comment:5 @johnjamesjacoby20 months ago

In 7368:

Add rewrite rules and permalink structure methods to BP_Component class. See #4954.

comment:6 @johnjamesjacoby20 months ago

In 7369:

Rewrite Rules:

Add parse_query() method to BP_Component class, allowing each component to have dedicated query parsing logic. See #4954.

comment:7 @johnjamesjacoby20 months ago

In 7370:

Rewrite Rules:

Wrap $query param in array and pass byref in BP_Component::parse_query(). See #4954.

comment:8 @johnjamesjacoby20 months ago

In 7371:

Rewrite Rules:

Hook bp_parse_query() to 'parse_query' allowing components to parse the main query if needed. See #4954.

comment:9 @sooskriszta19 months ago

  • Cc vivek@… added

comment:10 @r-a-y18 months ago

  • Milestone changed from 1.9 to 2.0

comment:11 @ircbot15 months ago

This ticket was mentioned in IRC in #buddypress-dev by jjj. View the logs.

comment:12 @boonebgorges15 months ago

In 7809:

Store directory_pages data in the persistent cache.

This information is needed during every bootstrap, yet rarely changes, so there
should be significant performance benefit from caching it.

Invalidation takes place when one of the pages is updated, or when the bp-pages
option is changed.

Note that this change will likely be moot after migrating to the WP rewrite
API. See #4954.

See #5362

Props Denis-de-Bernardy

comment:13 @r-a-y15 months ago

Attached is a first pass at rewrite rules.

This patch has no slug administration stuff and still uses our existing BP directory pages as I wanted to port our current URI functionality to work with rewrite rules at least for a first pass.

Note: When you apply the patch, make sure to manually flush the existing rewrite rules by going to "Settings > Permalinks" in the admin area.

Some dev notes:

  • Current URI properties are set on 'init' - Our old method of URI parsing is defined in the bp_core_set_uri_globals() function, which runs on 'init'. With rewrite rules, URI parsing starts on 'parse_query', which runs later than 'init'. This has the potential to break plugins looking for stuff like the current_X items, displayed_user or current_group objects on 'bp_init'. To workaround this, I had to unfortunately duplicate some logic. See the backpat_globals() method in the members and groups component loaders, which uses the new bp_core_get_from_uri() function. It's definitely not pretty, but perhaps we can leave this in for a few releases with the intent of removing this method entirely in a future release.
  • Some hooks are moved to 'bp_parse_query' - bp_setup_nav() and _bp_maybe_remove_redirect_canonical() now run on 'bp_parse_query' to account for the URI property changes above. This should rarely break anything.
  • AJAX and bp_current_X() checks - With rewrite rules, the 'parse_query' hook does not run during AJAX requests. That means any BP URI property checks (using a bp_is_current_X() function) during AJAX will fail. To workaround this, I decided to keep on using bp_core_set_uri_globals(), but run it on AJAX only. I don't see a better way around this without using some version of that function during AJAX. Might build upon the new bp_core_get_from_uri() function, which is lighter than the old bp_core_set_uri_globals(). Will look into this some more.
  • Shortlink canonical redirection - Try something like domain.com/?bp_user_id=1 or domain.com/?bp_group_id=1. This will redirect to the pretty permalink. Still a lot of work to do to our output link functions if we want to support non-fancy permalinks entirely.
  • bp-default compatibility - Yup, still works :) I've tweaked bp_core_load_template() to only run on bp-default set ups, while making sure the existing filters in that function will still work for plugins that use them. This isn't needed any more. I've patched this in a later version of BP. When repatching rewrite rules later on, ignore most of the stuff in bp_core_load_template() except for the stuff at the end as we'll want to remove the last else block.
  • BP directory page as front page - Briefly tested with the activity directory and this also works fine. Though I should point out that if we are getting rid of WP pages, setting a BP component as the front page will be a little harder to implement.
  • Page menu highlighting - Not sure how we're going to tackle this while keeping backward compatibility.
  • Existing routing unit tests pass - With some modifications to initialize and flush rewrite rules.
  • Root profiles doesn't work yet - Still need to look into this. First thoughts are this is going to be a PITA.
  • Search functionality doesn't work yet - Ran out of time for this patch. Need to probably write a specialized rewrite rule for BP_SEARCH_SLUG.

Feedback appreciated.

Last edited 3 months ago by r-a-y (previous) (diff)

@r-a-y15 months ago

Patch created off r7859.

comment:14 @johnjamesjacoby15 months ago

At a cursory, this is a really great first step.

In my imagination, the upgrade routine to 2.0.0 would look at the existing bp_pages setting, and create the appropriate rewrite rules for each directory. If no bp_page setting exists, we'd use a constant, or finally fall back on the default slug.

I think root profiles won't be that bad. Existing code for bbPress's root forums handles this scenario, and I'm hoping that using rewrite rules will allow any component to exist in the root, barring the usual and obvious slug collision problem that exists in WordPress anyways.

comment:15 @ircbot15 months ago

This ticket was mentioned in IRC in #buddypress-dev by r-a-y. View the logs.

comment:16 @r-a-y15 months ago

I think root profiles won't be that bad. Existing code for bbPress's root forums handles this scenario, and I'm hoping that using rewrite rules will allow any component to exist in the root

I'll have to take a look at how bbPress does root forums!


01b.ajax.patch is an alternative way to address the AJAX issue brought up in comment:13.

(Requires the earlier 01.patch)

This technique loads up the WordPress query with the wp() function during AJAX requests, which will allow 'parse_query' to run... well, almost! bp_parse_query() bails when in the admin area. There's probably a good reason why bp_parse_query() does this, so I left that function untouched and added a stub method to run our 'bp_parse_query' hook.

Hat-tip to Boone for pointing me in this direction.

The only other issue is the hardcoded PHP_SELF. I'm not sure if that value changes in different environments.

@r-a-y15 months ago

Requires 4954.ray.01.patch.

comment:17 @johnjamesjacoby15 months ago

bp_parse_query bails in wp-admin because it's only intended to be used to parse the query out into rewrite rule directives, and it's an unfortunate consequence of using WordPress's AJAX URL that this has to happen within admin_ajax.

The way bbPress gets around this is by introducing a dedicated set of theme-side AJAX handlers, which is a direction I support us going in eventually that should probably have a separate ticket.

@r-a-y15 months ago

Requires 4954.ray.01.patch - Fixes calculating path for backpat globals

comment:18 follow-up: @johnjamesjacoby15 months ago

One thought after applying this and stepping through it, I'm worried it's more invasive to the existing URI router than what I had imagined. I'm thinking we should be able to bolt the rewrite rules on top of the existing functionality, and de-prioritize the old URI router similar to the way we did with theme compatibility. 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.

Thoughts on renaming bp_path to bp_action to retain the existing component/action/variables naming and relationships we're so used to now? Or, rather than having BuddyPress act as a central router again (with standard component/path/action vars) if maybe they should always be unique. Something like:

  • bp_directory=bp_members = Members Directory
  • bp_member_id=1 = Member Profile for User ID 1

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

comment:19 in reply to: ↑ 18 @boonebgorges15 months ago

Replying to johnjamesjacoby:

One thought after applying this and stepping through it, I'm worried it's more invasive to the existing URI router than what I had imagined. I'm thinking we should be able to bolt the rewrite rules on top of the existing functionality, and de-prioritize the old URI router similar to the way we did with theme compatibility. 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.

r-a-y's patch works in part by setting up the current_component, current_action, and action_variables. My understanding is that in this way, plugins don't need to use rewrite rules if they don't want to. They can continue to use bp_is_current_component() and even reference the global properties directly if they want to.

Philosophically (if I may ;) ) I think that if we can do this in such a way as to complete remove reliance on bp_core_set_uri_globals(), we should do so. As I understand it, this is the whole impetus for moving to rewrite rules. The fact that r-a-y's patch allows this strikes me as a strength rather than a weakness.

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

The problem is that bp_path is an array of what we currently call current_action and action_variables.

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

I like this. But to my mind, the strategy here is to do this as a progressive enhancement. We can set these items and use them internally (eg, as the internals of bp_is_user() or bp_is_directory(). But we should continue to set the old current_component, current_action, action_variables for backward compatibility reasons.

===

I think the general direction r-a-y's patch is good. However, it also terrifies me. It would be nice to draw up a plan for what needs to be tested, and how it's going to work. A couple preliminary thoughts:

  • Some scenarios: root profiles; multiblog mode; username compat mode; MS subdomains; MS subdirectories; WP installed in a subdirectory; page_on_front; nested bp-pages
  • 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.
  • 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.

comment:20 @r-a-y15 months ago

Replying to 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, you'd be right that they might not work. By "not work", I mean the third-party component's single page might not load up. If anyone knows of any plugin that add a root component, please let me know. I think there is a premium events plugin out there.

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 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.

Last edited 15 months ago by r-a-y (previous) (diff)

comment:21 @boonebgorges15 months ago

In 7892:

Add some basic unit tests for root_profiles routing. See #4954

comment:22 @landwire14 months ago

  • Cc sascha@… added

comment:23 @r-a-y14 months ago

  • Milestone changed from 2.0 to 2.1

As discussed in this week's dev chat, there's not enough time to address all the issues for 2.0.

Moving to 2.1.

comment:24 @r-a-y9 months ago

  • Milestone changed from 2.1 to 2.2

JJJ stated that this wouldn't make 2.1 last week. Off to 2.2!

comment:26 @DJPaul5 months ago

  • Milestone changed from 2.2 to Future Release

comment:27 @johnjamesjacoby3 months ago

In 9466:

Replace all references to bp_get_root_domain() . '/' . bp_get_groups_root_slug() with the appropriate bp_get_groups_directory_permalink() function. This ensures all usages and filters are applied uniformly. See #4954 for the long-game here.

comment:28 @johnjamesjacoby3 months ago

In 9467:

Replace all references to bp_get_root_domain() . '/' . bp_get_members_root_slug() with bp_get_members_directory_permalink(), ensuring all usages and filters are applied uniformly. See #4954.

comment:29 @johnjamesjacoby3 months ago

In 9468:

Replace all references to bp_get_root_domain() . '/' . bp_get_blogs_root_slug() with bp_get_blogs_directory_permalink(), ensuring all usages and filters are applied uniformly. See #4954.

comment:30 @johnjamesjacoby3 months ago

In 9472:

Update bp_blogs_creation_location() to use bp_get_blogs_directory_permalink(). See #4954.

Note: See TracTickets for help on using tickets.