Comments: Do not use HTTP 500 for non-server errors.

[mdawaffe]

Changes proposed in this Pull Request:

Jetpack should not be responding with HTTP 5xx errors when a client makes a bad or unauthorized requests. HTTP 4xx should be used for such requests.

Testing instructions:

It's annoying. Do the following for both master and this branch (update/comments-should-not-die-with-500).

1. Edit modules/comments/comments.php so that the signature check always fails. Something like the following will work:

diff --git a/modules/comments/comments.php b/modules/comments/comments.php
index e352ab7f8..cfbc9a92b 100644
--- a/modules/comments/comments.php
+++ b/modules/comments/comments.php
@@ -471,4 +471,6 @@ class Jetpack_Comments extends Highlander_Comments_Base {
 		}
 
+$check = '';
+
 		// Bail if token is expired or not valid
 		if ( $check !== $post_array['sig'] ) {

2. Ensure Jetpack Comments is enabled.
3. Compose a comment on a post. Do not yet submit it.
4. In your browser console, look at the network requests log.
5. Submit the comment.

In master, you'll see the wp-comments-post.php request has a 500 response. In this branch, you'll see a 400.

Remember to undo the hack from step 1 :)

Proposed changelog entry for your changes:

• Use HTTP 4xx status codes for comment errors.

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello mdawaffe! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D28464-code before merging this PR. Thank you!

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 657a57e

[kraftbj]

Are there any consumers of this error that is expecting a 500 within Jetpack that you're aware of? I can't think of any, but that would be the only reason not to do it (yet).

[mdawaffe]

I know of none. I do know the Automattic Systems team would like Jetpack (and WordPress in general) to not respond with 500s unless something is actually broken with the server (which is rarely the case by the time PHP is checking this sort of thing).

I'm not sure about these 500s specifically, but Jetpack's in general cause false positives for them.

[mdawaffe]

All of (most of) core's equivalent comment errors return 4xx: https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/comment.php#L3114

(Any WP_Error there without a message/status just results in an exit: https://core.trac.wordpress.org/browser/tags/5.2/src/wp-comments-post.php#L25)

[kraftbj]

The status arg on the error data probably should be more documented in Core :), but it all works and tracks.

[jeherve]

LGTM! Merging.

[westonruter]

Related Trac ticket for wp-comment-post.php in core to send back 400 instead of 200 for invalid comment submissions: https://core.trac.wordpress.org/ticket/47393

[kraftbj]

r206694-wpcom