Allowing wp_update_comment() to return WP_Error

[enrico.sorcinelli]

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

[SergeyBiryukov]

Related: #39730

[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.

[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.

[enrico.sorcinelli]

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

[enrico.sorcinelli]

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

[jnylen0]

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:

• #38700, especially discussion around ticket:38700#comment:7
• #39735

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/

[jbpaul17]

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

[jbpaul17]

Punting to 4.8.1 per today's bug scrub.

[enrico.sorcinelli]

Updated to the current trunk.

[enrico.sorcinelli]

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 commented lines updated on success (0 or 1).
• false or WP_Error on failure.

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

[enrico.sorcinelli]

Updated wp_update_comment_data documentation

[enrico.sorcinelli]

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

[jnylen0]

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]

Fixed space

[enrico.sorcinelli]

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.

[afercia]

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

[enrico.sorcinelli]

Updated to the current trunk.

[jbpaul17]

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

[enrico.sorcinelli]

Updated to the current trunk.

[pento]

This ticket needs testing and a decision.

[desrosj]

This still needs testing and a decision. It is also marked early, and we have passed into late on the 5.2 timeline.

[SergeyBiryukov]

Related: #18630

[desrosj]

With 5.3 beta 1 in a few hours, this unfortunately needs to be punted.

[davidbaumwald]

The latest patch no longer applies cleanly, so this needs-refresh. It's marked early and should be shepherded fairly soon if this is to land in 5.4.

[imath]

Hi thanks a lot @enrico.sorcinelli for your contributions and patches about this ticket.

I've updated the patch to current trunk and made the following edits in 39732.9.patch

1. I think it's safer to return what was used before if the $wp_error argument is false. That is to say 0.
2. I've edited what was done for the 'wp_update_comment_data' as I believe the WP_Error object never makes its way there as it is returned, so we must keep the comment $data unchanged. I believe it is better to pass the $wp_error argument to inform the plugin what type of return is expected if it's needed to stop the update process.
3. I believe it's wrong to return an error if the comment data has not changed because it prevents the comment meta update. So I removed the code you added after the $wpdb->update() part.
4. In edit_comment() we need to return the result of the update process and do the wp_die() into the comment edit screen otherwise we are redirected even if there was an error.
5. We need to adapt wp_ajax_edit_comment() to eventually return the error.
6. I've run composer format on all edited files.
7. I've tested the patch from the form of the wp-admin/comment.php screen. I confirm it works as expected.
8. I've tested the patch using the Quick edit (Ajax) link of a listed comment of the wp-admin/edit-comments.php screen. I confirm it works as expected.
9. I've run phpunit --group comment, phpunit --group ajax & phpunit --group restapi and I confirm tests passed successfully.

@davidbaumwald could you have a look at it and check if it's good to be committed?

[davidbaumwald]

@imath Had a chance to review and test this. Everything seems good to go from my point of view.

[imath]

Awesome thanks a lot @davidbaumwald I guess we can tag it as to « commit » 😊 On my side, I’ll work on a draft of the needed development note.

[davidbaumwald]

The latest patch now fails against trunk. @imath Unfortunately, this needs to move to 5.5.

[imath]

@davidbaumwald just refreshed the patch. Let's hope it gets committed before it loses again his freshness :)

[audrasjb]

Bad news, the patch lose its freshnesss :'(
Good news: patch refresh incoming :-)

[audrasjb]

Fresh patch for 5.5

[whyisjake]

In 48154:

Comments: Allow wp_update_comment() to return WP_Error().

The wp_update_comment_data filter introduced in 4.7 allows comment data to be filtered 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().

Fixes #39732.

Props: enricosorcinelli, swissspidy, gkloveweb, jnylen0, jbpaul17, afercia, SergeyBiryukov, audrasjb, imath, davidbaumwald.

[SergeyBiryukov]

In 48207:

Docs: Correct description for the $avoid_die parameter of wp_check_comment_flood().

