Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#39735 closed defect (bug) (invalid)

Right check for wp_update_comment (WP_REST_Comments_Controller::update_item)

Reported by: enricosorcinelli's profile enrico.sorcinelli Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords:
Focuses: rest-api Cc:


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 7 years ago.

Download all attachments as: .zip

Change History (6)

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

7 years ago

#2 @kadamwhite
7 years ago

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

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

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

4 years ago

#5 @SergeyBiryukov
4 years ago

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.