#39732 closed enhancement (fixed)
Allowing wp_update_comment() to return WP_Error
Reported by: | enrico.sorcinelli | Owned by: | 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)
Change History (70)
#2
follow-ups:
↓ 3
↓ 4
@
8 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 whatif ( is_wp_error( $updated ) )
does.
#3
in reply to:
↑ 2
;
follow-up:
↓ 5
@
8 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 whatif ( 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
@
8 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 whatif ( 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
@
8 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 whatif ( 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
#6
@
8 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:
- #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/
#7
@
8 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.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by flixos90. View the logs.
8 years ago
#12
@
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
or1
). false
orWP_Error
on failure.
I also updated all core wp_update_comment
calls by setting $wp_error
to true
.
#13
@
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
@
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.
#15
@
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
@
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
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
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#23
@
7 years ago
- Milestone changed from 4.9 to Future Release
Punting this to Future Release per the 4.9 bug scrub earlier today.
This ticket was mentioned in Slack in #core by enrico.sorcinelli. View the logs.
7 years ago
#28
@
6 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
@
5 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
@
5 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.
5 years ago
#35
@
5 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
- I think it's safer to return what was used before if the
$wp_error
argument is false. That is to say0
. - 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. - 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. - In
edit_comment()
we need to return the result of the update process and do thewp_die()
into the comment edit screen otherwise we are redirected even if there was an error. - We need to adapt
wp_ajax_edit_comment()
to eventually return the error. - I've run
composer format
on all edited files. - I've tested the patch from the form of the
wp-admin/comment.php
screen. I confirm it works as expected. - 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. - 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?
#36
@
5 years ago
@imath Had a chance to review and test this. Everything seems good to go from my point of view.
#37
@
5 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.
5 years ago
This ticket was mentioned in Slack in #core by imath. View the logs.
5 years ago
#40
@
5 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.
#41
@
5 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
@
4 years ago
- Keywords needs-refresh added
Bad news, the patch lose its freshnesss :'(
Good news: patch refresh incoming :-)
#51
@
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.
Related: #39730