REST API for annotations (back-end comments, take 2) #4685

jaswrks · 2018-01-26T00:32:24Z

Description

Alternative approaches explored: #4385 and #4386.

A WP REST API endpoint for annotations (back-end comments). Implemented as a custom comment type. The API uses the W3C annotation data model for annotation selectors and maintains some parity with the W3C annotation object model and protocol — while still doing things the WordPress way, which maximizes compatibility with WP REST API utilities, including those in JavaScript.

Overview of Changes

Introduces:

lib/class-wp-annotation-caps.php to deal with annotation capabilities.
lib/class-wp-rest-annotations-controller.php is the REST API controller.
lib/class-wp-annotation-utils.php are a few annotation utilities.

Modifies:

lib/load.php
lib/register.php

Near 100% coverage by unit tests

see: phpunit/annotations/*

API Documentation

See: 📄 https://speca.io/jaswrks/wp-annotations

How can you test the WP REST API for Annotations?

Install the Basic Auth plugin for the WP REST API. This allows you to run tests with only a username/password via Postman.
- Or, simply create this directory and file, which forces the current user to 1 while testing. If you go this route, be sure to delete the file after testing.
  wp-content/mu-plugins/rest-api-auth-bypass.php
```
  <?php
  add_action( 'rest_api_init', function() {
      wp_set_current_user( 1 ); // Assuming admin is user ID 1.
      // It's worth testing other roles too; e.g., editor, author, contributor, subscriber.
  } );
```
Now, visit this page and click "Run in Postman". You can then run your own tests locally in Postman. Either that, or follow along with the Annotation API docs and run your own tests over HTTP in any other way you'd like.

How Has This Been Tested?

Postman (as described above)
Several unit tests are included with this PR.

Types of changes

This approach uses a custom comment type instead of a custom post type.
This PR strips away support for front-end annotations. Front-end annotations were explored in a prior PR for the REST API, but not a concern at this time.

This approach requires a comment query filter to keep annotations out of the comments admin area, and away from plugins that might mistake them as being a more typical comment type. In this PR, the following alters comments_clauses in core.

/**
 * Filters WP_Comment_Query clauses.
 *
 * @param  array            $pieces Array of comment query clauses.
 * @param  WP_Comment_Query $query  Current WP_Comment_Query instance.
 *
 * @return array                    Array of comment query clauses.
 */
public static function on_comments_clauses( $pieces, $query ) {
	global $wpdb;

	/*
	 * When cache_domain is 'annotations', only then will annotations be returned by WP_Comment_Query.
	 * Otherwise, if cache_domain is not 'annotations', annotation comment types cannot be returned whatsoever.
	 *
	 * The point being that we want to keep annotations out of any normal comment query performed by core,
	 * and also keep them away from comment-related plugins; i.e., annotations will be unexpected by most plugins.
	 * If a plugin *does* want to query annotations, they should set cache_domain to 'annotations'.
	 */
	if ( 'annotations' !== $query->query_vars['cache_domain'] ) {
		$annotation_types = self::$types;

		foreach ( $annotation_types as &$_type ) {
			$_type = $wpdb->prepare( '%s', $_type );
		}
		$pieces['where'] .= $pieces['where'] ? ' AND ' : '';
		$pieces['where'] .= 'comment_type NOT IN (' . implode( ', ', $annotation_types ) . ')';
	}

	return $pieces;
}

Back-End Annotation Permissions

Simplified Explanation

If you can edit_posts, and you can edit_post (this post). Or, if you're the post author. Then you can read all, and create, edit, delete your own back-end annotations in this post.
Administrators and Editors can edit and delete any post, so they can read, edit, and delete any annotation without restriction.
Subscribers and the public have no access to back-end annotations whatsoever.

TODO

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.
Expand unit test coverage.
Fix bugs found by @chriskmnds. Noted in comments.

Non-Blocking TODOs

Refactor WP_Annotation_Utils::on_map_meta_cap() — move logic in each 'case' to a separate function, making a somewhat complex routine easier to follow along with.
Polish unit tests; e.g., use test generators.
Move capability-related methods to their own class.
Remove useless comments in caps class.
Cleanup comments in utils class.
Add tests that check valid/invalid UTF-8 sequences.
Add tests that consider unfiltered HTML.
fix: Check empty content before & after filters.
Add tests for annotations in trashed posts.
Add tests against WP_Comment_Query filters.
Test setting all comment statuses.
Cover all selector types in unit tests.
Test cap checks with missing/invalid args.
Add tests that expand coverage in REST controller.
Add tests that expand coverage in utils.
Fill in any remaining test coverage gaps.

