[wp-trac] [WordPress Trac] #36901: Removing wp_die() from wp_allow_comment()

[WordPress Trac]

#36901: Removing wp_die() from wp_allow_comment()
----------------------------------------+-----------------------------
 Reporter:  websupporter                |       Owner:
     Type:  enhancement                 |      Status:  new
 Priority:  normal                      |   Milestone:  Future Release
Component:  Comments                    |     Version:  trunk
 Severity:  normal                      |  Resolution:
 Keywords:  has-unit-tests needs-patch  |     Focuses:
----------------------------------------+-----------------------------

Comment (by websupporter):

 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:

 {{{#!php
 $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 :)

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