The function always return a boolean value, never a WP_Error object.

See #49572, #39732.

[SergeyBiryukov]

In 48208:

Comments: Rename the $avoid_die parameter of wp_allow_comment() and wp_new_comment() to $wp_error.

This makes the function signatures more consistent with wp_update_comment() and wp_set_comment_status().

wp_check_comment_flood() is left as the only function with the $avoid_die parameter for now, as it does not return a WP_Error object.

Follow-up to [48154], [48207].

See #39732.

[SergeyBiryukov]

In 48215:

Comments: Minor adjustments to wp_update_comment():

• Revert the logic of $wp_error checks to avoid negation.
• Clarify the return value, restore the edits from [47017].
• Update wp_update_comment_data filter check to allow false to prevent the update.

Follow-up to [48154].

See #39732.

[SergeyBiryukov]

In 48216:

Comments: Make wp_update_comment() return a WP_Error object on database error, if $wp_error parameter is true.

Follow-up to [48154], [48215].

See #39732.

[SergeyBiryukov]

In 48217:

Tests: Remove unnecessary i18n from _wp_update_comment_data_filter().

Follow-up to [48154].

See #39732.

[SergeyBiryukov]

In 48218:

Comments: Make wp_update_comment() return a WP_Error object for a canceled update, if $wp_error parameter is true.

Remove redundant checks for wp_update_comment() results being false, as the function always returns a WP_Error object now if $wp_error is true.

Follow-up to [48154], [48215], [48216].

See #39732.

[SergeyBiryukov]

I would also like to take this opportunity to reconsider the return values for an invalid comment or post ID, changing them from 0 to false, as previously suggested by @dd32 in comment:8:ticket:38700:

The two return 0's were seemingly added due to the function doc specifying 1 = success, 0 = failure, which was only because it returned the number of rows affected.
I know it'd technically be a BC change, but given WordPress really doesn't encourage strict type checking for falsey values I think that'd be a viable option here.

Also related: comment:3:ticket:39735.

See 39732.12.diff​.

[SergeyBiryukov]

In 48222:

Tests: Remove unnecessary tearDown() method in Tests_Ajax_EditComment.

Restoring the hook-related globals to their state at setUp() is addressed by WP_UnitTestCase_Base::_restore_hooks().

Follow-up to [48154].

See #39732.

[SergeyBiryukov]

In 48223:

Docs: Add a @since note to edit_comment() about the new return value.

Follow-up to [48154].

See #39732.

[SergeyBiryukov]

In 48228:

Tests: Remove unnecessary i18n from _wp_update_comment_data_filter().

Follow-up to [48154], [48217].

See #39732.

[SergeyBiryukov]

In 48230:

Comments: Add a @since note to the wp_update_comment_data filter about returning a WP_Error value.

Remove the ability to short-circuit comment update by returning false from the filter for now.

This was inconsistent with the pre_comment_approved filter, and should not be necessary if a more descriptive reason can be given by always using WP_Error.

See #39732.

[SergeyBiryukov]

In 48231:

Tests: Place remove_filter() calls for _wp_update_comment_data_filter() before the assertions, for consistency with other tests.

Follow-up to [48154], [48222].

See #39732.

[SergeyBiryukov]

In 48235:

Comments: Make wp_update_comment() return false instead of 0 for an invalid comment or post ID.

This addresses an inconsistency where 0 could mean one of the three scenarios:

• Invalid comment ID.
• Invalid comment post ID.
• No DB rows updated. This is not an error and should not be treated as one.

With this change, wp_update_comment() always returns either false or a WP_Error object on failure, depending on the value of the $wp_error parameter.

Follow-up to [48154], [48215], [48216], [48218], [48230].

Props dd32, jnylen0, enrico.sorcinelli.
Fixes #39732. See #38700, #39735.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2020/07/29/miscellaneous-developer-focused-changes-in-wordpress-5-5/