Document code coverage.

@WP.Annotation::WP_Annotation_Caps
  Methods:  71.43% ( 5/ 7)   Lines:  96.58% (113/117)
@WP.Annotation::WP_Annotation_Utils
  Methods:  66.67% (10/15)   Lines:  97.71% (256/262)
@WP.REST.Annotations::WP_REST_Annotations_Controller
  Methods:  85.00% (17/20)   Lines:  98.64% (364/369)

Try simplifying cap handlers, REST API for annotations (back-end comments, take 2) #4685 (comment)
Document args to cap handlers, REST API for annotations (back-end comments, take 2) #4685 (comment)
Comment on status considerations in cap handlers, REST API for annotations (back-end comments, take 2) #4685 (comment)
~~Consider typehinting WP_REST_Request, REST API for annotations (back-end comments, take 2) #4685 (comment)~~
Replace @since [version] with version in docBlocks.

Client Side Exploration of Selectors

jsFiddle: Building and highlighting a W3C Annotation Selector using XPath.
https://jsfiddle.net/jaswrks/1cLaeyfd/

chriskmnds · 2018-01-27T19:00:57Z

lib/class-wp-rest-annotations-controller.php

@@ -0,0 +1,1065 @@
+<?php


Hey Jason, isn't there a way to break down this humongous file into smaller modules? There is so much going on in here, it will be hard to maintain imo. Same as the utils class above. The separation of what is covered by each is not clear to me either. Maybe start by extracting the permission checks for a start, then gradually organise things maybe per CRUD operation., identify reusable patterns, etc.

This is obviously a big task, and by all means kudos for writing so much code.

@chriskmnds Thanks for the review 😄

lib/class-wp-rest-annotations-controller.php
break down this humongous file into smaller modules?

WP_REST_Annotations_Controller extends WP_REST_Comments_Controller in core. There's a nice article in the handbook that summarizes the infrastructure.

While I'm not opposed to separating areas of concern, I think it's important to follow the precedent that's been set and to maintain a consistent pattern.

class-wp-annotation-utils.php
extracting the permission checks for a start

I agree. Pulling the permissions out of there seems like a great idea.

chriskmnds · 2018-01-27T19:24:18Z

phpunit/class-rest-annotations-controller-test.php

@@ -0,0 +1,1244 @@
+<?php


I haven't gone through all the tests here, but I am wondering if you have created test cases to handle all the parameter values and types that are passed to the API. For example, do you check the expected return values when a required parameter is missing, when a wrong value is passed in the request, whether errors are communicated correctly, etc? For example, I get back a status 200 OK with a value of Unexpected '<' in the body of the response when I attempt to create an annotation with the parameters:

{ "post": 1, "via": "gutenberg", "content": " ", "author": "123" }

Is this normal, am I doing something wrong? Obviously author: "123" erred there. I believe a strong enumeration of all the expected values to/from the API would give a lot of value to this branch. Again, I am not sure if you are already doing that and there are a few edge cases that slipped through.

Similarly, I notice that passing an empty string for content content: "" results in a 400 Bad Request, while if I pass just a space with content: " " results in 200 OK. I can see in the response though that the rendered content is null/empty "". Why allow that then and not treat it like the former case? etc..

@chriskmnds Awesome work, thank you! 👁
It's great to have another pair of eyes on this.

test cases to handle all the parameter values and types that are passed to the API.

Yes, there were some like this already, but most had a focus on user permissions. I've been in the process of restructuring the unit tests for this branch and that work is done now. I just pushed the improved unit tests to this PR and I'll post a summary shortly. Always room for improvement though, so if you find anything else just let me know.

Both great finds, and both bugs. Nice work!

🐛 "Unexpected '<' in the body of the response"... Triggered by a nonexistentauthor ID. This was resulting in an internal WP_Error response that went unhandled, resulting in a parse error and a malformed response instead of the error message and code. Fixed by adding these three lines and unit tests now cover this.

🐛 "if I pass just a space with content: " " results in 200 OK." ... Yeah, I think it should be smarter than this. The same behavior seems to exist in WordPress core. I corrected this for annotations by adding a smarter analysis of the content and unit tests also. I'm considering a trac ticket for this in core.

Awesome, thanks for the follow up. My point was really just to make sure you solidify everything that is expected to/from the endpoints. I will try and go through the specs later this week and see if I spot anything else. Cool stuff :-)

Thanks again Chris. I really appreciate you taking the time to look over this with me. Anything I can help you with just hit me on Slack! :-)

No problem! and thanks, I will definitely ping you if/when needed :-)

