Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#36901 closed enhancement (fixed)

Removing wp_die() from wp_allow_comment()

Reported by: websupporter's profile websupporter Owned by: boonebgorges's profile boonebgorges
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Comments Keywords: has-patch needs-testing has-unit-tests has-dev-note
Focuses: Cc:

Description

Ticket #34059 introduced wp_handle_comment_post() to abstract the whole process of comment submission "in order to make the actual process of submitting a comment unit testable."

I want to propose, to extend this a bit more and to remove the wp_die() functionality, when it comes to duplicate comments and comment flood. Instead of wp_die(), we should return WP_Error objects, indicating a comment is duplicate or is considered comment flood. These error objects would travel upwards until they are handeled in wp-comments-post.php.

The advantage of this would be, we could use wp_allow_comment() to check a comment for duplicate and flood. This could be utilized in the REST API, where we could in case of a flood or a duplicate return a valid JSON-String instead of the wp_die()-HTML.

Attachments (12)

36901.patch (7.3 KB) - added by websupporter 8 years ago.
This patch is supposed to solve this problem. I am not quite happy about some of the structures and have som questions, so for example, why do we needed to hook check_comment_flood_db() and should we still, but it gives a start. I've also added some unit tests, since we could now test for duplicate comments and flood.
36901.2.patch (11.6 KB) - added by websupporter 8 years ago.
36901.diff (9.5 KB) - added by rachelbaker 7 years ago.
Use $avoid_die parameter and attempt to simplify the logic by using check_comment_flood_db() BUT unhooking it's action
36901.2.diff (9.8 KB) - added by boonebgorges 7 years ago.
36901.3.diff (9.9 KB) - added by websupporter 7 years ago.
36901.4.diff (10.7 KB) - added by rachelbaker 7 years ago.
Removes the check_comment action from check_comment_flood_db and includes minor docs and alignment tweaks.
36901.5.diff (10.6 KB) - added by rachelbaker 7 years ago.
Re-adds check_comment_flood action
36901.6.diff (10.6 KB) - added by rachelbaker 7 years ago.
Fixes whether typo, otherwise same as 36901.5.diff
36901.7.diff (11.4 KB) - added by rachelbaker 7 years ago.
Same as 36901.6.diff with correction to parameters for the check_comment action.
36901.8.diff (11.4 KB) - added by websupporter 7 years ago.
Doctype fix for wp_check_comment_flood()
36901.9.diff (12.0 KB) - added by websupporter 7 years ago.
Takes https://core.trac.wordpress.org/ticket/36901#comment:26 into account
36901.10.diff (1.1 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (57)

@websupporter
8 years ago

This patch is supposed to solve this problem. I am not quite happy about some of the structures and have som questions, so for example, why do we needed to hook check_comment_flood_db() and should we still, but it gives a start. I've also added some unit tests, since we could now test for duplicate comments and flood.

#1 @websupporter
8 years ago

  • Keywords has-patch dev-feedback needs-testing has-unit-tests added

This ticket was mentioned in Slack in #core-comments by websupporter. View the logs.


8 years ago

#3 @boonebgorges
8 years ago

  • Keywords needs-patch added; has-patch dev-feedback needs-testing removed
  • Milestone changed from Awaiting Review to Future Release

+1 to the general idea of making these functions more testable and flexible.

As noted in the Slack discussion linked above, 36901.patch introduces a number of backward compatibility breaks that we should try to avoid. First, the current behavior of check_comment() - and thus of calling functions like wp_allow_comment() and wp_new_comment() - is to die. This default behavior probably cannot safely be changed, at least not without extensive research into who (outside of core) is using the function, and how.

Additionally, we cannot change the function signature of check_comment_flood_ids(). This will break all existing uses of the function.

A different strategy would be to introduce a new parameter to wp_allow_comment(), check_comment(), and related functions. This parameter would be something like $return_error, and would default to false. If true, it would return a WP_Error instead of dieing. We may then decide to refactor wp_new_comment() and friends so that die behavior is handled in wp-comments-post.php. But this could be a separate project from adding the new parameter, and the new tests that the param would enable, etc.

A brief note that it's possible to work around some of these problems currently by registering a custom wp_die_handler.

This ticket was mentioned in Slack in #core-comments by websupporter. View the logs.


8 years ago

#5 @websupporter
8 years ago

I have updated my patch. So, what it does now:

  1. If wp_handle_comment_submission() detects no problem, it runs as usual wp_new_comment(). wp_new_comment() accepts a second parameter $return_error, which is set to false by default. wp_handle_comment_submission() runs the function with $return_error set to true.
  2. Basically wp_new_comment() passes $return_error simply to wp_allow_comment() and checks the result, if it is a WP_Error.
  3. So wp_allow_comment() also accepts a second parameter $return_error. Standard is false. This function checks, if the comment is a duplicate and returns a WP_Error if this is the case or executes wp_die() in case $return_error is set to false.
  4. After this check, the usual action check_comment_flood is triggered. But check_comment_flood_db() is no longer hooked into this action. But still, I thought it might be useful to add the $return_error here to let functions, which might be hooked in, know if wp_die() is the expected behavior in case of a flood.
  5. Now the new filter is_comment_flood runs. Here we expect a boolean in return indicating the flood. In addition to this boolean comment_author_IP, comment_author_email and comment_date_gmt as well as $return_error are available as parameter.
  6. I've wrote a wrapper function for this filter: check_comment_flood_db_filter(). Basically it just calls the old check_comment_flood_db() which also accepts now $return_error. In case $return_error is set true check_comment_flood_db() will return a boolean. Otherwise it will execute wp_die() or return nothing (the old behavior).
  7. In case $return_error is true and it is a flood, wp_allow_comment() will return an WP_Error. If $return_error is false nothing will happen since we should already be dead.
  8. Now wp_allow_comment() runs as usual checks the comment with check_comment() and so on, but here no further action seems necessary.
  9. In case of $return_error and duplicate comment or flood, the function has returned the corresponding WP_Error, which will be simply returned by wp_new_comment(). The usual behavior of wp_new_comment() is to die, to return false or the comment ID. Since wp_handle_comment_submission() called wp_new_comment() with $return_error true now we can also get an error object. So wp_handle_comment_submission() checks for the error and in this case wp_handle_comment_submission() simply returns the error.

Thanks a lot for the feedback I've received for my patch. It helped me a lot!

First, the current behavior of check_comment() - and thus of calling functions like wp_allow_comment() and wp_new_comment() - is to die

Just quickly: check_comment() does not die, it simply checks if a comment needs moderation and so on depending on get_option('comment_moderation'), get_option( 'comment_max_links' ) et.al. Two filters are running in the function: comment_text and comment_max_links_url, but none of them seem to run into a wp_die(). So as far as I see it the way to die is wp_new_comment() > wp_allow_comment().

we cannot change the function signature of check_comment_flood_ids()

I think check_comment_flood_db() is meant. My first approach was to turn this function itself to a filter, we discussed it a bit in Slack and the suggestion here was to write a wrapper function, which would be check_comment_flood_db_filter() (well, the naming indicates already, it is getting a bit weired here). I think the flood is the weirdest part with all its filters, actions and stuff.
But basically, if lets say a plugin calls check_comment_flood_db() the old way, $return_error is set to false and the function would either die or return nothing. But still, for us, it would return a boolean indicating if the comment is part of the flood.

Basically, as far as I can see it, if we would start wp_new_comment( $commentdata ) the behavior of all functions are like they are today. I checked it also with my phpunit tests. In case I run in wp_handle_comment_submission() the wp_new_comment() with false we die(). Otherwise we get our WP Error objects.

Another question in slack was by @dshanske:

Why return 409 and 429 though?
I understand the status code, but you are passing it as data.
Shouldn't it pass data about the duplicate?

https://wordpress.slack.com/archives/core-comments/p1463890408000435

I just oriented myself on how wp_handle_comment_submission() creates its error objects. Here it is done the same way, which later on in wp-comments-posts.php leads to this check:

$comment = wp_handle_comment_submission( wp_unslash( $_POST ) );
if ( is_wp_error( $comment ) ) {
        $data = intval( $comment->get_error_data() );
        if ( ! empty( $data ) ) {
                wp_die( '<p>' . $comment->get_error_message() . '</p>', __( 'Comment Submission Failure' ), array( 'response' => $data, 'back_link' => true ) );
        } else {
                exit;
        }
}

(Well, if you just hop into this ticket: Yes it dies :D )

If we want to change it, we would need to alter also wp_handle_comment_submission() and wp-comments-posts.php. I wouldn't mind, but can't evaluate the advantage.

And the status code set upstream?

I am not quite sure, if this means to set the Response Header before wp-comments-posts.php I think, this would be too early.

Last point: I understood @dshanske in the way a function which is hooked into an action needs to remain there for backwards compatibility, but I guess, I just misunderstood him. If this would be the case, the patch would become really awkward (I mean, even more!). I tried here to follow @boones suggestions:

The overall default behavior of wp_new_comment() should not change. That is, if the flood check fails, it should result in a wp_die() (at least for the purposes of this ticket). How that's implemented internally is less important, I think

I hope, this patch comes closer to this goal. Good news: In the end we all die but we are getting older now. I am happy about every feedback :)

#6 @boonebgorges
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I won't have time to go in-depth on this for a while, but at a glance, a big +1. This is exactly the strategy I had in mind.

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


8 years ago

This ticket was mentioned in Slack in #core-comments by websupporter. View the logs.


8 years ago

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


8 years ago

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


8 years ago

#11 @rachelbaker
8 years ago

  • Milestone changed from Future Release to 4.7
  • Owner set to rachelbaker
  • Status changed from new to assigned

Would love to get this into 4.7 to resolve a REST API blocker. Assigned to myself to review sometime this week.

#12 @rachelbaker
7 years ago

@websupporter Thank you for your work on this ticket. The meaning of $return_error seems to get muddy in a few places and is often used as a parameter in functions (like in check_comment_flood_db()) where they aren't expected to return a WP_Error at all. What do you think about using a parameter that signals executions of wp_die should be avoided instead? Maybe $avoid_die? This would allow us to still return WP_Error objects up the function chain while avoiding mis-leading variables. Thoughts?

@rachelbaker
7 years ago

Use $avoid_die parameter and attempt to simplify the logic by using check_comment_flood_db() BUT unhooking it's action

#13 @rachelbaker
7 years ago

36901.diff is a riff off of 36901.2.patch. Changes include:

  • Changed the $return_error variable to $avoid_die, still defaulting to false.
  • Removed the check_comment_flood_db() function from the check_comment_flood action. Still keeping the do_action( 'check_comment' ) in wp_allow_comment() for back-compat.
  • Call the check_comment_flood_db() function directly from within wp_allow_comment() instead of adding a wrapper function or filter. @boonebgorges any back-compat concerns here that I am missed?
  • Added missing @since tags.
  • Added missing @return types of WP_Error where possible.

