[wp-trac] [WordPress Trac] #39735: Right check for wp_update_comment (WP_REST_Comments_Controller::update_item)

[WordPress Trac]

#39735: Right check for wp_update_comment
(WP_REST_Comments_Controller::update_item)
-------------------------------+-----------------------
 Reporter:  enrico.sorcinelli  |       Owner:
     Type:  defect (bug)       |      Status:  closed
 Priority:  normal             |   Milestone:
Component:  REST API           |     Version:  4.7
 Severity:  normal             |  Resolution:  invalid
 Keywords:                     |     Focuses:  rest-api
-------------------------------+-----------------------
Changes (by jnylen0):

 * status:  new => closed
 * version:  trunk => 4.7
 * resolution:   => invalid
 * milestone:  Awaiting Review =>


Comment:

 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` - [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-
 includes/comment.php?marks=2140-2142#L2123 invalid comment ID].  Handled
 [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/rest-
 api/endpoints/class-wp-rest-comments-controller.php?marks=673-676#L663
 here] in the API code.
 - `0` - [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-
 includes/comment.php?marks=2145-2147#L2123 invalid comment post ID].
 Handled [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-
 includes/rest-api/endpoints/class-wp-rest-comments-
 controller.php?marks=692-694#L663 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
 [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/wp-
 db.php?marks=2024,2029,2033#L1992 here] or
 [https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/wp-
 db.php?marks=1728,1756,1792#L1715 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/39735#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform