Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST API Support #334

Closed
wants to merge 67 commits into from
Closed

REST API Support #334

wants to merge 67 commits into from

Conversation

hbarroso
Copy link

This PR will allows the Co-Authors-Plus plugin to serve REST services through the new WordPress 4.4 API feature.

Please refer to API.md file for the full documentation.

- Added API REST class
Created Class with the first route "search". Currently it only supports query param without
any exclusion. Tests were also added to verify this route.
@hbarroso hbarroso changed the title Initial Commit REST API Support Feb 16, 2016
It should only load the API if installed WP supports the new API features.
Tests were also added to cover this new feature.
@mjangda
Copy link
Member

mjangda commented Feb 17, 2016

coauthors/v1/author/{author_id}/{post_id}

Why do we include the author_id in the route rather than as a separate param? Should we have a matching GET endpoint to get authors for a post?

@hbarroso
Copy link
Author

@mjangda That is a very rough route idea of what I wanted to do when I started. But I agree, it shouldn't need the author_id. Also I think it would be a good idea to also rename that route to post/{post_id} , in which we could add, get, remove multiple users from a post as parameter of array of elements.

…e 'search' route.

Also, a test was added to cover this.
While locally this was working fine, on CircleCI tests were failing where
a mocked user should be logged out. This commit fix this.
PHP 5.3 don't support it, which was causing CI to fail.
A new route was added /post that adds or appends new coauthors.
Added new class constants and made them uppercase existing ones.
Added post_validate_callback() to validate post_id argument.
Renamed get_permission() to has_permission().
Added a new method to the current_user_can_set_authors() to check if the call is being
made from the plugin or a REST request.
Refactored some tests and added coverage for the new route.
Decouped the old API class into single classes that extend
from a parent CoAuthors_API_Controller(). The boot process
now takes place inside the php/api/boot.php file, instead of
loading it directly from CoAuthor_Plus class.
A new method was added to CoAuthors_API_Controller, that helps
filter an array of authors. Used in Search and Post API classes.

