BP REST API - V2

[espellcaste]

Custom REST API endpoints from BuddyPress are handled into a separate Github repo: ​https://github.com/buddypress/BP-REST/

This means that any REST API code is first worked on there, and we bundle it with BP core when releasing a version.

This creates an odd behavior for code development. The need to set up another project locally. And it might create some issues too for testing. ​See this for a recent example.

That repo was very useful to me (I'm the one that advocated for it in the past) when I started to contribute to it since I was not very savant with SVN. However, that changed and I think we should consider and discuss if we should retire this project and bring everything in house.

Some of my reasons are:

• no need to maintain Github Actions in another project
• no need for tickets and github issues
• if you svn buddypress, you get BP REST API too
• ^{no need to setup another project}
• any phpcs rule we apply to core, it is reflected in the BP REST API (since they'll be in the same codebase)
• any improvements in the running of the unit tests, BP REST benefits See #9081

We need to consider:

• Unit tests
• Remove deployment integration
• Loading of files
• Is there any benefit in keeping the separate repo now?

There is no rush to make this move. But I'd like to start a conversation about it. :)

[imath]

Hi @espellcaste

I agree with you. Moreover I think bringing the BP REST API development into Core would improve its usage by all BuddyPress contributors, and would probably be more straightforward for contributors to report issues.

I think we should probably take this opportunity to build a "v2" to fix #8200. Version 1 could be deprecated / included into BP Classic to provide backwards compatibility if needed.

I'm totally in favor of this move.

[espellcaste]

I'm not sure we want to bundle V1 with BP Classic, though. Right?! A community might want to use the V1 API without the other feature(s) from BP Classic.

As to V2, we can actually create it whenever we want. So far, we haven't because there was only that ticket. We were waiting for more reasons for a V2.

I'm totally in favor of this move.

Great! I'll start to think about how to move it smoothly.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/9145

[espellcaste]

I started the move here: ​https://github.com/buddypress/buddypress/pull/337

• Unit tests are good;
• Removed all mentions of the BP REST API plugin;
• Added V1 so that we can tell the V2 apart.
• Changed the name of the classes from endpoint to the more accurate: Controller;
• Tested locally.

It'd be great to have a code review and some testing from other folks too.

We should block new features from being added to the ​original repo. And we can migrate current issues without tickets later. And archive the repo afterwards.

[imath]

Hi @espellcaste

I just did a code review, I'm adding this comment to it:

As to V2, we can actually create it whenever we want

I'm worried this might mean: never or very very late...

I've read again #8200. By the time BP was 6.0.0, the version after the BP REST API has been included in core (5 years ago) @boonebgorges was talking about "backward-incompatible API change requests" not about features.

What's for sure is current API has a design weakness or inconsistency that is requiring a v2:

• Every endpoint get_item(), update_item() and create_item() methods are wrongly returning a list of 1 object.
• This inconsistency compared to the WP REST API makes the API less predictible.
• BP will be 15.0.0, 10 major releases after the one the BP REST API was introduced and this weakness or inconsistency is still there.

To me a serious and responsible move would be to hit 2 birds with one stone & avoid 2 big changes: the merge move and later (how many years?) a v2 move. If I suggested to use the "BP Classic" plugin to include the deprecated version(s) of the BP REST API this was to carry on moving forward while leaving the possibility to users still using plugins slowing down BuddyPress progress by keeping their code out of date to be able to preserve their community features. To me it's easier to say use "BP Classic" compared to use "BP Classic oh and if you need the deprecated version of the BP REST API, use also the BP REST API v1 backward compatibility plugin".

[espellcaste]

@imath I think we have not, or I have not, released a V2 yet due to lack of a strong motive. Since currently, the V2 would only change the shape of the returned object, we have been waiting for more possible breaking changes or features. So, if we all agree, we can add a V2 in BuddyPress 15.0.0 to resolve this inconsistency, because this addition would be minor and low risk. I can add V2 as part of 15.0 :)

Now, for the V1 removal from the plugin and putting into BP Classic, I personally don't see the need there. We could keep both APIs available, which is something common in ​related plugins. To me, moving to BP Classic would be the approach if we were deprecating V1, and we would not support fixes and etc, which I don't think that's what you are suggesting. And it'd defeat the purpose of the ticket, since we would still continue to have another plugin to manage things, which is what I was trying to resolve here. =P

Beyond all that, we could extend the V1 controllers/classes and reuse most of the code, since V2 is not that different from V1 (except for the returned object shape).

But the main argument against bundling V1 into BP Classic is it'd defeat the purpose of the ticket, unless we plan to deprecate it and not add fixes into it.

--

And if this doesn't make sense, we can bundle V1 into BP Classic too. I'd defer to you here. It'd be good to understand what sort of support it would have there. Do we add fixes into it later? Do we have a setting to activate it or just installing the plugin is enough? Just by having BP Classic, do we get V1 too?

[imath]

Thanks for your explanations @espellcaste I think I misunderstood your intent. I thought just like the WP REST API, going v2 would mean deprecating v1 (see #comment:1 the Version 1 could be deprecated part).

Reading your explanations, I understand you want to maintain each possible version of the BP REST API. It's great, but it requires a lot more work.

You took Woocommerce as an example, this project has 1,396 contributors listed on their GitHub repository, we have 17 contributors listed on ours: this makes me think we should consider more reachable expectations, for example: actively maintain the latest version of the BP REST API (and deprecate all previous ones). I think we should try to keep things as simple as possible for us and for the people who will come after us.

I personally think a "more than 5 years" inconsistency is becoming a strong motive to go v2 😅.

My suggestion is to do one bigger change, instead of 2 successive changes. But I'm open to different opinions, as your first intent for this ticket was to simply move v1 here, if the majority of team members also think we should only do that: I can wait for a v2, but the more we wait, the more it will be difficult to deal with making the change (waiting 10 years for rewrites already showed this to us).

[espellcaste]

Creating new versions don't usually mean deprecating old ones. It all depends on what type of support we want to provide (fixes and security updates, etc). It looks to me we don't want to support V1 in any shape or form, so moving into BP Classic would also defeat this purpose, since we would need to provide support via that other plugin too.

We can just move this ticket to create V2, and deprecate the old version. What do you think?

[imath]

Very good point about moving v1 into BP Classic: you're totally right.

We can just move this ticket to create V2, and deprecate the old version.

I think it's what we should do, yes. We'll need to update some code (blocks & group members management) but this work will help us explain what changes are needed to support v2.

[espellcaste]

Perfect. I'll start by updating this ticket to reflect this new goal, and update the pull request moving the REST API code into V2. :)

This is almost ready.

This pull request is ready to be tested. There is some outstanding ​questions for some behaviors.

I'd recommend, for now, to only look at the code/implementation and offer suggestions before diving into testing mode. Just so that by the time we go to QA it, we'll be at a more solid state.

@imath Three clarifying questions:

Looking at this comment related to the _deprecated_class and it seems the location this should be added is inside a class constructor. Which is different from the the location class-bp-patch has it on.

​https://github.com/WordPress/wordpress-develop/blob/9ad162e53ee9732c8ed63150650f7f9d3067a9d7/src/wp-includes/functions.php#L5662-L5663

I do ​see some places where folks are using it outside of the constructor, but it doesn't seem to be the location it was _designed_ to be used.

---

Some of the new classes names are a bit off, like BP_Members_REST_Notices_Controller instead of BP_Members_Notices_REST_Controller. Is that expected?

---

I would add to all these v1 named files a deprecation notice in case it's directly required by a plugin.

I'm really curious if you know of a plugin that's actually loading one of our controller files manually into their own plugin.

That would be very odd, it's as if we would load the file for the WP_REST_Controller class so that we extend it, which would be uncommon to say the least. That technically would mean a conflict since BuddyPress and the other plugin would be loading the classes twice. So that doesn't make much sense to me. 🤔

A ​quick search came up empty but I assume you might know of an example.

And just to be clear, I'm not against this change. I'm mostly curious, would be flabbergasted, if someone would be doing that at all for this type of class/endpoint/controller.

Hi @renatonascalves about the 3 clarifying questions:

1. If _deprecated_class is not the right function to use, I'm fine with a _doing_it_wrong() informing to use v2 controllers instead.
2. About class names, the most important is we make sure to have BP_{component_id}... to be inside the regular map of the autoloader. Your way of naming it with REST_Controller at the end is better I agree.
3. If you thing the above point is enough, I trust you. My reasoning is more "file was there in 14.0, it's no more the case in 15.0, use a deprecated function to inform about it just in case" 😅

@imath

1. _deprecated_class will be moved to the BP REST plugin classes constructor.
2. I'll update with the suggested naming so that REST_Controller is used at the end.
3. That sounds good. We can keep it.

Awesome, maybe we still need to inform about deprecated class usage from BuddyPress when the BP REST plugin is not active?
At least I believe we should remove these class names from the autoloader irregular map.

@imath

• Removed V1 class names from the autoloader irregular map (the BP REST plugin are loading them now);
• V2 classes uses the autoloader, as extected;
• Pr updated with the new class naming _REST_Controller at the end;
• Added a deprecation notice for v1 named files in case it's directly required by a plugin;
• _deprecated_class moved to the class constructor of the BP REST plugin endpoints . The error appears in the debug.log and in the header of the HTTP response.

maybe we still need to inform about deprecated class usage from BuddyPress when the BP REST plugin is not active?

This is not necessary because without the BP REST plugin, a site can't use the V1 endpoints at all. They can only use the V2 endpoints. And if the need to use V1, they need to install the BP REST plugin.

Here is the PR in the BP REST plugin: ​https://github.com/buddypress/BP-REST/pull/512

:)

@imath Awesome! I'll apply the latest suggestions. I actually forgot to rename the phpunit test files related to the endpoints to controller. Good catch!

[espellcaste]

We are ready to merge/commit the V2 of the BP REST API. Before doing so, let me share some of the steps we will take.

• I'll commit the code from this ​pr to BuddyPress trunk.
• I'll merge the code from this ​pr in the BP REST plugin GitHub repo.

The main difference here is that V2 will be bundled with trunk and it'll be the default REST API for BuddyPress. If you need to use the V1 of the BP REST API, you'll need to install the BP REST plugin (more documentation about how to perform this task soon).

After those two are done, we will start making other changes to better support V2 inside BuddyPress itself. Here are some of the tickets in the current milestone:

• #9229
• #8774
• #9231
• #9175
• #9137
• #9157
• #9160
• #8200

And start deprecating V1 in other places too:

• update the current bp rest doc site to use wp.apiRequest.
• move bp_rest_api_register_request_script to deprecated/10.php. See ​here.
• Add _deprecated_function See ​here
• Remove the bp.apiRequest file in BuddyPress 16 (ticket soon)

While those tickets are not fixed, we will keep this one open. I'm glad to say most of them are already done and tested, so we should expect very few issues.

I'll be publishing a blog post in our ​development blog soon about V2 BP REST API and the changes that came along with it.

:)

