WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 9 months ago

#39730 closed defect (bug) (fixed)

Checking if wp_new_comment() return value is a WP_Error

Reported by: enrico.sorcinelli Owned by: peterwilsoncc
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: Comments Keywords: has-screenshots has-patch
Focuses: Cc:

Description

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

Attachments (7)

39730.patch (3.0 KB) - added by enrico.sorcinelli 18 months ago.
39730.2.patch (2.9 KB) - added by enrico.sorcinelli 18 months ago.
39730.3.patch (2.8 KB) - added by enrico.sorcinelli 13 months ago.
Updated to the current trunk.
39730.4.patch (3.7 KB) - added by enrico.sorcinelli 13 months ago.
Updated to the current trunk.
39730.5.patch (4.1 KB) - added by enrico.sorcinelli 12 months ago.
Updated to the current trunk.
39730.6.patch (3.7 KB) - added by ryotsun 9 months ago.
Fix codes according to the review comments.
39730.diff (4.0 KB) - added by peterwilsoncc 9 months ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
18 months ago

  • Milestone changed from Awaiting Review to 4.8

#3 @enrico.sorcinelli
18 months ago

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

#4 @Presskopp
18 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by flixos90. View the logs.


14 months ago

#6 @flixos90
14 months ago

  • Milestone changed from 4.8 to 4.8.1

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

@enrico.sorcinelli
13 months ago

Updated to the current trunk.

@enrico.sorcinelli
13 months ago

Updated to the current trunk.

#7 @enrico.sorcinelli
13 months ago

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 );
    
  1. Go to an edit post admin page
  2. 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):

https://cldup.com/e2Kp1ZhUNs.png

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

Last edited 12 months ago by enrico.sorcinelli (previous) (diff)

This ticket was mentioned in Slack in #core by jnylen. View the logs.


13 months ago

This ticket was mentioned in Slack in #core by enrico.sorcinelli. View the logs.


13 months ago

#10 @jnylen0
13 months ago

  • Milestone changed from 4.8.1 to 4.9

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.

This ticket was mentioned in Slack in #core by jnylen. View the logs.


12 months ago

@enrico.sorcinelli
12 months ago

Updated to the current trunk.

#12 @enrico.sorcinelli
12 months ago

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

#13 @enrico.sorcinelli
12 months ago

  • Keywords has-screenshots added

#14 follow-up: @peterwilsoncc
10 months ago

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()

#15 @westonruter
9 months ago

  • Keywords needs-patch added; has-patch removed

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

@ryotsun
9 months ago

Fix codes according to the review comments.

#16 in reply to: ↑ 14 @ryotsun
9 months ago

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()

#17 @jbpaul17
9 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


9 months ago

@peterwilsoncc
9 months ago

#19 @peterwilsoncc
9 months ago

  • Keywords needs-testing removed

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.

#20 @peterwilsoncc
9 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.