#39735 closed defect (bug) (invalid)
Right check for wp_update_comment (WP_REST_Comments_Controller::update_item)
Reported by: | enrico.sorcinelli | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | |
Focuses: | rest-api | Cc: |
Description
The WP_REST_Comments_Controller::update_item
method assumes that the wp_update_comment
function returns also false
value in order to check if the update fails.
This is never true since wp_update_comment
currently only returns 1
or 0
(and, if #39732 will be accepted, it could return also a WP_Error
object)
The attached patch fixes the check, even if the downside is that if no DB rows are updated, the method above return a WP_Error
.
Maybe have we to work into wp_update_comment
allowing it to return also false
value?
Attachments (1)
Change History (6)
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
8 years ago
#3
@
8 years ago
- Milestone Awaiting Review deleted
- Resolution set to invalid
- Status changed from new to closed
- Version changed from trunk to 4.7
Hi @enrico.sorcinelli - the current behavior is correct, though the code could definitely be improved.
There are several possible return paths from wp_update_comment
:
0
- invalid comment ID. Handled here in the API code.0
- invalid comment post ID. Handled here in the API code.0
- no DB rows updated. This is not an error and should not be treated as one.1
- DB row updated successfully.false
- the low-level DB update failed, for example here or here. This indicates a problem with the server, so it should be a HTTP 500 error as it is currently.
The REST API should accept an update that does not actually change anything - this is not an error condition. It's unfortunate that wp_update_comment
uses 0
as the return value for both an error and a success. We work around this in the REST API by detecting the possible error conditions first. Then we know that 0
actually represents a success.
See #38700 for more details and previous changes here.
We can improve this code by first changing wp_update_comment
to have more sensible return values, as you've proposed in #39732. Let's move the discussion over there.
Finally, if there are missing test cases for any of the conditions I outlined above, I'd like to get them added.
Per @jnylen0 in slack the current behavior is unintuitive, but correct; tagging him to clarify further