REST API: Cannot send an empty or no-op comment update

[jnylen0]

Sending a POST/PUT/PATCH request to /wp/v2/comments/:id fails if the update is empty, or if the comment data is the same as what's currently stored in the database.

This is due to a quirk of wp_update_comment: it returns the number of rows updated by $wpdb->update, which may be zero. However, zero is also used as the return value for an error condition. The API should differentiate between these two states.

For any other object type, sending a "null update" via the API is successful.

[jnylen0]

Cleaner patch using did_action( 'edit_comment' )

[rmccue]

I'm not a fan of using did_action here in a meta-programming way, as it's possible plugins could be firing this hook. Could we instead check the new data against the current data, and if it's equal, don't update?

[jnylen0]

I don't think that can be done easily, since wp_update_comment calls wp_filter_comment and apply_filters( 'comment_save_pre' ). We'd have to duplicate all of this logic.

If a plugin fires edit_comment before this code is triggered, this will work fine, because we're tracking the number of additional times it was called during wp_update_comment.

If a plugin manages to fire edit_comment during wp_update_comment without updating the comment, then that is an absurd thing to do, and we shouldn't expect it to work anyway.

[rmccue]

We can check if the output from get_item() for the submitted items matches the submitted values. That gets us 99% of the functionality we need here without needing the meta-hack.

[jnylen0]

The following scenario seems fairly plausible to me:

• A client submits a comment edit with some transformation needed (an invalid entity; a disallowed HTML tag). HTTP 200
• The client accidentally submits the same request again (the user clicked save twice in a row). The content in the database is different from what was submitted, and we pass off to wp_update_comment, which "fails" but not really. HTTP 500 this time.

The real problem is the inconsistent return values of wp_update_comment, so we should address the issue right there at the source. Maybe to avoid did_action or flag-setting hacks, we should add a filter that allows us to change the "real" error return value from 0 to false, then remove it when we get done?

[jnylen0]

Use a new wp_update_comment_error filter to work around the underlying issue

[rmccue]

I like this much better. :)

[rmccue]

​Discussed on Slack, the better solution really is to add a $wp_error flag to wp_update_comment to match wp_insert_post (etc), which would return us a proper error instead. Since this is an edge case, and it's too late in the cycle to add this, punting to 4.8.

[dd32]

I think this could also be changed from return 0; to return false; in addition to, or in place of, adding a $wp_error.

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.

[jnylen0]

@rmccue @dd32 I think we should fix this in 4.7. An unexpected HTTP 500 for no good reason is pretty bad.

The two error conditions in wp_update_comment that we're trying to catch are (1) invalid comment ID and (2) invalid comment post ID. We already check (1) in the API, let's check (2) as well and return a better error code than the 500.

After that, we can fix this issue by disregarding the return value of wp_update_comment.

[jnylen0]

Perform all error checking ourselves, then disregard the return value of wp_update_comment.

[dd32]

(re-adding early keyword unless it's moved out of Future Release)

I agree that these error conditions should be fixed, I don't personally believe that the error conditions need to be disambiguated right-this-instant though. Error messages are probably the easiest thing to expand upon in future iterations.

I disagree that discarding the return value is a viable option here, it's not. At the very minimum those two checks should be performed, and then a false === $wp_update_comment_return should also be added which returns the current error.

38700.wp-error.diff​ is the changed needed to wp_update_comment() so you can disambiguate the errors (and also fixes the incorrect PHPDoc to show how wild the return values are - "0 or 1 on success, 0.. on failure"), if others feel that it's worth pushing ahead here and that it's the most important thing to do, this is the route I'd suggest taking when no compromise is being made. wp_update_comment() really needs a make over. (38700.wp-error.2.diff​ is what the return values should be updated to, but IMHO is too late in the beta cycle)

[jnylen0]

I disagree that discarding the return value is a viable option here, it's not.

Can you elaborate, please? There are no other possible return paths from wp_update_comment, other than those 2 error conditions which are checked and tested in 38700.4.diff​.

Edit: I see it now, I missed that $wpdb->update could return false.

I really don't care how this is fixed, but it's a pretty nasty bug in the API and needs to go in 4.7.

[jnylen0]

Handle $wpdb->update returning false

[jnylen0]

38700.5.diff​ handles the possible false return value from $wpdb->update. Note, this is an additional bugfix: currently we are not catching this error in trunk.

It does seem wise to me to avoid major changes to wp_comment_update this late in the cycle. For example, 38700.wp-error.diff​ contains a behavior change: the function will bail out earlier than before if the database update fails.

[jnylen0]

Refreshed patch; don't include tests for other object types

[jnylen0]

In 38700.6.diff​:

• Comment updates that do not result in any rows updated are OK. Add test for this situation.
• Incidental bugfix: return a better error message for updating a comment with an invalid post ID. Currently it's a generic 500.
• Incidental bugfix: return a 500 if $wpdb->update fails. Currently this is not detected or handled.

The root cause of this ticket (HTTP 500 on a "no-op" comment update) is inconsistent return values from wp_update_comment: 0 can indicate either failure (error) or success (0 rows updated).

We can fix it at the API level by handling both error cases that would cause wp_update_comment to return 0. One is already handled; this patch adds better handling for the other one (invalid post ID).

[jnylen0]

In 39371:

REST API: Add tests for empty or "no-op" updates.

The API should allow updates that don't actually change anything. This allows
clients to, for example, accidentally send the same request twice without
encountering unexpected errors. This currently works for posts, terms, and
users, so this commit adds test cases accordingly.

See #38700 for issues preventing this from working for comments.

Fixes #38975.

[pento]

In 39372:

REST API: Add tests for empty or "no-op" updates.

The API should allow updates that don't actually change anything. This allows clients to, for example, accidentally send the same request twice without encountering unexpected errors. This currently works for posts, terms, and users, so this commit adds test cases accordingly.

See #38700 for issues preventing this from working for comments.

Merge of [39371] to the 4.7 branch.

Props jnylen0.
See #38975.

[jnylen0]

Use an existing string; tweak tests to more closely match r39371

[jnylen0]

In 39597:

REST API: Allow sending an empty or no-op comment update.

In general, updates that don't actually change anything should succeed.
[39371] added tests for other object types, and this commit fixes empty updates
for comments and adds the missing test.

Fixes #38700.

[jnylen0]

Needs to be ported to the 4.7 branch.

[pento]

In 39628:

REST API: Allow sending an empty or no-op comment update.

In general, updates that don't actually change anything should succeed. [39371] added tests for other object types, and this commit fixes empty updates for comments and adds the missing test.

Merges [39597] to the 4.7 branch.

Props jnylen0.
Fixes #38700.

[SergeyBiryukov]

Replying to dd32:

38700.wp-error.diff​ is the changed needed to wp_update_comment() so you can disambiguate the errors (and also fixes the incorrect PHPDoc to show how wild the return values are - "0 or 1 on success, 0.. on failure"), if others feel that it's worth pushing ahead here and that it's the most important thing to do, this is the route I'd suggest taking when no compromise is being made. wp_update_comment() really needs a make over. (38700.wp-error.2.diff​ is what the return values should be updated to, but IMHO is too late in the beta cycle)

Follow-up: #39732

[SergeyBiryukov]

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.