[espellcaste]

In 14026:

Include the V2 of the BP REST API in BuddyPress core.

We are officially deprecating the V1 of the BP REST API. And bundling the new, default, V2 of the BP REST API inside BuddyPress core. Previously, the V1 was developed as a plugin in a separate repo (​https://github.com/buddypress/BP-REST).

• One of the main differences between the V1 and V2 is how objects are returned. Single items are no longer returned as an array;
• We have a new BP_Test_REST_Controller_Testcase for testing the new API endpoints;
• We changed the names of our controllers to follow our autoloader rules;
• Removed the BP-REST plugin from wp-env and from our release script;
• And we added notices for the deprecated V1 API (endpoints and files).

Props imath & I. ;)

Fixes #8200
See #9145
Closes ​https://github.com/buddypress/buddypress/pull/337

[espellcaste]

In 14027:

Removing the old Group Avatar and Member Avatar endpoints files from the old core location.

Previously those files were located in the core component, and later were moved to their own component. Since the files in the new component were also deprecated as part of V2, we are removing the legacy ones.

See #9145

[espellcaste]

In 14028:

Remove duplicate @since 15.0.0 tags introduced at [14026].

See #9145

[imath]

In 14031:

Remove a PHP Unit asset no more needed

See #9145

[espellcaste]

In 14034:

BP REST API: spammed users can not be retrivied from non-admin users.

Align the BP REST API with the web version, and show spammer users to Administrators only.

Props imath and emaralive.

See #9145
Fixes #9231

[espellcaste]

In 14035:

BP REST API: Allow creating messages with non-empty content.

Make sure it is possible to create messages with non-empty content in the BP REST API. This is a followup from [13986].

Props imath and emaralive.

See #9145
Fixes #9175

Adds the following page to developer doc:

• Introduction about the BP REST API
• Reference: list of REST API Routes
• Components routes
• Members routes

Working on these docs made me suggesting improvements about Components and Members REST controllers, @renatonascalves could you have a look at these?

See ​https://github.com/buddypress/bp-documentation/issues/298

Ticket: ​https://buddypress.trac.wordpress.org/ticket/9145

[imath]

Sorry I wasn't fully awake when committing [14042].

I missed rightly referencing this ticket and most importantly giving props to @espellcaste who greatly improved the PR thanks to his review.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/9145