jaswrks · 2018-01-29T11:38:29Z

Unit tests for this API have been restructured to improve their organization, readability, and to expand coverage. Tests now run approx 45k assertions, with new tests having been added to check things like invalid parameter responses, max string lengths, and more CRUD permutations.

Four Sets of Tests

class-annotation-caps-test.php Tests annotation capabilities.
class-annotation-queries-test.php Tests annotation queries.
class-annotation-utils-test.php Tests utilities.
class-rest-annotations-controller-test.php Tests the REST API controller.

jaswrks · 2018-01-29T11:38:36Z

🐛 Bug Fixes

Author ID was not being compared as an array in the REST API controller when defining the $is_current_user_query variable. Making it impossible to filter down to only your own annotations. Fixed in this line and now covered by unit tests.
A whitespace-only content string was allowed to pass through when creating or updating a comment. Props @chriskmnds, see: REST API for annotations (back-end comments, take 2) #4685 (comment), and this bug also exists in core: https://core.trac.wordpress.org/ticket/43177#ticket
Submitting a POST request to create a new annotation by a nonexistent author resulted in an error internally, but with a broken response. Props @chriskmnds, see: REST API for annotations (back-end comments, take 2) #4685 (comment)
Trashed annotations having a _wp_trash_meta_status that would restore it as public, were wrongly considered readable by other authors in some cases. Fixed in lines like this one and tested.
A foreach was missing its $prop (and instead, inheriting $prop from a previous loop). This was causing wrong and inconsistent behavior in some tests. Fixed and now covered by unit tests.
W3C Selectors were checked by a yoda condition in the wrong direction, which caused some valid selectors to fail validation when they were actually fine. Now covered by unit tests.
The existing Admin_Test was broken when running alongside those for annotations. The wpSetUpBeforeClass method should be used instead of setUpBeforeClass. I fixed it in this branch so tests will run properly, and opened a separate PR to correct this also.

jaswrks · 2018-01-30T17:44:33Z

Fresh rebase on master branch and I updated links to commits in my earlier comments.

atimmer

I've looked at the first part of the code changes and had these remarks.

My main feeling when I look at this PR is that its a ton of changes all at once. I think it could be a lot cleaner if WordPress had first-class support for comment types and private comments.

I am going to look into making this possible in core.

atimmer · 2018-01-31T15:40:42Z

lib/class-wp-annotation-caps.php

