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.