WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 2 months ago

#39730 new defect (bug)

Checking if wp_new_comment() return value is a WP_Error

Reported by: enrico.sorcinelli Owned by:
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: Comments Keywords: has-patch has-screenshots
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 (5)

39730.patch (3.0 KB) - added by enrico.sorcinelli 8 months ago.
39730.2.patch (2.9 KB) - added by enrico.sorcinelli 8 months ago.
39730.3.patch (2.8 KB) - added by enrico.sorcinelli 3 months ago.
Updated to the current trunk.
39730.4.patch (3.7 KB) - added by enrico.sorcinelli 3 months ago.
Updated to the current trunk.
39730.5.patch (4.1 KB) - added by enrico.sorcinelli 2 months ago.
Updated to the current trunk.

Download all attachments as: .zip

Change History (18)

#1 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 4.8

#3 @enrico.sorcinelli
8 months ago

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

#4 @Presskopp
8 months ago

  • Keywords has-patch added

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


4 months ago

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

Updated to the current trunk.

@enrico.sorcinelli
3 months ago

Updated to the current trunk.

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

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


3 months ago

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


2 months ago

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


2 months ago

@enrico.sorcinelli
2 months ago

Updated to the current trunk.

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

  • Keywords has-screenshots added
Note: See TracTickets for help on using tickets.