WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 4 weeks 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 10 months ago.
39730.2.patch (2.9 KB) - added by enrico.sorcinelli 10 months ago.
39730.3.patch (2.8 KB) - added by enrico.sorcinelli 5 months ago.
Updated to the current trunk.
39730.4.patch (3.7 KB) - added by enrico.sorcinelli 5 months ago.
Updated to the current trunk.
39730.5.patch (4.1 KB) - added by enrico.sorcinelli 4 months ago.
Updated to the current trunk.
39730.6.patch (3.7 KB) - added by ryotsun 5 weeks ago.
Fix codes according to the review comments.
39730.diff (4.0 KB) - added by peterwilsoncc 4 weeks ago.

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
10 months ago

  • Milestone changed from Awaiting Review to 4.8

#3 @enrico.sorcinelli
10 months ago

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

#4 @Presskopp
10 months ago

  • Keywords has-patch added

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


6 months ago

#6 @flixos90
6 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
5 months ago

Updated to the current trunk.

@enrico.sorcinelli
5 months ago

Updated to the current trunk.

#7 @enrico.sorcinelli
5 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 4 months ago by enrico.sorcinelli (previous) (diff)

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


5 months ago

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


4 months ago

#10 @jnylen0
4 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.


4 months ago

@enrico.sorcinelli
4 months ago

Updated to the current trunk.

#12 @enrico.sorcinelli
4 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
4 months ago

  • Keywords has-screenshots added

#14 follow-up: @peterwilsoncc
8 weeks 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
5 weeks 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
5 weeks ago

Fix codes according to the review comments.

#16 in reply to: ↑ 14 @ryotsun
5 weeks 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
4 weeks ago

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

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


4 weeks ago

@peterwilsoncc
4 weeks ago

#19 @peterwilsoncc
4 weeks 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
4 weeks 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.