Make WordPress Core

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#39732 closed enhancement (fixed)

Allowing wp_update_comment() to return WP_Error

Reported by: enricosorcinelli's profile enrico.sorcinelli Owned by: enricosorcinelli's profile enrico.sorcinelli
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.8
Component: Comments Keywords: has-patch has-unit-tests commit has-dev-note
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 (12)

39732.patch (8.0 KB) - added by enrico.sorcinelli 7 years ago.
39732.2.patch (7.9 KB) - added by gk.loveweb 7 years ago.
39732.3.patch (7.9 KB) - added by enrico.sorcinelli 7 years ago.
39732.4.patch (10.0 KB) - added by enrico.sorcinelli 7 years ago.
Updated to the current trunk.
39732.5.patch (10.7 KB) - added by enrico.sorcinelli 7 years ago.
Updated wp_update_comment_data documentation
39732.6.patch (10.7 KB) - added by enrico.sorcinelli 7 years ago.
Fixed space
39732.7.patch (11.2 KB) - added by enrico.sorcinelli 7 years ago.
Updated to the current trunk.
39732.8.patch (11.6 KB) - added by enrico.sorcinelli 6 years ago.
Updated to the current trunk.
39732.9.patch (11.1 KB) - added by imath 4 years ago.
39732.10.patch (11.0 KB) - added by imath 4 years ago.
39732.11.diff (11.1 KB) - added by audrasjb 4 years ago.
Fresh patch for 5.5
39732.12.diff (905 bytes) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (70)

#1 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.8

Related: #39730

#2 follow-ups: @swissspidy
7 years 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.

@gk.loveweb
7 years ago

#3 in reply to: ↑ 2 ; follow-up: @gk.loveweb
7 years 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
7 years 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
7 years 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 7 years ago by enrico.sorcinelli (previous) (diff)

#6 @jnylen0
7 years 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 years 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 years ago

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


7 years ago

#10 @jbpaul17
7 years 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.


7 years ago

@enrico.sorcinelli
7 years ago

Updated to the current trunk.

#12 @enrico.sorcinelli
7 years 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 7 years ago by enrico.sorcinelli (previous) (diff)

@enrico.sorcinelli
7 years ago

Updated wp_update_comment_data documentation

#13 @enrico.sorcinelli
7 years 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
7 years 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
7 years ago

Fixed space

#15 @enrico.sorcinelli
7 years 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
7 years 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.


7 years ago

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


7 years ago

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


7 years ago

@enrico.sorcinelli
7 years ago

Updated to the current trunk.

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


6 years ago

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


6 years ago

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


6 years ago

#23 @jbpaul17
6 years ago

  • Milestone changed from 4.9 to Future Release

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

@enrico.sorcinelli
6 years ago

Updated to the current trunk.

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


6 years ago

#25 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 5.0

#26 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#27 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

This ticket needs testing and a decision.

#28 @desrosj
5 years ago

  • Milestone changed from 5.2 to 5.3

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

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


5 years ago

#31 @desrosj
4 years ago

  • Milestone changed from 5.3 to 5.4

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

#32 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

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.

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


4 years ago

#34 @audrasjb
4 years ago

  • Keywords needs-dev-note added

@imath
4 years ago

#35 @imath
4 years ago

  • Keywords needs-testing needs-refresh removed

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?

Last edited 4 years ago by imath (previous) (diff)

#36 @davidbaumwald
4 years ago

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

#37 @imath
4 years ago

  • Keywords commit added; early removed

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.

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


4 years ago

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


4 years ago

#40 @davidbaumwald
4 years ago

  • Keywords needs-refresh added; commit removed
  • Milestone changed from 5.4 to 5.5

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

@imath
4 years ago

#41 @imath
4 years ago

  • Keywords commit added; needs-refresh removed

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

#42 @audrasjb
4 years ago

  • Keywords needs-refresh added

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

@audrasjb
4 years ago

Fresh patch for 5.5

#43 @audrasjb
4 years ago

  • Keywords needs-refresh removed

#44 @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#45 @SergeyBiryukov
4 years ago

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.

#46 @SergeyBiryukov
4 years ago

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.

#47 @SergeyBiryukov
4 years ago

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.

#48 @SergeyBiryukov
4 years ago

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.

#49 @SergeyBiryukov
4 years ago

In 48217:

Tests: Remove unnecessary i18n from _wp_update_comment_data_filter().

Follow-up to [48154].

See #39732.

#50 @SergeyBiryukov
4 years ago

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.

#51 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#52 @SergeyBiryukov
4 years ago

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.

#53 @SergeyBiryukov
4 years ago

In 48223:

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

Follow-up to [48154].

See #39732.

#54 @SergeyBiryukov
4 years ago

In 48228:

Tests: Remove unnecessary i18n from _wp_update_comment_data_filter().

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

See #39732.

#55 @SergeyBiryukov
4 years ago

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.

#56 @SergeyBiryukov
4 years ago

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.

#57 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.