+/**
+ * Annotations API: WP_Annotation_Caps class
+ *
+ * @package gutenberg


I think these can have capital letters, so Gutenberg is probably more appropriate.

A capital seems right to me too. The rest of the PHP files in Gutenberg use lowercase though, so if this changes it should probably happen in one fell swoop as a separate PR. Do you agree?

Ah, you did it for consistency. That makes sense. I would leave it like this then. And indeed if we want to change it, it needs to happen in one rename PR.

atimmer · 2018-01-31T15:42:35Z

lib/class-wp-annotation-caps.php

+	 * @param array  $caps    Required capabilities.
+	 * @param string $cap     Capability to check/map.
+	 * @param int    $user_id User ID (empty if not logged in).
+	 * @param array  $args    Arguments to {@see map_meta_cap()}.


You should describe here what this function can have as $args.

Oh, yes. That's a good idea, ty.

atimmer · 2018-01-31T15:42:40Z

lib/class-wp-annotation-caps.php

+	 * @param array  $caps    Required capabilities.
+	 * @param string $cap     Capability to check/map.
+	 * @param int    $user_id User ID (empty if not logged in).
+	 * @param array  $args    Arguments to {@see map_meta_cap()}.


You should describe here what this function can have as $args.

Since this function forks itself out to the other cap-specific handlers, I documented the arguments separately in each one to help clarify. Thanks again!

atimmer · 2018-01-31T15:55:32Z

lib/class-wp-annotation-caps.php

+			$wp_trash_meta_status = $wp_trash_meta_status ? $wp_trash_meta_status : '0';
+			$comment_status       = WP_Annotation_Utils::translate_comment_status( $wp_trash_meta_status );
+		}
+		if ( ( 'trash' === $actual_comment_status && ( ! $user_id || (int) $comment->user_id !== $user_id ) )


This if statement requires a bit more docs I think.

Agree, thanks.

atimmer · 2018-01-31T16:00:19Z

lib/class-wp-annotation-caps.php

+ *
+ * @since [version]
+ */
+final class WP_Annotation_Caps {


There is a lot of duplication in these files, especially with the array_merge's at the end. Maybe you can find a way to resolve this?

Great observation, thank you. I was thinking the same thing when I wrote this and tried to avoid some of that with the get_comment_info() and get_post_info() utils. But the extraction of those variables are now in duplicate and I'd like to improve. Really want to avoid the use of extract(). I'll dig in and see if there's anything I can do.

I did some work on this in recent commits. Thanks!

atimmer · 2018-01-31T16:02:29Z

lib/class-wp-annotation-caps.php

+	 *
+	 * @return array          Array of required capabilities.
+	 */
+	public static function on_map_meta_cap( $caps, $cap, $user_id, $args ) {


Making all these methods static makes this class more like a pseudo-namespace than an actual object. What's wrong with instantiating and calling the instance method in the filter?

Yeah, singleton vs. static. I don't have a strong personal preference between them. There was nothing that needed instantiation in these, so I went with static as one step above global functions. If there are strong opinions about it I'm happy to change.

atimmer · 2018-01-31T16:09:16Z

lib/class-wp-annotation-utils.php

+	 * @see WP_Annotation_Utils::on_comments_clauses()
+	 * @see WP_REST_Comments_Controller::get_items()
+	 */
+	public static function on_rest_comment_query( $query_vars, $request ) {


You can type-hint the $request variable using WP_Request $request in the list of params. This works in PHP5.2.

Cool, yes. I didn't realize class hints went all the way back to 5.0.

I'm totally in favor of type hints anywhere they're supported, on just about everything. And I got excited when you mentioned that some work all the way back to 5.x. Woohoo! However, since this is not a practice that WordPress has adopted, and there's no mention of hints in the coding standards, I'm going to hold off for now and ask around. Maybe it's fine, I just haven't seen them used in core, have you?

I asked about this on Slack and the impression I have is that it's probably OK to use type hints, but there needs to be some further discussion and a precedent set. Ideally, there would be something added to the WP code standards on this topic before we start using them. https://wordpress.slack.com/archives/C02RQBWTW/p1517600728000053

Don't worry about creating that Trac ticket @jaswrks I mentioned, this PR will suffice for said discussion 💯

atimmer · 2018-01-31T16:12:36Z

lib/class-wp-annotation-utils.php

+
+		$meta_queries = array();
+
+		$vias = $request['via'];


I prefer to use get_param because there is less 'magic'.

For the sake of consistency, array access matches controllers in core; e.g., https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php

jaswrks · 2018-02-02T18:28:05Z

Props @atimmer and @chriskmnds for your reviews. I'm attaching a summary of recent improvements following your suggestions. If you have time to take another look and find any other issues please let know. Thank you both 😄

Summary of recent commits:

Meta-cap handlers moved to their own class; props to @chriskmnds.
Unit tests for meta-caps moved to their own class.
Unit tests reorganized. They now live in phpunit/annotations/*.

Many more unit tests and a coverage analysis (near 100% line coverage).

@WP.Annotation::WP_Annotation_Caps
  Methods:  71.43% ( 5/ 7)   Lines:  96.58% (113/117)
@WP.Annotation::WP_Annotation_Utils
  Methods:  66.67% (10/15)   Lines:  97.71% (256/262)
@WP.REST.Annotations::WP_REST_Annotations_Controller
  Methods:  85.00% (17/20)   Lines:  98.64% (364/369)

Further simplified the meta-cap handlers; props to @atimmer.
Found one more bug. It was related to empty content checks whenever a user without the unfiltered_html cap created or updated an annotation. It was possible to submit a comment containing HTML tags not in the whitelist (which get stripped away) potentially leaving empty content after content filters were applied. Fixed in c62eb64.
I left inline comments on some of the more complex conditionals.
I docBlock'd the $args to each meta-cap handler; props to @atimmer.
Updated leading topic in this PR with an overview of files it contains, and there's a checklist of my recent work up there also if it helps.

jaswrks · 2018-02-03T06:07:09Z

Referencing a trac ticket I was previously unaware of:
40031: Consider Adding Web Annotations to WordPress

I invited others from that discussion to leave feedback here and I'm looking forward to other opinions and ideas. My comment here: https://core.trac.wordpress.org/ticket/40031#comment:24

jaswrks · 2018-02-03T06:50:28Z

@atimmer writes...

My main feeling when I look at this PR is that its a ton of changes all at once. I think it could be a lot cleaner if WordPress had first-class support for comment types and private comments.

I agree. As some of you may already know, I went back and forth on this and even completed a large chunk of work in two previous iterations of this API. My first attempt was with a custom post type, then with both front-end and back-end annotations as two different comment types.

I left my thoughts and a list of pros/cons in each of those previous iterations.

Then I honed in on private backend annotations only, using this PR with a single custom comment type. Only private backend annotations right now, because that's the current Gutenberg objective, but also because I concluded a few things from my experimentation.

Front-End Annotation Conclusions

If front-end annotations ever do become a thing, I think they'd be better implemented in a way that would simply extend the existing default comment type. In other words, WP core comments could simply be enhanced; allowing them to have annotation-like properties. We don't even need a custom comment type for this, it could just be that comments are allowed to have Selectors like those implemented by this PR, which is what makes a comment an annotation.

That way front-end comments and front-end annotations are both the same thing. The only real
difference being that an annotation has a selector; i.e., annotates a specific part of the document. If it doesn't (or is later orphaned) it simply joins or rejoins the flow of comments and is therefore treated like any other front-end comment, because it doesn't select anything.

Back-End Annotation Conclusions

That's the focus of this PR.

What would have made this work 1000% easier, lighter, and more tightly integrated with core, is if custom comment types were a first-class citizen in core. If core had a register_comment_type() and register_comment_status() system in place, much of the code for this API and the capabilities associated with it could have been avoided. Alas, we're not there yet, and until we are, the API introduced by this PR seems like the best option to me.

My hope is that this work will:

Satisfy requirements for this task I was given in the best way possible at this time.
Serve as inspiration to myself or others when considering annotations in core.
Demonstrate the need for more work to be done on custom comment types in core.

jaswrks · 2018-02-04T00:57:55Z

Referencing existing trac ticket for custom comment types: https://core.trac.wordpress.org/ticket/35214

atimmer

I have done a more thorough review of the PR. The main changes I would like to see are:

The annotation status should be in comment meta. I think saving the status in comment_approved is an unwise decision.
Annotations should have an annotation_type field (not mentioned in other code review comments). The default should be "comment". Grammatical suggestions or SEO suggestions are of a different subtype. We (@omarreiss and I) think we should have built-in support for different types of annotations from the start.

atimmer · 2018-02-07T12:34:15Z

lib/class-wp-annotation-utils.php

+	 *
+	 * @return stdClass|null           Data object. Null on failure.
+	 */
+	public static function get_comment_data( $comment ) {


You should consider returning a value object here. This looks a whole lot like a WP_Annotation object.

Good suggestion. I don't see any reason we can't have a WP_Annotation class, and I agree it would simplify things quite a bit.

atimmer · 2018-02-07T12:43:45Z

lib/class-wp-annotation-utils.php

+		global $wpdb;
+
+		if ( ! $comment ) {
+			if ( $error ) {


Why this $error check? This is never done in WordPress core as far as I am aware. Why not just return a WP_Error?

I see what you mean. I'm not sure I would have written it this way either, were it not already like this in wp_set_comment_status(). My intent was mainly to model this function after wp_set_comment_status() and add support for custom statuses.

That said, in the end, that $error parameter has not been needed by any code in this PR. So I don't see any reason to keep this either.

atimmer · 2018-02-07T12:55:10Z

lib/class-wp-annotation-utils.php

+		}
+
+		// @codeCoverageIgnoreStart
+		// Testing would require a DB write failure.


I do not think this is a good reason to ignore coverage. I know that WordPress is very hard to test because of tight coupling, but when you ignore the code coverage like this you are hiding this problem from daylight.

A very simple (and pragmatic!) solution to this is:

Inject $wpdb through the constructor. (I would argue all new classes should do this)

Mock $wpdb in your tests where necessary.

Good points, thank you. There were three or four sections like this I was unable to cover. I'll start by just removing those comments and then reassess once the next iteration is ready for review. The idea that it's "not possible" is presumptuous.

atimmer · 2018-02-07T12:56:16Z

lib/class-wp-annotation-utils.php

+		if ( $meta_queries ) {
+			if ( ! empty( $query_vars['meta_query'] ) ) {
+				// @codeCoverageIgnoreStart
+				// Testing would require a plugin modifying query.


Like above, this is not a very good reason. The inputs to this function can be changed to be able to test this code path.

#4685 (comment)

atimmer · 2018-02-07T12:58:19Z

lib/class-wp-annotation-utils.php

+		 *
+		 * @param array Comment types allowed for annotations.
+		 */
+		$allow_types = apply_filters( 'annotation_allow_types', self::$types );


This filter will never work because adding a type here will not show up in the query when querying comments.

Hmm. I'm not sure I follow on querying. Maybe if you explain further.

That said, I do agree that this filter should change.

At present, multiple annotation comment types are supported in all areas of this PR. The default annotation comment type is annotation. If this filter were moved and renamed it would more accurately describe the intention, which is to allow for additional annotation types added by plugins.

I mean that this filter should be applied in the on_comments_clauses method. Otherwise, a type added by this filter will be allowed, but will also be shown in default comment overview. Such as the front-end.

Right, I see what you meant now. That will get fixed when I move the filter and change the name. Definitely agree.

atimmer · 2018-02-08T10:52:26Z

lib/class-wp-rest-annotations-controller.php

+		// @codeCoverageIgnoreStart
+		// Not possible to test at this time.
+		// Strengthens security in custom post types and on sites with permission mods.
+		if ( ! current_user_can( 'edit_annotations', $type ) ) {


Why is a user that is not allowed to edit_annotations not allowed to create an annotation with a certain date/meta? I think this warrants a comment about the why.

Great suggestion. I'll elaborate on this in the comments. The rationale for this is not great at the moment, as it is currently impossible to create_annotations if you can't edit_annotations.

I left this in only because it would prevent date/meta from being set in the case of a custom post type or plugin altering permissions in such a way that it allows annotation creation, but locks editing. Those two fields would then be restricted in such a case, which seems desirable in that scenario.

I'm going review this again though, because it is very confusing and way out on the edge. I'd prefer to get rid of it in favor of a more straightforward approach.

Hmm, but I would argue that you should be allowed to at least set meta on an annotation if you are not allowed to edit it. For example, in the scenario where we would persist annotations from Yoast SEO, we would like to save some extra data in meta. But then users who can only create annotations (not edit) would not be able to set this extra data.

Good point. I have no problem loosening permissions here by removing meta from the list.

A scenario where a user can create, but not edit, is not one that would occur unless permissions were altered by a plugin. I will leave a portion of this in to protect the date and date_gmt fields in that scenario, but I'll remove meta as you point out.

atimmer · 2018-02-08T10:53:50Z

lib/class-wp-rest-annotations-controller.php

+			}
+		} // @codeCoverageIgnoreEnd
+
+		if ( ! current_user_can( 'edit_others_annotations', $type ) ) {


This is more obvious to me 👍 . Maybe it also should have a comment describing the implication if this check wasn't there. Something like:

// Without this check, users would essentially be able to impersonate other authors by modifying this field.

atimmer · 2018-02-08T10:57:52Z

lib/class-wp-rest-annotations-controller.php

+
+		if ( ! $post_ids ) {
+			return new WP_Error( 'rest_missing_annotation_posts', __( 'Missing post(s).', 'gutenberg' ), array(
+				'status' => 400,


I don't know how this is implemented in the REST API code, but isn't there a way to have a validation step? 400 status codes don't belong in a permissions check.

I agree with 400 error codes being out of place here.

The REST API is rather interesting in its approach to permissions. To properly check permissions one must take the current WP_REST_Request into consideration, which is a variable that's passed into these permission checks.

The only problem with this is that rest_send_allow_header() filters every API response, and it calls each of the permission checks with the current WP_REST_Request, which may or may not contain the type of request parameters expected to have been validated already; e.g., it may call get_items_permissions_check( $request ) with request parameters originally intended to create a new annotation.

That makes it more difficult to write a permission check that considers the current request, because at times the current request contains, or doesn't contain, all the information necessary to check permissions properly.

There might be something I can do to improve this though.

atimmer · 2018-02-08T11:07:18Z

lib/class-wp-rest-annotations-controller.php

+				$is_current_user_query = $current_user_id && array( $current_user_id ) === $request['author'];
+
+				foreach ( array(
+					'author',


Does this mean that you are not allowed to filter annotations on other authors unless you can edit their annotations? That sounds wrong, or am I not understanding the capabilities correctly?

The intention here is to prevent non-administrative users (e.g., authors/contributors) from looking at another user's history. The author, author_exclude, and author_email parameters are also guarded in core, in a similar way, for comments. A useful exception to this rule is that any user who can edit, can list their own annotations; i.e., note the $is_current_user_query variable a few lines below this.

That makes sense. I think it warrants an inline comment.

atimmer · 2018-02-08T12:17:38Z

phpunit/annotations/class-annotation-caps-test.php

+	 *
+	 * @return array Test arrays.
+	 */
+	protected function generate_standard_meta_cap_tests() {


Do you know about @dataProvider? This makes PHPUnit aware of the data set and tests will continue if one of the data points fails.

Thank you. I experimented with data providers a bit in the unit tests for this API. One thing I have against them is that they slow down unit tests that cover 1000's of permutations. Slower, because each dataset delivered by a data provider results in setUp() and tearDown() being called on each of those iterations; e.g., 45k permutations x 2 setup/teardown calls.

I found that tests ran much faster if they were generated in groups of permutations being run together. Resulting in just one setUp() and tearDown() for each group, which is all that was needed in most of the tests for this API.

Another thing I noticed is that all data providers run before any testing begins. So if a data-provider depends on any database-driven objects, those are lost when tests run, given all the heavy lifting done by WP_UnitTestCase.

Ah, then it is not feasible.

jaswrks · 2018-02-08T13:00:22Z

I have done a more thorough review of the PR.

@atimmer Thanks for digging in with me here. Your review is much appreciated and I'll take a look at each of the items you pointed out shortly. 😄

jaswrks · 2018-02-08T15:30:08Z

@atimmer Very helpful review. I'll go over the rest a bit later. Thanks again!

jaswrks · 2018-02-08T16:54:12Z

@atimmer @omarreiss

What would you like to shoot for in support of multiple annotation types?

I see two workable solutions:

Multiple annotation comment types. For example, annotation, seo_annotation, etc. This is how it works now and this approach could be improved by adding better filters. It could be extended further by registering meta fields to allow for seo_annotation subtypes, for example.
One annotation comment type, and then something like the annotation_type meta that you suggested. I assume it is this approach you like best, correct?

Can you think of any pros/cons associated with each of those?

One reason I went with option 1 is because I felt it would be easier to merge into a future WordPress core that supports register_comment_type(). When/if that occurs, then using multiple comment types may allow greater flexibility; e.g., each type of annotation could then be associated with different capabilities and other type-specific settings.

atimmer · 2018-02-09T15:17:34Z

@jaswrks The goal of multiple annotation types is to make sure the UI can default to only showing one type of annotation. I think that is possible using different annotation types. I'll discuss this with @omarreiss again to see if that achieves the goals we want to hit.

atimmer · 2018-02-14T13:06:05Z

I've discussed this and we think that having different comment types is 👌🏻.

jaswrks · 2018-02-19T09:24:19Z

👁👁 I could use another pair of eyes on this now. I think I've covered all recent requests and enhancements. I've added two new classes, some new functions, and new filters to make it easier for plugins to extend annotations.

New class: WP_Annotation
New class: WP_Annotation_Selector
New function and filter in: get_annotation()
New function and filter in: get_annotation_types()
New function and filter in: get_default_annotation_type()
New function and filter in: get_annotation_statuses()
New function and filter in: get_default_annotation_status()
Removed three or four instances of @codeCoverageIgnoreStart.
Removed an iteration by reference in favor of clarity.
Added inline comments to explain complex conditionals.
Removed meta from restricted fields when creating
as a user who lacks the ability to edit annotations.
Improved status codes in REST API controller permission checks.
Updated all existing unit tests following these changes.
Added new unit tests for the WP_Annotation class.

Getting an Annotation Instance

<?php
$annotation = get_annotation( 123 ); // WP_Annotation
$selector   = $annotation->get_selector(); // WP_Annotation_Selector
echo $selector->type; // e.g., XPathSelector

How to Add Annotation Types

These are comment types.

<?php
add_filter( 'get_annotation_types', function( $types ) {
    return array_merge( $types, array( 'seo_annotation' ) );
} );

How to Add Annotation Statuses

<?php
add_filter( 'get_annotation_statuses', function( $statuses ) {
    return array_merge( $statuses, array( 'accepted' ) );
} );

aduth · 2018-09-13T18:28:30Z

Is the work here still relevant, and can it be reasonably be brought up to date with current master? Or can we close it?

jaswrks · 2018-09-14T10:22:03Z

I just rebased this and squashed all commits to clean things up. All of the unit tests are passing and I ran a lint check on this again and corrected a few things that were caught by what looks to be an updated PHPCS configuration since this was worked on before.

So I don't see any reason this couldn't be used. LGTM! 🚢

Is the work here still relevant

I haven't been in on recent discussions about Annotations. @atimmer is working on some portions though. Do you still intend to use this REST API?

The docs for this API are here:
https://speca.io/jaswrks/wp-annotations

atimmer · 2018-09-14T11:57:45Z

The work is still relevant, however, I haven't prioritized this part. That's because a full-fledged commenting API with persistence is a ton more work than I have the capacity for. So I have focused on getting client side annotations to work because it will give us a feature that will work right now. Client-side annotation is also required for feature parity of the Yoast plugin with the classic editor.

It's good to note that I am keeping in mind this PR, so all my work is future compatible with the commenting API.

gziolo · 2018-12-14T10:38:50Z

@atimmer should we move it to Trac?

atimmer · 2018-12-14T12:22:00Z

@gziolo I don't see us working on this in the very short term, so I don't know if there is a lot of value of moving this to trac now.

gziolo · 2018-12-14T12:43:48Z

It seems like it isn't actionable in the upcoming months. It wasn't listed on the roadmap for 2019: https://make.wordpress.org/core/2018/12/08/9-priorities-for-2019/. Collaborative editing is planned for the 3rd phase of Gutenberg development. Let's close this PR and get back to it later. We can always reopen 😄

@jaswrks many thanks for your work done so far. I hope we can reuse most of the code in the future 🙇

This was referenced Jan 26, 2018

Add annotations REST API using comments #4386

Closed

Add annotations REST API using custom post type. #4385

Closed

mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Extensibility The ability to extend blocks or the editing experience labels Jan 26, 2018

mtias added this to the Bonus Features milestone Jan 26, 2018

chriskmnds reviewed Jan 27, 2018

View reviewed changes

jaswrks mentioned this pull request Jan 29, 2018

Fix Admin_Test, use wpSetUpBeforeClass() #4737

Merged

3 tasks

gziolo removed the [Feature] Extensibility The ability to extend blocks or the editing experience label Jan 31, 2018

atimmer requested changes Feb 1, 2018

View reviewed changes

atimmer requested changes Feb 8, 2018

View reviewed changes

ntwb mentioned this pull request Mar 5, 2018

Add RFC for Type Declarations WordPress-Coding-Standards/rfcs#3

Closed

aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018

Add REST API for back-end comments (annotations)

43f0636

atimmer mentioned this pull request Oct 2, 2018

Annotation API #7718

Merged

4 tasks

mtias added Future and removed Future labels Oct 7, 2018

mtias removed this from the Bonus Features milestone Oct 7, 2018

gziolo closed this Dec 14, 2018

omarreiss added the [Feature] Annotations Adding annotation functionality label Mar 26, 2019

REST API for annotations (back-end comments, take 2) #4685

REST API for annotations (back-end comments, take 2) #4685

Conversation

jaswrks commented Jan 26, 2018 • edited by gziolo Loading

Description

Overview of Changes

Introduces:

Modifies:

Near 100% coverage by unit tests

API Documentation

How can you test the WP REST API for Annotations?

How Has This Been Tested?

Types of changes

Back-End Annotation Permissions

Simplified Explanation

TODO

Non-Blocking TODOs

Client Side Exploration of Selectors

chriskmnds Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskmnds Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

jaswrks Jan 29, 2018 • edited Loading

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

jaswrks commented Jan 29, 2018 • edited Loading

Four Sets of Tests

jaswrks commented Jan 29, 2018 • edited Loading

🐛 Bug Fixes

jaswrks commented Jan 30, 2018 • edited Loading

atimmer left a comment • edited Loading

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

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

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

jaswrks Feb 3, 2018 • edited Loading

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

jaswrks commented Feb 2, 2018 • edited Loading

Summary of recent commits:

jaswrks commented Feb 3, 2018 • edited Loading

jaswrks commented Feb 3, 2018 • edited Loading

Front-End Annotation Conclusions

Back-End Annotation Conclusions

My hope is that this work will:

jaswrks commented Feb 4, 2018

atimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaswrks Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaswrks Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaswrks Feb 8, 2018 • edited Loading

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

jaswrks Feb 8, 2018 • edited Loading

jaswrks commented Jan 26, 2018 •

edited by gziolo

Loading

chriskmnds Jan 27, 2018 •

edited

Loading

chriskmnds Jan 27, 2018 •

edited

Loading

jaswrks Jan 29, 2018 •

edited

Loading

jaswrks commented Jan 29, 2018 •

edited

Loading

jaswrks commented Jan 29, 2018 •

edited

Loading

jaswrks commented Jan 30, 2018 •

edited

Loading

atimmer left a comment •

edited

Loading

jaswrks Feb 3, 2018 •

edited

Loading

jaswrks commented Feb 2, 2018 •

edited

Loading

jaswrks commented Feb 3, 2018 •

edited

Loading

jaswrks commented Feb 3, 2018 •

edited

Loading

jaswrks Feb 8, 2018 •

edited

Loading

jaswrks Feb 8, 2018 •

edited

Loading

jaswrks Feb 8, 2018 •

edited

Loading

jaswrks Feb 8, 2018 •

edited

Loading

atimmer Feb 9, 2018 •

edited

Loading

jaswrks Feb 19, 2018 •

edited

Loading

jaswrks Feb 8, 2018 •

edited

Loading

jaswrks commented Feb 8, 2018 •

edited

Loading