WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#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_Errorobject)

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)

39735.patch (2.5 KB) - added by enrico.sorcinelli 3 years ago.

Download all attachments as: .zip

Change History (4)

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


3 years ago

#2 @kadamwhite
3 years ago

Per @jnylen0 in slack the current behavior is unintuitive, but correct; tagging him to clarify further

#3 @jnylen0
3 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.

Note: See TracTickets for help on using tickets.