BP REST API - V2 #337

renatonascalves · 2024-07-15T13:25:01Z

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

This Pull Request is for code review only. Please keep all other discussion in the BuddyPress Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the WordPress Core Handbook for more details.

renatonascalves · 2024-07-17T02:39:04Z

src/bp-blogs/bp-blogs-template.php

-		if ( ! $blogs_template && isset( $args['blog_id'] ) && $args['blog_id'] ) {
+		$has_current_blog = isset( $blogs_template ) && isset( $blogs_template->blog->blog_id );
+
+		if ( false === $has_current_blog && isset( $args['blog_id'] ) && $args['blog_id'] ) {


IMO, we should respect if the blog_id is passed, regardless if there is a blog in the current loop. But, I assume this logic is here for a reason, so let's keep it.

But technically, it's not possible to pass another, different blog_id into bp_get_blog_avatar inside a blog loop because of this logic.

I agree with you, maybe we can fix it later in another ticket?

Created: https://buddypress.trac.wordpress.org/ticket/9228

imath

Hi @espellcaste

Thanks a lot for your work on this.

I was very surprised by the inclusion of V1 into class names: what is the purpose of bp_rest_version() then?
Shouldn't wp-json/buddypress/v1 results in loading classes for the version 1 of the BP REST API, wp-json/buddypress/v2 results in loading classes for the version 2 of the BP REST API, and so on?
What happens to plugins using directly current BP REST API classes? If you change their prefix in favor of BP_REST_V1, I'm afraid we would end up with fatal errors.
According to this comment about the WP API feature as a plugin, we should keep BP_REST_ class prefix for v1.

I strongly advise you to keep class names the way they currently are: do not use BP_REST_V1 prefixes. We need to preserve Plugin backwards compatibility.

src/bp-core/bp-core-rest-api.php

src/bp-core/classes/class-bp-component.php

src/bp-core/deprecated/15.0.php

renatonascalves · 2024-08-23T00:54:34Z

This is almost ready.

renatonascalves · 2024-08-24T01:29:49Z

This pull request is for code review. 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

Hi @renatonascalves

Thanks a lot for your work on this. First I'm sorry I'm hesitating a lot about the best way to deprecate v1.

As I said in one of my review comments and considering the one you made on the Notices PR, I believe we should make sure to inform developers if:

they are using directory v1 classes without having the BP REST plugin active => _deprecated_class() see the comment I made about class-buddypress.php
they are including v1 files inside BuddyPress => _deprecated_file() see the comment I made about bp-activity/classes/class-bp-rest-activity-endpoint.php

This means renaming all v2 files and classes so that they match the autoloader regular map. Eg: class BP_Activity_REST_Controller=> class-bp-activity-rest-controller.php` (like you previously did I believe - sorry for changing my mind!!). As I've tested it, you'll find below the diff I added to my local test of your PR below (if it can save you some time).
337.unfinished.patch

The Manage Group Members UI using the REST API works as expected after the change. The buddypress/v1 deprecated notice works as expected 👏 .

On a site note, once REST API v2 will be SVN committed, we'll need to update all BP Blocks to use the new rest version as buddypress/v1 is hardcoded into these.

imath · 2024-09-03T17:05:56Z

src/bp-core/bp-core-rest-api.php

+		)
+	);
+}
+add_action( 'bp_init', 'bp_rest_api_register_request_script' );


According to our discussion about it, we shouldn't register this script anymore

Suggested change

add_action( 'bp_init', 'bp_rest_api_register_request_script' );

Right! I was going to apply this change in a second pr, separate ticket.

imath · 2024-09-03T17:07:33Z

src/bp-core/bp-core-rest-api.php

+/**
+ * Register the jQuery.ajax wrapper for BP REST API requests.
+ *
+ * @since 5.0.0
+ * @deprecated 10.0.0
+ */
+function bp_rest_api_register_request_script() {
+	if ( ! bp_rest_api_is_available() ) {
+		return;
+	}
+
+	wp_register_script(
+		'bp-api-request',
+		sprintf( '%1$sbp-core/js/bp-api-request%2$s.js', buddypress()->plugin_url, bp_core_get_minified_asset_suffix() ),
+		array( 'jquery', 'wp-api-request' ),
+		bp_get_version(),
+		true
+	);
+
+	wp_localize_script(
+		'bp-api-request',
+		'bpApiSettings',
+		array(
+			'unexpectedError'   => __( 'An unexpected error occurred. Please try again.', 'buddypress' ),
+			'deprecatedWarning' => __( 'The bp.apiRequest function is deprecated since BuddyPress 10.0.0, please use wp.apiRequest instead.', 'buddypress' ),
+		)
+	);
+}


This part should be moved into:
src/bp-core/deprecated/10.0.php

I was going to apply this change in a second pr, separate ticket.

src/class-buddypress.php

src/bp-activity/classes/class-bp-rest-activity-endpoint.php

imath · 2024-09-03T21:00:02Z

src/bp-blogs/bp-blogs-template.php

-		if ( ! $blogs_template && isset( $args['blog_id'] ) && $args['blog_id'] ) {
+		$has_current_blog = isset( $blogs_template ) && isset( $blogs_template->blog->blog_id );
+
+		if ( false === $has_current_blog && isset( $args['blog_id'] ) && $args['blog_id'] ) {


I agree with you, maybe we can fix it later in another ticket?

src/bp-core/bp-core-functions.php

imath · 2024-09-03T21:05:32Z

src/bp-core/bp-core-rest-api.php

+function bp_rest_api_v1_dispatch_error( $result, $server, $request ) {
+
+	// Bail early if the BP REST plugin is active.
+	if ( bp_rest_is_plugin_active() ) {
+		return $result;
+	}
+
+	$route = $request->get_route();
+
+	if ( empty( $route ) || ! str_contains( $route, 'buddypress/v1' ) ) {
+		return $result;
+	}
+
+	return new WP_Error(
+		'rest_no_route',
+		__( 'The V1 of the BuddyPress REST API is no longer supported, use the V2 instead.', 'buddypress' ),
+		array( 'status' => 404 )
+	);
+}
+add_filter( 'rest_pre_dispatch', 'bp_rest_api_v1_dispatch_error', 10, 3 );


Excellent work here 💪 ! I confirm it behaves as expected.

renatonascalves · 2024-09-04T01:53:35Z

@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 same class twice, since both plugins would be loading the same file. 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.

Co-authored-by: imath <[email protected]>

imath · 2024-09-05T22:18:20Z

Hi @renatonascalves about the 3 clarifying questions:

If _deprecated_class is not the right function to use, I'm fine with a _doing_it_wrong() informing to use v2 controllers instead.
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.
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" 😅

renatonascalves · 2024-09-06T03:43:56Z

@imath

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

imath · 2024-09-07T00:41:26Z

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.

renatonascalves · 2024-09-23T14:08:35Z

@imath

Removed V1 class names from the autoloader irregular map (the BP REST plugin are loading them now);
V2 classes uses the autoloader, as expected;
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: buddypress/BP-REST#512

:)

imath

Hi @renatonascalves congrats for the achieved works! Amazing job.

I've added a minor suggestion about translators inline comment. Maybe we should rename PHPUnit REST testcases / files to follow the controller switch, eg: BP_Test_REST_Group_Endpoint => BP_Test_Group_REST_Controller & test-endpoint.php => test-controller.php?

Feel free to SVN Commit!
NB: As you're adding a lot of files, don't forget to svn stat and eventually svn add new files before committing.

💚 💪

src/bp-core/classes/trait-attachments.php

renatonascalves · 2024-09-26T03:05:15Z

@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!

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 buddypress/buddypress#337 git-svn-id: http://buddypress.svn.wordpress.org/trunk@14026 cdf35c40-ae34-48e0-9cc9-0c9da1808c22

Moving BP REST API classes into BP core

47c7afa

renatonascalves self-assigned this Jul 15, 2024

renatonascalves added 2 commits July 15, 2024 20:14

Adjust blog

7cfe398

Make phpunit happy

67a2746

renatonascalves commented Jul 17, 2024

View reviewed changes

Address deprecated feature

5a8e7d3

renatonascalves marked this pull request as ready for review July 17, 2024 03:15

renatonascalves added 2 commits July 18, 2024 00:06

Remove references to the BP REST API Plugin

6de6733

Use correct subpackage

f3448ab

imath suggested changes Jul 22, 2024

View reviewed changes

src/bp-core/bp-core-rest-api.php Outdated Show resolved Hide resolved

src/bp-core/classes/class-bp-component.php Outdated Show resolved Hide resolved

src/bp-core/deprecated/15.0.php Outdated Show resolved Hide resolved

renatonascalves changed the title ~~Moving BP REST API classes into BP core~~ BP REST API - V2 Jul 27, 2024

renatonascalves added 8 commits July 27, 2024 17:23

Sync

0221875

Rename files

68ccb62

More changes to V2

7098ded

Adjust unit tests

a821e75

More fixes

0b1a600

Merge branch 'master' into feature/9145

81a8828

unit test fixes

81163bf

Misc changes

f8914d7

renatonascalves added 2 commits August 23, 2024 20:33

Merge branch 'master' into feature/9145

d3dce4d

Minor tweaks

f59e7a6

renatonascalves mentioned this pull request Aug 24, 2024

Setting the plugin to V1 buddypress/BP-REST#512

Merged

Make the unit tests happy

7e002b4

Adding error message to v1 requests

1ac3c0a

imath reviewed Sep 3, 2024

View reviewed changes

renatonascalves and others added 2 commits September 3, 2024 22:58

Update src/bp-core/bp-core-functions.php

7d04933

Co-authored-by: imath <[email protected]>

Merge branch 'master' into feature/9145

5c8acbb

Adding CR suggestion(s)

561d318

Merge branch 'master' into feature/9145

5288203

imath approved these changes Sep 26, 2024

View reviewed changes

src/bp-core/classes/trait-attachments.php Outdated Show resolved Hide resolved

renatonascalves added 5 commits September 26, 2024 11:29

Merge branch 'master' into feature/9145

a616cb7

Adjusting unit tests based on new feedback

86d4f3c

Make sure the MU unit tests pass

d5844f4

Try to fix a flaky unit test

f44b409

Fix another flaky test

8fcca6a

boonebgorges closed this in 41802f7 Sep 27, 2024

renatonascalves deleted the feature/9145 branch September 28, 2024 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BP REST API - V2 #337

BP REST API - V2 #337

renatonascalves commented Jul 15, 2024

renatonascalves Jul 17, 2024

imath Sep 3, 2024

renatonascalves Sep 4, 2024

imath left a comment

renatonascalves commented Aug 23, 2024

renatonascalves commented Aug 24, 2024 •

edited

Loading

imath left a comment

imath Sep 3, 2024

renatonascalves Sep 4, 2024

imath Sep 3, 2024

renatonascalves Sep 4, 2024

imath Sep 3, 2024

imath Sep 3, 2024

renatonascalves commented Sep 4, 2024 •

edited

Loading

imath commented Sep 5, 2024

renatonascalves commented Sep 6, 2024

imath commented Sep 7, 2024

renatonascalves commented Sep 23, 2024 •

edited

Loading

imath left a comment

renatonascalves commented Sep 26, 2024

BP REST API - V2 #337

BP REST API - V2 #337

Conversation

renatonascalves commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imath left a comment

Choose a reason for hiding this comment

renatonascalves commented Aug 23, 2024

renatonascalves commented Aug 24, 2024 • edited Loading

imath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatonascalves commented Sep 4, 2024 • edited Loading

imath commented Sep 5, 2024

renatonascalves commented Sep 6, 2024

imath commented Sep 7, 2024

renatonascalves commented Sep 23, 2024 • edited Loading

imath left a comment

Choose a reason for hiding this comment

renatonascalves commented Sep 26, 2024

renatonascalves commented Aug 24, 2024 •

edited

Loading

renatonascalves commented Sep 4, 2024 •

edited

Loading

renatonascalves commented Sep 23, 2024 •

edited

Loading