Tests were also added for the GET action.
The GET route 'args' were being declared has if they where no arguments.
Changed to accept the id.
Some parts of were also cleaned to coding guidelines
- Updated 'get_args' on CoAuthors_API_Controller so it allows to return different args
depending of which method it's uses.
- Added tests coverage
Once the guest_login field is set we don't need to updated it.
Tests were alsoo added to cover this new code.
@@ -126,6 +127,7 @@ function coauthors_plus() {
* and the custom post type to store our author data
*/
public function action_init() {
global $wp_version;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some whitespace issues here. Also, this variable is not used.

@mdawaffe
Copy link
Member

Search Endpoint:

Why is this a POST request? A sort of similar WP-API request (listing posts by an author) is a GET:
GET /wp-json/wp/v2/posts?author={author_id}

What does existing_authors mean? Don't I want to search for existing authors?

@mdawaffe
Copy link
Member

Post - GET endpoint:

Why does this require authorization? The authorship of posts is public.

@hbarroso
Copy link
Author

does_coauthor_exists() and does_coauthor_email_exists() would read better as does_coauthor_exist() and does_coauthor_email_exist().

EDIT: actually, the name of does_coauthor_email_exists() is confusing. If it's passed a coauthor object, it can return false even when the email exists :) Is there a better name that describes what the function does?

Since it's only ever called like that once, should we just not bother with the second parameter? It could return false if the email doesn't exist, or the coauthor ID of the coauthor with that email if it does exist. Then we could do that secondary check in ->put_item() itself (or some other function).

I'm not sure if that's the best solution. Do you have any another suggestions?

I like your suggestion. I think, if we used that logic in more than one place it would be useful to put it inside a new method. But using the logic inside the->put_item() is suffice IMO. I've also updated the methods names to *_exist() in 0c4599a

@mdawaffe
Copy link
Member

I've also updated the methods names to *_exist() in 0c4599a

Thanks. Is does_coauthor_email_exists() still a good name, then?

@hbarroso
Copy link
Author

Thanks. Is does_coauthor_email_exists() still a good name, then?

Yes, I think changing to something like is_email_unique() would be more meaningful now.

@mdawaffe
Copy link
Member

mdawaffe commented Apr 18, 2016

I suggest we refactor this method into two different methods. One for checking the post type and another for the actual permissions. This way the API can safely use the correct one without having to use extra parameters.

I agree, though it seems like one function to check if a user can set authors for a specific post, and a different one for if a user can set authors for posts of a specific type:
current_user_can_set_post_authors( $post_id ), which could be used almost everywhere
current_user_can_set_post_type_authors( $post_type ), which could be used in a few places.

Would that cover everything, do you think?

EDIT: but yes, a different PR makes sense.

@hbarroso
Copy link
Author

hbarroso commented Apr 18, 2016

current_user_can_set_post_authors( $post_id ), which could be used almost everywhere
current_user_can_set_post_type_authors( $post_type ), which could be used in a few places.

Would that cover everything, do you think?

I like that. But what about the part that checks for the user level?
Do mean, leave it inside the current current_user_can_set_authors as it is and use those two new methods as helpers inside as well? Or should we also refactor that code into a new method?

I'm talking about this code

        $current_user = wp_get_current_user();
        if ( ! $current_user ) {
            return false;
        }
        // Super admins can do anything
        if ( function_exists( 'is_super_admin' ) && is_super_admin() ) {
            return true;
        }

        $can_set_authors = isset( $current_user->allcaps['edit_others_posts'] ) ? $current_user->allcaps['edit_others_posts'] : false;

        return apply_filters( 'coauthors_plus_edit_authors', $can_set_authors );

This code doesn't depend on $post_id or $post_type.

@mdawaffe
Copy link
Member

I'm talking about this code

That's the code I'm talking about too. It looks as though it's supposed to take into account post-specific capabilities but doesn't.

It seems like it should be a wrapper for either current_user_can( 'edit_post', $post_id ) and/or checking for the $post_type->caps->edit_others_posts capability, but it doesn't do either :) I'm not sure what the "right" thing to do here is - thoughts? We can leave the implementation to a different PR, though.

@hbarroso
Copy link
Author

@mdawaffe Yeah, I'm not sure either. For the sake of keeping the PR more clean (regarding the API implementation) I'd suggest we refactor just that code into a different method so we don't need the $is_api_request and just use that new method. And then on a future PR we deal with this. What do you think?

@mdawaffe
Copy link
Member

That makes sense to me.

public function delete_item( WP_REST_Request $request ) {
global $coauthors_plus;

$coauthor_id = (int) sanitize_text_field( $request['id'] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably just need either (int) or sanitize_text_field here (not both)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's redundant . I'll use (int) instead.

@mattoperry
Copy link
Contributor

Hey guys -- I've had a look over this and added a couple very minor notes, but it looks generally good and I'm not sure I have a ton of additional notes beyond what @mdawaffe has added above. I think what I'd like to do here is to add this to the 3.3 milestone and test this in a couple of real-world situations ... there are also a couple of people on the VIP team I'd like to show this to.

@mattoperry mattoperry modified the milestones: 3.3, 3.2 Apr 19, 2016
The Id is already being sanitized in the sanitize_callback, and the method
just need to typecast it.
Some unused code was removed and the permission part was refactored into a new
method. This way, the current method doesn't need the $is_api_request parameter.

The API code was updated to use this method as well.
@djouonang
Copy link

Hello I have been trying to use the wp rest api to pull data from local site to live wordpress site with use of shortcode. When i try to do the reverse that is to pull posts from live to local it works but when i change links to retrieve from local to live it does not display anything on the page. I already have the wp rest api installed on both sites. Below is my code:

function my_recent_posts_shortcode($atts){

$response = wp_remote_get( 'http://localhost/wordpress/wp-json/wp/v2/posts' );

if( is_wp_error( $response ) ) {

return;

}

$posts = json_decode( wp_remote_retrieve_body( $response ) );

if( empty( $posts ) ) {

return;

}

if( !empty( $posts ) ) {

$list = '';

foreach( $posts as $post ) {

$list .='

link. '">' . $post->title->rendered . '
';
}

return $list . '';

}

}

add_shortcode('recent-posts', 'my_recent_posts_shortcode');

@philipjohn philipjohn modified the milestone: 3.3 Dec 3, 2016
@misfist
Copy link

misfist commented Jan 23, 2017

I'm really confused about what the status of this is. The REST API code isn't in the being discussed here doesn't seem to be in any existing branch. Could someone shed some light?

@misfist
Copy link

misfist commented Feb 23, 2017

Now that the REST API is in core, this has become even more important. Is there any update on this?

@emhoracek
Copy link

@mikeselander
Copy link

mikeselander commented Jun 2, 2017

@philipjohn @mjangda what's the plan for this PR and functionality? We have a client need for REST endpoints for coauthors plus and while this doesn't meet our need exactly, I'm curious what considerations are being made for coauthors plus usage in the REST API. Specifically related to returning post results via the REST API core endpoints.

@philipjohn
Copy link
Contributor

This isn't a priority right now, but we will have a bunch of work starting on the plugin soon so might be able to get this looked at as part of that.

@misfist
Copy link

misfist commented Jun 17, 2017

@philipjohn - How/why is making Co-authors Plus work with the REST API, which is now in core not a priority?

@philipjohn
Copy link
Contributor

@misfist Having API endpoints would definitely be nice, but it's not crucial to the functioning of this plugin which would work just as well without them. There are benefits to having these API endpoints, but the priority is fixing existing bugs and other more frequently requested enhancements.

@rebeccahum rebeccahum closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants