#4954 closed enhancement (fixed)
Migrate BP's custom URI parser to use WP's Rewrite API
Reported by: | boonebgorges | Owned by: | imath |
---|---|---|---|
Milestone: | 12.0.0 | Priority: | high |
Severity: | major | Version: | |
Component: | Route Parser | Keywords: | has-patch has-unit-tests |
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 (4)
Change History (108)
This ticket was mentioned in IRC in #buddypress-dev by jjj. View the logs.
11 years ago
#13
@
11 years 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 thecurrent_X
items,displayed_user
orcurrent_group
objects on'bp_init'
. To workaround this, I had to unfortunately duplicate some logic. See thebackpat_globals()
method in the members and groups component loaders, which uses the newbp_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
ordomain.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 tweakedThis 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 inbp_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.bp_core_load_template()
except for the stuff at the end as we'll want to remove the lastelse
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.
#14
@
11 years 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.
This ticket was mentioned in IRC in #buddypress-dev by r-a-y. View the logs.
11 years ago
#16
@
11 years 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.
#17
@
11 years 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.
#18
follow-up:
↓ 19
@
11 years 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 Directorybp_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
#19
in reply to:
↑ 18
@
11 years 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.
#20
@
11 years 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, 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 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.
#23
@
11 years 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.
#24
@
10 years ago
- Milestone changed from 2.1 to 2.2
JJJ stated that this wouldn't make 2.1 last week. Off to 2.2!
#25
@
10 years ago
Related: #bbPress2704
#34
@
7 years ago
- Keywords trac-tidy-2018 added
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/
#35
@
7 years ago
- Milestone Awaiting Contributions deleted
- Resolution set to maybelater
- Status changed from new to closed
#36
@
5 years ago
- Keywords has-patch dev-feedback added; trac-tidy-2018 removed
- Milestone set to Up Next
- Resolution maybelater deleted
- Status changed from closed to reopened
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
#37
@
5 years ago
- Milestone changed from Up Next to 6.0.0
Move the first tickets to next major release.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
#40
@
5 years ago
- Milestone changed from 6.0.0 to Awaiting Contributions
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.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
3 years ago
#42
@
3 years ago
- Keywords has-patch removed
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/
#43
@
3 years ago
@imath is there any guarantee that BP Rewrites will be added to buddypress core or will it be a standalone plugin.
#44
@
3 years ago
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.
#45
@
20 months ago
- Keywords needs-patch added; dev-feedback removed
- Milestone changed from Awaiting Contributions to 12.0.0
- Owner changed from johnjamesjacoby to imath
- Status changed from reopened to assigned
BuddyPress will use the WP Rewrite API although we didn't get enough BP Rewrites testers.
As discussed into various BP Slack discussions and announced into the 11.0.0 Hello Screen (modal displayed when you activated BuddyPress) or into the 11.0.0 release post, we'll switch our current URL parser in favor of using the WP Rewrite API.
This will be our main focus for the 12.0.0 release.
This ticket was mentioned in PR #65 on buddypress/buddypress by @imath.
20 months ago
#46
- Keywords has-patch has-unit-tests added; needs-patch removed
Create the bp-core-rewrites.php
file to put BP Rewrites function inside it & edit the BP_Component class to generate rewrites when a component has a directory.
Compared to the BP Rewrites plugin, I've put some new code to make it easier for plugin developers to create BP Rewrites for their custom component. Most of the job can be done by setting the rewrite_ids
values from this component eg: $component->rewrite_ids = array( 'directory' => 'custom_rewrite_id' )
.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #67 on buddypress/buddypress by @imath.
20 months ago
#49
The custom post type name is buddypress
. Why?
- It should be a unique name no other plugins chose
- Using
buddypress
should help us enjoy WP template hierarchy
Bump raw db version to the first commit number we made about BP Rewrites.
Add an upgrade routine to edit existing directory pages post type as well as nav menu items.
Stop using get_page_by_path()
in favor of bp_core_get_directory_page_id()
.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #69 on buddypress/buddypress by @imath.
20 months ago
#51
It introduces the bp_rewrites_get_url()
function. Its role is to build every BuddyPress URL using the BP Rewrites API.
It also deprecates softly some key functions like bp_get_root_domain()
to let us review all BP links during 12.0 development cycle and make them use the bp_rewrites_get_url()
function or a wrapper of it.
It also deprecates slug constant as we will be able to customize every slugs from the future "URL" tab of BuddyPress settings page.
It puts the Components $rewrite_ids
in place and deprecates the $root_domain
BP global.
This PR will soon be completed with Unit tests for the bp_rewrites_get_url()
function.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #70 on buddypress/buddypress by @imath.
19 months ago
#53
bp_members_get_user_url()
is a wrapper of bp_rewrites_get_url()
. It replaces bp_core_get_user_domain()
to build Member URLs using BP Rewrites.
As many URLs are built concatenating bp_core_get_user_domain()
with URL chunks, the safer way to make sure Developers update the way they build their BuddyPress links is to deprecate this function.
bp_core_get_username()
has also been deprecated in favor of bp_members_get_user_slug()
.
This PR also updates some PHPUnit tests!
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #73 on buddypress/buddypress by @imath.
19 months ago
#55
- Improve the Members component adding permastructs and custom rewrite rules for registration and activation pages.
- Improve the Blogs component adding custom rewrite rule to handle the Blogs create page.
- Make a huge progress about replacing all occurrences of
bp_get_root_domain()
(45 replacements were performed). There might be some last ones inside PHPUnit tests. - Deprecate
bp_displayed_user_domain()
,bp_loggedin_user_domain()
,bp_groups_directory_permalink()
,bp_get_groups_directory_permalink()
,bp_blogs_directory_permalink()
&bp_get_blogs_directory_permalink()
and replace them with these new functions:bp_displayed_user_url()
,bp_loggedin_user_url()
,bp_groups_directory_url()
,bp_get_groups_directory_url()
,bp_blogs_directory_url()
&bp_get_blogs_directory_url()
. These deprecations are required because these functions were used to build some BuddyPress URLs concatenating URL chunks. - Start replacing
bp_loggedin_user_domain()
&bp_displayed_user_domain()
with the new functions.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
19 months ago
#56
This is a real bad idea. This is going to create havoc as the functions you are trying to deprecate are used very commonly.
Why not change the internal code of the functions to use current method for generating url instead of deprecating? what are the cons of doing that?
If the current deprecation spree keeps going on with the core functions, this will kill the whole BuddyPress eco system(not all themes and plugins are going to update their code for the trivial reason of name change).
Please reconsider it.
19 months ago
#57
Hi,
Thanks for your feedback. It's nice to read from you, it's been a while. If you can reply with a solution to this question I'm very open to reconsider it.
How can you know when a plugin author uses this code: bp_blogs_directory_permalink() . 'create/'
? I need to make sure to convert it to an URL built using the BP Rewrites so that it can use the customized slug and be reachable using pretty or plain links ?
19 months ago
#58
Hi,
Thank you for your work on the BuddyPress core!
It is nice to participate again.
As far as I am aware BuddyPress has limited number of end point which allow create actions(group,blogs).
If we look at these 2, we only have to worry about these 2 end points with hard coded action. A simple solution will be to add a canonical redirection from old url to new and have a filter to toggle this behaviour for these two.
For everything else, there are proper functions available in BuddyPress that points to the url. My main concerns were bp_loggedin_user_domain, bp_displayed_user_domain which are pervasive. These were marked deprecated in this commit
https://github.com/buddypress/buddypress/pull/73/commits/5da133d58cf97310ce697471bafd892713220cac
I am looking forward to spend time with the rewrite changes this week and have a better grasp to provide any feedback/assistance from next week.
19 months ago
#59
I wish it there was "proper functions available in BuddyPress that points to the URL".
I've spent 2 years on the BP Rewrites plugin and I can tell you it's not the case. Here are some examples from our code base:
- bp_displayed_user_domain() . bp_get_profile_slug() . '/change-avatar/delete-avatar
- bp_loggedin_user_domain() . bp_get_members_invitations_slug() . '/send-invites'
- bp_displayed_user_domain() . bp_get_messages_slug() . '/' . bp_current_action()
There are a lot more.
So you're suggesting to redirect all hardcoded URLs?
19 months ago
#60
Great points.
In my opinion, a function should be deprecated if the stop doing what they are supposed to do.
How a function is used by others may not be a ground for it. Even the newly introduced functions can be misused and hard coded by some people.
I will put one example:-
bp_loggedin_user_domain() - The purpose of this function is to provide url of the logged user. Has that changed? is that not achievable anymore by internally modifying it? If the purpose did not change, there is no rationale behind deprecating it.
Same goes for others.
Core can not track all the use cases and can not stop the misuse but the core can update the function to reflect the current approach and also replace the hard coded instance in the core itself(if there are some).
The goal should be to allow users minimum friction to update.
Even with the new rewrite API, the end points are not going to change automatically unless the admin does it manually. Making them aware that they test their change for url and report to the breaking plugin/theme will save a lot of time and resentment from community.
Also, when I said redirect, my intention was for group/blog creation only. I don't see any harm in doing it from other end points as part of a back compat plugin if enough people demand it.
Thank you for your attention.
19 months ago
#61
@sbrajesh
Thanks for your feedback. I'm more in line with this definition of deprecation. These 2 functions will not be used anymore in BuddyPress once BP Rewrites will be fully merged. As we used to use them the wrong way, concatenating URL chunks, plugin authors did the same and it's fine. We need to stop doing so in favor of new functions making sure it's possible to customize slugs, be compatible with plain links and comply with a WordPress standard: the WP Rewrite API. I believe our role is to tell plugin developers they need to update their plugin for community benefits. When a user will have an issue he won't look for the code responsible for the fail, he will write a topic into our support forum giving more work to people contributing to support.
We agreed on this scenario:
- move to BP Rewrites
- build a BP Classic plugin for plugin who won't update their code.
My plan is to deprecate the problematic functions to be sure thanks to notices I replaced these functions from our code base.
Then I can move these functions into the BP Classic plugin. If this plugin is not used the deprecation notices will help us to inform people are using a plugin requiring the BP Classic plugin to be compatible with BuddyPress.
If I don't have this deprecation notices, I'm losing this possibility.
As @dcavins and you seem very concerned about the two functions (logged in & displayed domains). I'm going to leave them the way they are for now (although I think it will end up generating issues). It will make my work more difficult. Then we'll discuss again about these 2 functions. So far your arguments haven't convinced me, but I need to move on.
I'll commit this PR soon.
19 months ago
#62
Hi @imath
Thank you.
I am sure I know a bit or two about deprecation. My concern is your reason behind deprecation.
In order to deprecate something, you have to think about its impact and the benefits. is it absolutely necessary?
When you allow 3rd parties to use your API, you need to consider it even more deeply than if you were using your own APIs.
I am sure you have good intention but the direction and rationale is plainly incorrect. It is like knowingly breaking every existing BuddyPress theme and plugin(yes, throwing notice is same as breaking).
With the current shrinking market that BuddyPress has I doubt that there will be a recovery from this move. It will be much prudent for developers to work with platforms which provide stability like WordPress does instead of working with a volatile API.
It is your decision as you have put a lot of work in BuddyPress but I will suggest that you re-asses the way WordPress and the APIs WordPress eco system works. When did you remember a major API break in WordPress? There was a time around 10 -12 years ago when a change in $wpdb->query had a wide impact but other than that, it has always been trivial change for existing applications.
This is how a software that has a large eco system/dependent plugin systems work. APIs are like contract and you don't break because you got a new one. you break it when it can't be maintained.
Anyway, I am not a big fan of breaking existing things for the sake of changing names and what you are doing is going outside the scope of the core to force others to update. Not sure if that will help or be harmful for the BP.
With that I will leave it for you to take the final decision.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
19 months ago
This ticket was mentioned in PR #75 on buddypress/buddypress by @imath.
19 months ago
#65
- Add rewrite tags/rules for the groups directory type and the create URLs
- Introduce
bp_get_group_url()
/bp_group_url()
& to retrieve/output a Groups single item URL using BP Rewrites. - Introduce
bp_get_groups_directory_url()
/bp_groups_directory_url()
to retrieve/output the Groups directory URL using BP Rewrites. - Introduce
bp_get_group_restricted_screens()
,bp_get_group_extension_screens()
&bp_get_group_screens()
to get information about the Groups screens (in particular each screen rewrite ID). - Perform easiest replacements for
bp_get_group_permalink()
/bp_group_permalink()
&bp_get_groups_directory_permalink()
/bp_groups_directory_permalink()
. - Improve code formatting & properly escape single group URLs into templates.
- Update impacted Unit Tests
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #77 on buddypress/buddypress by @imath.
19 months ago
#67
BP_Core_Nav
is the class we use to set up BuddyPress single item navigations. It now only needs slugs to build URLs.
- Remove all components
parent_url
attributes when setting sub nav items. - When a plugins will specify a
link
for the main nav item or aparent_url
for the sub nav item, it will use this link to preserve backward compatibility. - The PR also migrates the Community search feature so that it uses BP Rewrites.
- Update some PHPUnit tests.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #78 on buddypress/buddypress by @imath.
19 months ago
#69
- Introduce a function to get all component navigations.
bp_get_component_navigations()
will be used to make it easier to get all customizable slugs within the BuddyPress settings area. - Perform all remaining
bp_loggedin_user_domain()
replacements in favor ofbp_loggedin_user_url()
.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #79 on buddypress/buddypress by @imath.
19 months ago
#71
- Replace all remaining
bp_displayed_user_domain()
usage in favor ofbp_displayed_user_url()
. - Introduce the
bp_members_get_path_chunks()
function to quickly build BP Rewrites argument for member's URL using an array of slugs. - Deprecate
bp_activities_member_rss_link()
,bp_blogs_blog_tabs()
&bp_groups_header_tabs()
, - Improve
bp_displayed_user_link()
so that it's possible to pass an array of slugs to output an escaped BP Rewrites ready URL.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #81 on buddypress/buddypress by @imath.
18 months ago
#73
- Introduce the
bp_groups_get_path_chunks()
function to build BP Rewrites arguments using a regular array. - Improve
bp_members_get_path_chunks()
so that it avoids looking for a custom slug when the chunk is a numeric value. - Introduce the
bp_get_group_manage_url()
function to build Group's front-end Admin URLs using BP Rewrites. - Deprecate
bp_group_admin_permalink()
&bp_get_group_admin_permalink()
in favor ofbp_group_manage_url()
&bp_get_group_manage_url()
. - Replace all remaining usage of
bp_get_group_permalink()
in favor ofbp_get_group_url()
orbp_get_group_manage_url()
.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #83 on buddypress/buddypress by @imath.
18 months ago
#75
- Start moving deprecated functions inside a
function_exists( 'bp_classic' )
check, corresponding functions were added inside the BP Classic backcompat plugin. - Replace all remaining deprecated functions usage.
- Improve
bp_groups_get_path_chunks()
to deal with front-end group admin URLs and the groups create URLs. - Adjust some routing unit tests
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #84 on buddypress/buddypress by @imath.
18 months ago
#77
Compared to commit 13442, change the logic of Components user navigation generation by introducing a BP_Component::register_nav()
function to globalize the nav items and make them available for the added URLs WP Admin settings screen. After a second thought, using BP_Component::setup_nav()
to play the registration role can be problematic considering backward compatibility. We are maintaining BP_Component::setup_nav()
to play the role of generating the user navigation, Third party plugins wishing their slugs to be customizable will need to "opt-in" BP Rewrites using BP_Component::register_nav()
.
This first version of the URLs WP Admin settings screen does not handle slug customization yet, it's used to make sure all BP Component user navigation slugs can be customized.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #85 on buddypress/buddypress by @imath.
18 months ago
#79
- Edit BP_Groups_Component::setup_nav() so that it uses
bp_get_group_screens()
to generate the single group's navigation items. bp_get_group_screens()
is making it possible to get all single group's nav item slugs from the BP Urls settings tab, use it to add these slugs into the Accordion UI.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #86 on buddypress/buddypress by @imath.
18 months ago
#81
Add bp_rewrites_pre_get_slug
filter to bypass slug customization. This filter will let the BP Classic plugin apply a classic BuddyPress experience (without BP Rewrites improvements).
Improve the BP_Group_Extension
to support BP Rewrites and do some code formatting cleanup.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #87 on buddypress/buddypress by @imath.
18 months ago
#84
- Start implementing the
parse_query()
methods for the Activity/Blogs and Members components. - Remove the notice used to force pretty permalinks
- Use plain links to test
parse_query()
methods are rightly setting BuddyPress URI globals - Introduce some helper functions used during the BP Rewrites API parsing process :
-
bp_is_directory_homepage()
is checking if a BP Directory is used as the site's homepage. bp_rewrites_get_custom_slug_rewrite_id()
finds a Rewrite ID out of a custom slug.bp_rewrites_get_member_data()
returns the field to use to get the user by.bp_reset_query()
makes it possible to parse again a request in specific cases (eg: root profiles)
-
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #88 on buddypress/buddypress by @imath.
18 months ago
#86
Implement parse_query()
method for Groups component
Create a method to set the current group and be able to use it inside the parse_query()
method of the Groups component
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #91 on buddypress/buddypress by @imath.
18 months ago
#88
- Introduces
bp_core_setup_query_parser()
to decide when to postpone some key hooks firing tobp_parse_query
. - Introduces
bp_core_get_query_parser()
to get the query parser in use. It contains a filter BP Classic will be able to use to force the Legacy URL parser. So far it useslegacy
for pretty links andrewrites
for plain links. - Edit
bp_redirect_canonical()
&bp_get_canonical_url()
so that they use the BP Rewrites API. - Introduces
bp_core_set_ajax_uri_globals()
to set the BuddyPress URI globales using the BP Rewrites API inside the Ajax context thanks to the updatedbp_reset_query()
function. - To avoid using
WP()
at each Ajax call, introduces a simple Ajax actions registration process thanks to the newbp_ajax_register_action()
. - Update Legacy & Nouveau template packs so that they use this logic. As BP Default will require the legacy URL parser and will be moved inside BP Classic, the theme can stay the way it is.
- Improve the
bp_rewrites_pre_get_slug
filter logic making it depends on thebp_core_get_query_parser()
function. - Make sure to reset the Members navigation to a new BP_Core_Nav based on the displayed user once the user is available in the components
parse_query()
method.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #94 on buddypress/buddypress by @imath.
17 months ago
#90
- Deprecate
bp_core_set_uri_globals()
&& completely stop using the BP Legacy URL parser in favor of BP Rewrites 🙌. This function will be moved inside the BP Classic compatibility plugin see https://github.com/buddypress/bp-classic/pull/3 - Introduce a new hook
bp_register_nav
to hook to when globalizing Members single item navigations inBP_Component
. - Improve
bp_get_component_navigations()
so that Avatar/Cover images nav items are moved inside the Profile subnav if Extended profile is active - Improve
BP_Core_Nav
so that any Action Variable slugs can be customized. - Improve Members & Groups component canonical redirections.
- Handle slugs customization persistency using directory pages post metas
- Introduce a new repair tool to reset all slugs to BuddyPress default one.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
This ticket was mentioned in PR #95 on buddypress/buddypress by @imath.
17 months ago
#92
- Make sure BP Activity Embeds are supported when using the BP Rewrites API
- Deprecate
bp_admin_display_directory_states()
and move the BP Page directory states feature to BP Classic, see https://github.com/buddypress/bp-classic/pull/4 - Remove the Page settings Admin help tab to move it in BP Classic
- Introduce the
bp_core_include_directory_on_front()
in order to maintain the possibility to use a BP Directory page as the site's home page.
PS: this will be the last step of the BP Rewrites merge process. All new issues about the BP Rewrites API will be managed in new tickets.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/4954
In 6954: