Checking if wp_new_comment() return value is a WP_Error

[enrico.sorcinelli]

Since wp_new_comment() can return WP_Error (also it can be returned through pre_comments_approved filter), we should check it in all call occurrences.

This essentially affects adding new comments (via ajax) in post admin where the result was a new empty comment (with anonymous avatar) with not working links (since they don't have comment id).
I added the check before get_comment($comment_id) (*) call and comments list building.

The patch also fix all other wp_new_comment() calls I found.

(*): In general, maybe should WP_Comment take care to the object passed in the constructor by checking if it's a WP_Error

[SergeyBiryukov]

Related: #39732

[enrico.sorcinelli]

I just updated the patch by removing redundant comments and by adding missing curly braces.

[flixos90]

Let's focus on this in 4.8.1, together with #39732.

[enrico.sorcinelli]

Updated to the current trunk.

[enrico.sorcinelli]

Updated to the current trunk.

[enrico.sorcinelli]

I updated the patch also by documenting that by returning WP_Error from pre_comments_approved filter will shortcircuit the insertion and allow skipping further processing. This new little enhancement offers to the user the chance to skip comment insertion based on a custom criteria after the core checks.

Additional instruction about how to reproduce the error on the current trunk:

1. Add a filter like following:

   <?php
   function wp_is_comment_flood_filter () {
           return true;
   }
   add_filter ( 'wp_is_comment_flood', 'wp_is_comment_flood_filter', 10 );
   

2. Go to an edit post admin page
3. Add a new not-empty comment

A new one empty comment will appear on comment list below with anonymous user, unresolved avatar and with not working links (since they don't have comment id):

Anyway the new comments haven't been inserted on the database and by reloading page will clean anonymous comments.

[jnylen0]

Closely linked to #39732 which is slated for 4.9, and it makes me a bit nervous to put either of these in a minor release.

[enrico.sorcinelli]

Updated to the current trunk.

[enrico.sorcinelli]

I just upload the updated patch to current trunk.
I also fixed documentation and fixed filter removal code in test unit.

[peterwilsoncc]

Taking a quick look at the latest patch, I have a few comments:

• In wp-admin/includes/ajax-actions.php
  • can you use wp_send_json_error() instead of wp_die()
• in tests/phpunit/tests/ajax/ReplytoComment.php
  • there is no need to remove the filter during tearDown(), the WP test suite does this automatically
  • Can you add an docblock to test_pre_comments_approved()

[westonruter]

If we don't get this fixed up in the next day we'll punt to 4.9.1

[ryotsun]

Fix codes according to the review comments.

[ryotsun]

I've fixed the following.

Replying to peterwilsoncc:

Taking a quick look at the latest patch, I have a few comments:

• In wp-admin/includes/ajax-actions.php
  • can you use wp_send_json_error() instead of wp_die()
• in tests/phpunit/tests/ajax/ReplytoComment.php
  • there is no need to remove the filter during tearDown(), the WP test suite does this automatically
  • Can you add an docblock to test_pre_comments_approved()

[peterwilsoncc]

In 39730.diff​:

• reverts wp_send_json_error() back to wp_die() from 39730.6.patch​ (this was my mis-steer, sorry folks)
• fixes a minor code style violation
• removes internationalisation from filter used in tests to prevent false errors in languages in other than english (the assertion does not use i18n)

I can reproduce the error described, the patch fixes it. Will commit momentarily.

[peterwilsoncc]

In 41980:

Comments: Check if wp_new_comment() returns an error.

Adds checks throughout to allow for wp_new_comment() returning a WP_Error instance.

Updates the docs for the pre_comment_approved filter to include that it can be passed an error.

Props enrico.sorcinelli, ryotsun.
Fixes #39730.