WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 10 months ago

Last modified 9 months ago

#38700 closed defect (bug) (fixed)

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

Reported by: jnylen0 Owned by: 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 12 months ago.
38700.2.diff (4.9 KB) - added by jnylen0 12 months ago.
Cleaner patch using did_action( 'edit_comment' )
38700.3.diff (5.6 KB) - added by jnylen0 12 months ago.
Use a new wp_update_comment_error filter to work around the underlying issue
38700.4.diff (5.9 KB) - added by jnylen0 12 months 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 12 months ago.
38700.wp-error.2.diff (3.3 KB) - added by dd32 12 months ago.
38700.5.diff (5.7 KB) - added by jnylen0 12 months ago.
Handle $wpdb->update returning false
38700.6.diff (3.2 KB) - added by jnylen0 11 months ago.
Refreshed patch; don't include tests for other object types
38700.7.diff (3.1 KB) - added by jnylen0 11 months ago.
Use an existing string; tweak tests to more closely match r39371

Download all attachments as: .zip

Change History (34)

@jnylen0
12 months ago

#1 @jnylen0
12 months ago

  • Keywords has-patch has-unit-tests added

@jnylen0
12 months ago

Cleaner patch using did_action( 'edit_comment' )

#2 @rmccue
12 months 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
12 months 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
12 months 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
12 months 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
12 months ago

Use a new wp_update_comment_error filter to work around the underlying issue

#6 @rmccue
12 months ago

  • Milestone changed from Awaiting Review to 4.7

I like this much better. :)

#7 @rmccue
12 months 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
12 months 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
12 months 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
12 months ago

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

#10 @dd32
12 months 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 12 months ago by dd32 (previous) (diff)

#11 @jnylen0
12 months 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 12 months ago by jnylen0 (previous) (diff)

@jnylen0
12 months ago

Handle $wpdb->update returning false

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


12 months ago

@jnylen0
11 months ago

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

#14 @jnylen0
11 months 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
11 months 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.


11 months ago

#17 @pento
11 months 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
11 months ago

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

#18 @jnylen0
11 months 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.


11 months ago

#20 @jnylen0
11 months ago

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

#21 @pento
11 months ago

  • Keywords commit added

#22 @jnylen0
10 months 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
10 months 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
10 months 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.


9 months ago

Note: See TracTickets for help on using tickets.