WordPress.org

Make WordPress Core

Opened 2 years ago

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

Download all attachments as: .zip

Change History (27)

#1 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 4.8

#3 @enrico.sorcinelli
2 years ago

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

#4 @Presskopp
2 years ago

  • Keywords has-patch added

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


2 years ago

#6 @flixos90
2 years ago

  • Milestone changed from 4.8 to 4.8.1

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

@enrico.sorcinelli
22 months ago

Updated to the current trunk.

@enrico.sorcinelli
22 months ago

Updated to the current trunk.

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

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


22 months ago

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


22 months ago

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


22 months ago

@enrico.sorcinelli
22 months ago

Updated to the current trunk.

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

  • Keywords has-screenshots added

#14 follow-up: @peterwilsoncc
19 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
18 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
18 months ago

Fix codes according to the review comments.

#16 in reply to: ↑ 14 @ryotsun
18 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
18 months ago

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

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


18 months ago

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