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

[boonebgorges]

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

[johnjamesjacoby]

In 6954:

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

[johnjamesjacoby]

In 7367:

Rewrite Rules:

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

[johnjamesjacoby]

In 7368:

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

[johnjamesjacoby]

In 7369:

Rewrite Rules:

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

[johnjamesjacoby]

In 7370:

Rewrite Rules:

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

[johnjamesjacoby]

In 7371:

Rewrite Rules:

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

[boonebgorges]

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

[r-a-y]

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.

[r-a-y]

Patch created off r7859.

[johnjamesjacoby]

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.

[r-a-y]

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-y]

Requires 4954.ray.01.patch.

[johnjamesjacoby]

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-y]

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

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

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

[boonebgorges]

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.

[r-a-y]

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.

[boonebgorges]

In 7892:

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

[r-a-y]

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.

[r-a-y]

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

[netweb]

Related: ​#bbPress2704

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

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.

[johnjamesjacoby]

In 9472:

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

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/

[imath]

Hi everyone!

I’m reopening this ticket to suggest a new approach about migrating from our legacy URL parser to the WordPress Rewrite API.

First, I’d like to thank @r-a-y for his work and the time he spent building the 3 patches. It helped me a lot to identify the difficulties and the potential impacts of such a change. And at the same time I’m telling him « sorry » because I chose another road to reach the goal of this ticket: I suggest to use the existing BP Component’s methods to add rewrite tags, rewrite rules, permalink structures and to parse the WP Query for each directory component.

My idea is to work on this change progressively because, as r-a-y explained:

• a lot of BuddyPress globals such as the current component, item, action or action variables are set before the WP Query is parsed.
• the Member or Group BP Navs are set before the WP Query is parsed
• the “late included” files are included before the WP Query is parsed too.
• Some specific features like adding a directory page as the site’s home page or making a directory page a sub-page of another WordPress page are pretty challenging to maintain.

I think it would be difficult to add this change without breaking some plugins or confusing some users habits with BuddyPress.

That’s why I chose to use an “experimental” option to our options screen and to make the change reversible. I think this would leave the time to users to get used to it and to plugin authors to adapt to this new way of parsing URLs. Users could experiment Rewrites and eventually inform plugin authors they need to fix things and come back to the legacy URL parser.

Once the user activate this experimental option, the “Pages” tab is replaced by a “URLs” one and it’s now possible to edit all directories slug and for this first step all the primary nav items of the displayed member as shown on the screen capture below.

If the site’s were using BuddyPress directory pages into one or WP Menus, these pages are still there but they now have a different post type “BuddyPress directory”. The custom slugs are still saved within the post_name field of this post type for directory pages and for the navigation I chose to use a post meta.

If you switch back to the legacy parser disabling the “Custom URLs” option: everything is brought back to their previous state.

If you are using Custom URLs, you can now choose Plain permalinks 🙂 Below is an example of the User’s profile page.

I’ve tried to edit as less code as possible and chose to use filters to rebuild links (see src/bp-core/bp-core-rewrites.php. You’ll see that some parts are quite challenging (eg: the root profiles or Ajax). I made sure the few edits I had to make had no impact on the “legacy URL parser BP” (Unit tests are still successful).

There are still a lot of links to check and I think we can improve Ajax by only doing the needed “WP” parse for registered BuddyPress ajax actions. But you can already have a good preview of the result applying the ​4954.patch.

What do you think ? What about doing a first step for the 6.0.0 milestone ? What about leaving it as an experimental option for 1 or 2 releases before deprecating the legacy URL parser ?

PS: if you want to see how I’ve built the patch step by step → ​https://github.com/imath/BuddyPress/commits/wp-rewrites

[imath]

Move the first tickets to next major release.

[imath]

We won't have time to discuss about it during 6.0.0 and I believe it's not easy to progress on such a change. Working on the patch made me realized we can work on this from a feature as a plugin. I will try to find the time soon to put this together.

[imath]

Hi everyone,

Thanks for your contributions so far, the remaining work will happen in the ​BP Rewrites feature as a plugin.

Here's a detailed post about it: ​https://bpdevel.wordpress.com/2021/08/13/bp-rewrites-1-0-0-alpha/

[teeboy4real]

@imath is there any guarantee that BP Rewrites will be added to buddypress core or will it be a standalone plugin.

[imath]

Hi @teeboy4real

The goal is to have it merged into Core. Using a feature as a plugin will help us to have more people test it: a plugin is always easier to install than applying a patch. We believe for such a change, we need a lot of testing to make sure we can make it as smoothly as possible.