#14 @websupporter
7 years ago

Hi Rachel,
just downloaded and tested your patch. It worked as expected. I was running in duplicate and flood when I was supposed to and the error travelled down to wp_handle_comment_submission().

The three tests worked also as expected.

$avoid_die is much better in my eyes and I like the usage of wp_doing_ajax() since we now have this function!

If we can avoid check_comment_flood_db() in the action (or what I did with a new filter is_comment_flood and check_comment_flood_db_filter()) I would be very happy.

Thanks a lot for your suggestions and your testing. For me, the current patch is much better :)

#15 @websupporter
7 years ago

Finally I found the reason why check_comment_flood_db() is not directly called:

Make the entire comment flood check pluggable as it can cause load problems on large sites.

https://core.trac.wordpress.org/changeset/5947

Unfortunatly there is no ticket associated and I couldn't find one searching these tickets: https://core.trac.wordpress.org/query?version=2.3&resolution=fixed

So the basic idea seems to be, nine years ago large sites did run into problems.

Why did they run into problems?
I do not understand why, is it the database request back then?

SELECT comment_date_gmt FROM $wpdb->comments WHERE comment_author_IP = '$ip' OR comment_author_email = '$email' ORDER BY comment_date DESC LIMIT 1

Might it already been resolved now?
SELECT comment_date_gmt FROM $wpdb->comments WHERE comment_date_gmt >= %s AND ( $check_column = %s OR comment_author_email = %s ) ORDER BY comment_date_gmt DESC LIMIT 1

As far as I understand currently the db columns comment_author_email and comment_date_gmt (besides others) are indexed. Back than it seems like only comment_ID, comment_approved and comment_post_ID where indexed (see https://core.trac.wordpress.org/browser/branches/2.3/wp-admin/includes/schema.php#L53). Maybe this meant the queries took long to execute and its ok now?

Why as action hook?
The basic idea of the action seems to be, so large sites can dehook the check (and hook into with their own checks).

Any thoughts on this?

@boonebgorges
7 years ago

#16 @boonebgorges
7 years ago

Call the check_comment_flood_db() function directly from within wp_allow_comment() instead of adding a wrapper function or filter. @boonebgorges any back-compat concerns here that I am missed?

Yes, I think so. I've done the following on client sites:

remove_action( 'check_comment_flood', 'check_comment_flood_db' );
add_action( 'check_comment_flood', 'my_own_version' );

Pretty much what @websupporter lays out in his last comment. (BTW, thanks for digging into the logs :) )

36901.2.diff is an ugly attempt at back compat for this usage, but I think it works. Briefly: we continue to add_action( 'check_comment_flood', 'check_comment_flood_db' );. But check_comment_flood_db() is now just a wrapper for an add_filter() call, which attaches the *actual* comment flood checking function to the new wp_is_comment_flood filter. (Phew.)

@websupporter
7 years ago

#17 @websupporter
7 years ago

This patch modifies 36901.2.diff.

Two problems I had with 36901.2:

  1. wp_is_comment_flood needed to filter the boolean $is_flood but actually filtered the authors IP. (L668)
  1. It still executed check_commend_flood_db() in wp_allow_comment() although its hooked in. (L676)

Just wanted to add this quickly, so we have a working patch.

I see how this would be BC now :)

#18 @boonebgorges
7 years ago

@websupporter Thank you for the patch - as you can see, I was sketching an idea that I hadn't really tested :-D

@rachelbaker
7 years ago

Removes the check_comment action from check_comment_flood_db and includes minor docs and alignment tweaks.

#19 @rachelbaker
7 years ago

@websupporter
In testing your 36901.3.diff because the check_comment action was still running check_comment_flood_db the wp_check_comment_flood() function was running twice. I removed the action hook in 36901.4.diff and included some minor doc fixes as well.

Can you give the latest patch a sanity check to make sure I didn't miss anything?

#20 @mdgl
7 years ago

Visiting from #18630 I think this patch is moving along the right sort of lines. The latest version, however appears to break backwards compatibility with approaches such as that mentioned in comment:16. Developers will expect to be able to unhook check_comment_flood_db() and attach their own function to action check_comment_flood to customise this behaviour. Furthermore, the implied double negative in the naming of $avoid_die is rather confusing and would probably be better specified in the positive. My earlier patch on #18630 followed the convention established by wp_insert_post() and wp_set_comment_status() although I agree this is still a bit ugly and that the overall code base isn't very consistent in this regard. The new patch also has a minor typo in the comments: "wheter" which should be "whether".

@rachelbaker
7 years ago

Re-adds check_comment_flood action

#21 @rachelbaker
7 years ago

No clue what I was doing yesterday, but just ignore 36901.4.diff. In 36901.5.diff I fixed the back-compat with the check_comment_flood action that I keep breaking (doh!) and also include the $avoid_die param for the check_comment_flood action.

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


7 years ago

#23 @websupporter
7 years ago

I checked 36901.5 and it looks good to me. The typo mentioned by @mdgl is also fixed (sorry @mdgl, I was looking for a ticket concerning this problem, but I didn't find the one, where you were engaged in).

I don't mind if we stick to $avoid_die. It's compelling to use $wp_error = true in order to get something like a standard but on the other hand for example in wp_check_comment_flood() we die or return true, so $wp_error would be misleading here and we would need another name. With $avoid_die we have one name running through the whole process. The name also focuses on what we want to do: avoid to die.

@rachelbaker
7 years ago

Fixes whether typo, otherwise same as 36901.5.diff

#24 @rachelbaker
7 years ago

  • Owner changed from rachelbaker to websupporter
  • Status changed from assigned to reviewing

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


7 years ago

#26 follow-up: @mdgl
7 years ago

The new patch (.6) looks as though it will do the job, but I don't see why we need to pass the $avoid_die [yech!] parameter into the (legacy) check_comment_flood action since there is no way for the hooked function to return anything. In any case, this action is declared with only three parameters in default-filters.php.

The other backwards-compatibility issue I can think of is that now calling function check_comment_flood_db() will effectively do nothing, whereas previously it actually checked for a comment flood and "died" if one was detected. Developers may be relying on that behaviour.

#27 @rachelbaker
7 years ago

@mdgl Thank you for the feedback. My thought behind adding the $avoid_die parameter to check_comment is allowing functions that hook into the action to know if the use of wp_die() is expected. I am open to the idea that I am overthinking this and trying to cover a potential use-case that doesn't exist.

Thank you for catching the missing parameter change needed on default-filters.php.

@rachelbaker
7 years ago

Same as 36901.6.diff with correction to parameters for the check_comment action.

#28 @mdgl
7 years ago

I thought the idea was to effectively replace the (legacy) check_comment_flood action with the (new) wp_is_comment_flood filter. If this is the case, I wouldn't personally bother with updating the older action - if you want to "avoid dying", just use the new filter instead.

#29 @websupporter
7 years ago

I think we can't just get rid of the check_comment_flood action and the hooked in function check_comment_flood_db() as a lot of installations unhook this. So Boone suggested:

check_comment_flood_db() is now just a wrapper for an add_filter() call, which attaches the *actual* comment flood checking function to the new wp_is_comment_flood filter. (Phew.)

I think this whole wp_allow_comment() has actually a lot of Phews :D

I wouldn't personally bother with updating the older action.

This was my attempt in 36901.2.patch. But this implies to get rid of the action and simply replace it with a filter. When I read the patch now, I don't remember why I wrapped the check_comment_flood_db() into the check_comment_flood_db_filter() back than, but seems not to be important for the point here :)

The other backwards-compatibility issue I can think of is that now calling function check_comment_flood_db() will effectively do nothing, whereas previously it actually checked for a comment flood and "died" if one was detected. Developers may be relying on that behaviour.

Hmm... If this is an issue we could give check_comment_flood_db() again its parameters and add the $avoid_die. If $avoid_die is true we could add the filter and with false we call wp_check_comment_flood() directly... (Phew ;P)

Will upload two patches. the first one just fixes the @return doc of wp_check_comment_flood() (Does not return a WP_Error). The 2nd will extend check_comment_flood_db(). I seperate those, because maybe someone has a better idea for the 2nd patch or it's not an issue at all (or not such an big issue, maybe... ?)

@websupporter
7 years ago

Doctype fix for wp_check_comment_flood()

#30 in reply to: ↑ 26 @rachelbaker
7 years ago

Replying to mdgl:

The other backwards-compatibility issue I can think of is that now calling function check_comment_flood_db() will effectively do nothing, whereas previously it actually checked for a comment flood and "died" if one was detected. Developers may be relying on that behaviour.

check_comment_flood_db() is never called directly by WordPress itself. We can check the plugin repo to see if/how many are directly calling that function. I think this change does warrant a post on Make Core to communicate the change.

cc @websupporter

#31 @rachelbaker
7 years ago

  • Keywords needs-dev-note added

#32 @rachelbaker
7 years ago

@boonebgorges Any concerns with 36901.8.diff? Otherwise, I would like to get this committed so we can make the appropriate changes in the REST API comment endpoints.

#34 @boonebgorges
7 years ago

Very anecdotal, but I can't find a single reference on GitHub of check_comment_flood_db() being called directly: https://github.com/search?p=1&q=check_comment_flood_db&type=Code&utf8=%E2%9C%93 I'll search a copy of the plugin repo when I'm near a computer that has a copy handy.

That said, the trick in 36901.9.diff seems pretty clever to me. Respecting $allow_die in check_comment_flood_db() doesn't seem totally necessary to me, though. How about something like this?

do_action( 
    'check_comment_flood', 
    $commentdata['comment_author_IP'], 
    $commentdata['comment_author_email'], 
    $commentdata['comment_date_gmt'] 
    $commentdata['comment_date_gmt'], 
    $avoid_die,
    false
);

...
add_action( 'check_comment_flood',      'check_comment_flood_db',       10, 5 );

...

function check_comment_flood_db( $ip, $email, $date, $avoid_die, $check_flood_now = true ) { 
 	if ( $check_flood_now ) { 
 	    // We run the comment flood check and die in case of a flood. 
 	    wp_check_comment_flood( false, $ip, $email, $date, $avoid_die );
....  

This'll make it possible to call check_comment_flood_db() as an alias for wp_check_comment_flood() even when $allow_die is *false*. I'm not sure there's a real use case for this (why not just call wp_check_comment_flood()?) but it seems a bit more organized to me.

This is just a thought, though. I'm comfortable with either 36901.8.diff or 36901.9.diff as well.

Rest of the patch reads OK to me. @rachelbaker when you're happy with the approach, you've got the go-ahead from me.

This ticket was mentioned in Slack in #core-comments by boone. View the logs.


7 years ago

#36 @boonebgorges
7 years ago

  • Owner changed from websupporter to boonebgorges
  • Status changed from reviewing to accepted

I've done a search of the wordpress.org plugin repo, and I didn't find a single instance of check_comment_flood_db() being called directly. It's always in the context of remove_filter() etc, which will continue to work without the added tricks in 36901.9.diff. So I'm going to go with the simpler approach of 36901.8.diff.

I'll write a make/core post that explains the issue.

Also, after discussion with @rachelbaker in Slack (linked above), I'll be removing the HTML status code being passed as data to the WP_Error objects. The parallel with wp_handle_comment_submission() is not entirely accurate; error objects in that function need status codes for certain backward compatibility reasons. The error string ('comment_duplicate', etc) contains enough information to differentiate the nature of the error, and the consumer of the error can decide which HTML status code it wants to use when handling the error (if HTML status codes are even relevant for the consumer).

#37 @boonebgorges
7 years ago

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

In 38778:

Comments: Abstract die() calls from comment submission routine.

Since 4.4, comment submission has been mostly abstracted into a function,
rather than being processed inline in wp-comments-post.php. This change
made it easier to write automated tests against the bulk of the comment
submission process. wp_allow_comment() remained untestable, however:
when a comment failed one of its checks (flooding, duplicates, etc),
die() or wp_die() would be called directly. This shortcoming posed
problems for any application attempting to use WP's comment verification
functions in an abstract way - from PHPUnit to the REST API.

The current changeset introduces a new parameter, $avoid_die, to the
wp_new_comment() stack. When set to true, wp_new_comment() and
wp_allow_comment() will return WP_Error objects when a comment check
fails. When set to false - the default, for backward compatibility -
a failed check will result in a die() or wp_die(), as appropriate.

Prior to this changeset, default comment flood checks took place in the
function check_comment_flood_db(), which was hooked to the
'check_comment_flood' action. This design allowed the default comment
flood routine to be bypassed or replaced using remove_action().
In order to maintain backward compatibility with this usage, while
simultaneously converting the comment flood logic into something that
returns a value rather than calling die() directly,
check_comment_flood_db() has been changed into a wrapper function for
a call to add_filter(); this, in turn, adds the *actual* comment flood
check to a new filter, 'wp_is_comment_flood'. Note that direct calls
to check_comment_flood_db() will no longer do anything in isolation.

Props websupporter, rachelbaker.
Fixes #36901.

#39 @boonebgorges
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@needle reports on make/core https://make.wordpress.org/core/2016/10/11/comment-allowed-checks-in-wordpress-4-7/#comment-31324 that [38778] results in the incorrect status codes being returned in some cases. This is my fault - I thought that the codes in @websupporter's patches were superfluous, but I was incorrect. A new patch is coming up.

#40 follow-up: @boonebgorges
7 years ago

36901.10.diff restores the previous status codes for comment floods and duplicates.

For the record, the issue here is that wp-comments-post.php uses the data from the error object to send a status header: https://core.trac.wordpress.org/browser/tags/4.6.1/src/wp-comments-post.php?marks=24#L20 Previously, I overlooked this.

Could I get a sanity check from @needle or anyone else who can reproduce?

#41 @websupporter
7 years ago

Works for me. I get the 409 as HTTP status code.

#42 in reply to: ↑ 40 @needle
7 years ago

Replying to boonebgorges:

36901.10.diff restores the previous status codes for comment floods and duplicates.

Could I get a sanity check from @needle or anyone else who can reproduce?

Thanks @boonebgorges that fixes it for me.

#43 @boonebgorges
7 years ago

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

In 38783:

Comments: When checking comments, returned error object should include HTTP status code.

The status code in the WP_Error data array is needed to send
headers in wp-comments-post.php, and was erroneously not included in
[38778].

Props needle, websupporter.
Fixes #36901.

#44 @johnbillion
7 years ago

In 39045:

XML-RPC: Correctly handle empty and duplicate comments.

This prevents wp_die() being sent in response to an XML-RPC call that attempts to submit a duplicate comment, and correctly returns an error in response to an attempt to submit an empty comment.

Props markoheijnen, websupporter.
Fixes #14452, #38466.
See #36901

#45 @desrosj
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.