Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#38700 closed defect (bug) (fixed)

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

Reported by: jnylen0's profile jnylen0 Owned by: jnylen0's profile jnylen0
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit fixed-major
Focuses: Cc:

Description

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.

Attachments (9)

38700.diff (5.3 KB) - added by jnylen0 7 years ago.
38700.2.diff (4.9 KB) - added by jnylen0 7 years ago.
Cleaner patch using did_action( 'edit_comment' )
38700.3.diff (5.6 KB) - added by jnylen0 7 years ago.
Use a new wp_update_comment_error filter to work around the underlying issue
38700.4.diff (5.9 KB) - added by jnylen0 7 years ago.
Perform all error checking ourselves, then disregard the return value of wp_update_comment.
38700.wp-error.diff (3.3 KB) - added by dd32 7 years ago.
38700.wp-error.2.diff (3.3 KB) - added by dd32 7 years ago.
38700.5.diff (5.7 KB) - added by jnylen0 7 years ago.
Handle $wpdb->update returning false
38700.6.diff (3.2 KB) - added by jnylen0 7 years ago.
Refreshed patch; don't include tests for other object types
38700.7.diff (3.1 KB) - added by jnylen0 7 years ago.
Use an existing string; tweak tests to more closely match r39371

Download all attachments as: .zip

Change History (37)

@jnylen0
7 years ago

#1 @jnylen0
7 years ago

  • Keywords has-patch has-unit-tests added

@jnylen0
7 years ago

Cleaner patch using did_action( 'edit_comment' )

#2 @rmccue
7 years ago

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?

#3 @jnylen0
7 years ago

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.

#4 @rmccue
7 years ago

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.

#5 @jnylen0
7 years ago

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

Use a new wp_update_comment_error filter to work around the underlying issue

#6 @rmccue
7 years ago

  • Milestone changed from Awaiting Review to 4.7

I like this much better. :)

#7 @rmccue
7 years ago

  • Keywords 4.8-early added
  • Milestone changed from 4.7 to Future Release
  • Owner set to rmccue
  • Status changed from new to assigned

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.

#8 @dd32
7 years ago

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.

#9 @jnylen0
7 years ago

  • Keywords 4.8-early removed

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

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

@dd32
7 years ago

#10 follow-up: @dd32
7 years ago

  • Keywords 4.8-early added

(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)

Last edited 7 years ago by dd32 (previous) (diff)

#11 @jnylen0
7 years ago

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.

Last edited 7 years ago by jnylen0 (previous) (diff)

@jnylen0
7 years ago

Handle $wpdb->update returning false

#12 @jnylen0
7 years ago

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.

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


7 years ago

@jnylen0
7 years ago

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

#14 @jnylen0
7 years ago

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).

#15 @jnylen0
7 years ago

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.

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


7 years ago

#17 @pento
7 years ago

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

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

#18 @jnylen0
7 years ago

  • Keywords 4.8-early removed
  • Milestone changed from Future Release to 4.7.1

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


7 years ago

#20 @jnylen0
7 years ago

  • Owner changed from rmccue to jnylen0
  • Status changed from assigned to accepted

#21 @pento
7 years ago

  • Keywords commit added

#22 @jnylen0
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#23 @jnylen0
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs to be ported to the 4.7 branch.

#24 @pento
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

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


7 years ago

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


4 years ago

#27 in reply to: ↑ 10 @SergeyBiryukov
4 years ago

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

#28 @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.