WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 7 weeks ago

#39732 assigned enhancement

Allowing wp_update_comment() to return WP_Error

Reported by: enrico.sorcinelli Owned by: enrico.sorcinelli
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Comments Keywords: has-patch has-unit-tests needs-testing early
Focuses: Cc:

Description

The new wp_update_comment_data filter introduced in 4.7 allows to filter comments data before it is updated in the database.

The patch aims to handle WP_Error as the filter above return value in a similar manner as is done for wp_new_comment().

Attachments (7)

39732.patch (8.0 KB) - added by enrico.sorcinelli 10 months ago.
39732.2.patch (7.9 KB) - added by gk.loveweb 10 months ago.
39732.3.patch (7.9 KB) - added by enrico.sorcinelli 10 months ago.
39732.4.patch (10.0 KB) - added by enrico.sorcinelli 5 months ago.
Updated to the current trunk.
39732.5.patch (10.7 KB) - added by enrico.sorcinelli 5 months ago.
Updated wp_update_comment_data documentation
39732.6.patch (10.7 KB) - added by enrico.sorcinelli 5 months ago.
Fixed space
39732.7.patch (11.2 KB) - added by enrico.sorcinelli 4 months ago.
Updated to the current trunk.

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
10 months ago

  • Milestone changed from Awaiting Review to 4.8

Related: #39730

#2 follow-ups: @swissspidy
10 months ago

  • Keywords has-patch has-unit-tests added

Two notes on the patch after a quick glance:

  • Coding standards need to be adhered to (e.g. curly braces)
  • The // Check for WP_Error comments are redundant. It's clear what if ( is_wp_error( $updated ) ) does.

#3 in reply to: ↑ 2 ; follow-up: @gk.loveweb
10 months ago

Replying to swissspidy:

Two notes on the patch after a quick glance:

  • Coding standards need to be adhered to (e.g. curly braces)
  • The // Check for WP_Error comments are redundant. It's clear what if ( is_wp_error( $updated ) ) does.

Please check my patch above, I have removed inline comment which is no longer needed or useful.

#4 in reply to: ↑ 2 @enrico.sorcinelli
10 months ago

Replying to swissspidy:

Two notes on the patch after a quick glance:

  • Coding standards need to be adhered to (e.g. curly braces)
  • The // Check for WP_Error comments are redundant. It's clear what if ( is_wp_error( $updated ) ) does.

My fault :-)
I added curly braces where missing (FYI, wp-admin/includes/comment.php contains a lot of braceless statements) and removed redundant comments

#5 in reply to: ↑ 3 @enrico.sorcinelli
10 months ago

Replying to gk.loveweb:

Replying to swissspidy:

Two notes on the patch after a quick glance:

  • Coding standards need to be adhered to (e.g. curly braces)
  • The // Check for WP_Error comments are redundant. It's clear what if ( is_wp_error( $updated ) ) does.

Please check my patch above, I have removed inline comment which is no longer needed or useful.

In the fix of my patch you sent, it seems that you've removed all curly braces, not added where they were missing, as noticed

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

#6 @jnylen0
10 months ago

In general I agree that enhancing wp_update_comment to return a WP_Error is a good idea. However, as written, this would break a bunch of plugins and other code that relies on the current behavior. Instead, we need to add a second parameter $wp_error that defaults to false. If it is true then we can use the newer, better return values. @dd32 started on a patch to do this: 38700.wp-error.diff

See also:

Finally, we should make sure new code corresponds to the coding standards (braces around statements), but we don't want to change the existing code even if its formatting is not ideal. This creates needless code churn and there are much more important tasks to be done: https://make.wordpress.org/core/handbook/contribute/code-refactoring/

#7 @jbpaul17
7 months ago

  • Keywords needs-refresh added; has-patch removed

@enrico.sorcinelli based on the comments it sounds like another refresh is needed on this ticket.

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


7 months ago

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


6 months ago

#10 @jbpaul17
6 months ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

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


6 months ago

@enrico.sorcinelli
5 months ago

Updated to the current trunk.

#12 @enrico.sorcinelli
5 months ago

I updated the patch based on @jnylen0 suggestions in order to keep the current behaviour of wp_update_comment. wp_update_comment now accepts a second parameter $wp_error (default to false). If true then we return WP_Error on failure.

So, there will be the following possible return paths from wp_update_comment:

  • Number of comment rows updated on success (0 or 1).
  • false or WP_Error on failure.

I also updated all core wp_update_comment calls by setting $wp_error to true.

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

@enrico.sorcinelli
5 months ago

Updated wp_update_comment_data documentation

#13 @enrico.sorcinelli
5 months ago

I just documented that returning a WP_Error value from wp_update_comment_data filter will shortcircuit updating and allow skipping further processing.

#14 @jnylen0
5 months ago

  • Keywords has-patch needs-testing early added; needs-refresh removed
  • Milestone changed from 4.8.1 to 4.9

39732.5.patch generally looks good to me, though it needs more testing and it should land early in a major release cycle.

I am not sure about this part, it seems like we probably need better error handling here (and there is an extra space inside wp_die ():

<?php
$result = wp_update_comment( $_POST, true );

if ( is_wp_error( $result ) ) { 
    wp_die ( $result->get_error_message() ); 
}

In general we need to be sure we understand all the ways existing behavior will change as a result of this patch.

@enrico.sorcinelli
5 months ago

Fixed space

#15 @enrico.sorcinelli
5 months ago

I just removed space.

The new code above lives in edit_comment() function that is used only twice in the core for editing comments, (edit screen and quick edit).
Both codes already have inside other wp_die statements here.

The main modification is that the patch aims to solve the fact that wp_update_comment uses 0 as the return value for both an error and a success (and until now editing comments in admin did not care of comment update return path at all).
All wp_update_comment calls in the core have been updated and tested with new unit tests included in the patch.

The only thing we need to be sure is to check all wp_update_comment extra-core calls (like plugins) where comment update calls will return only false and not also 0 on failure.

But it's not a real problem for me since wp_update_comment already can return false and related code should take care with this regardless the proposal enhancement.

#16 @afercia
5 months ago

  • Owner set to enrico.sorcinelli
  • Status changed from new to assigned

Assigning to @enrico.sorcinelli as he's dedicated a considerable amount of time to this ticket. Also, quoting @jnylen0 (ping) this should land early in a major release cycle in order to be properly and broadly tested. That means: now :)

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 jnylen. View the logs.


4 months ago

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.

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


8 weeks ago

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


7 weeks ago

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


7 weeks ago

#23 @jbpaul17
7 weeks ago

  • Milestone changed from 4.9 to Future Release

Punting this to Future Release per the 4.9 bug scrub earlier today.

Note: See TracTickets for help on using tickets.