[wp-trac] [WordPress Trac] #56712: Correct default values in wp_handle_comment_submission()

[WordPress Trac]

#56712: Correct default values in wp_handle_comment_submission()
----------------------------+-----------------------------
 Reporter:  SergeyBiryukov  |      Owner:  (none)
     Type:  defect (bug)    |     Status:  new
 Priority:  normal          |  Milestone:  6.1
Component:  Comments        |    Version:  5.9
 Severity:  normal          |   Keywords:  has-patch php81
  Focuses:                  |
----------------------------+-----------------------------
 Background: #34059, #56681.

 This came up when looking into the last remaining test failure on PHP 8.1:
 {{{
 1) WP_Test_REST_Posts_Controller::test_get_post_draft_edit_context
 Failed asserting that '<ul class="wp-block-latest-posts__list wp-block-
 latest-posts"><li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3701">Protected: Hola</a><div class="wp-block-
 latest-posts__post-excerpt">Hello World!</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3601">Post 28</a><div class="wp-block-latest-
 posts__post-excerpt">Post excerpt 0005016</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3600">Post 27</a><div class="wp-block-latest-
 posts__post-excerpt">Post excerpt 0005015</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3599">Post 26</a><div class="wp-block-latest-
 posts__post-excerpt">Post excerpt 0005014</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3598">Post 25</a><div class="wp-block-latest-
 posts__post-excerpt">Post excerpt 0005013</div></li>\n
 </ul> <ul class="wp-block-latest-posts__list wp-block-latest-posts"><li><a
 class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3701">Protected: Hola</a><div class="wp-block-
 latest-posts__post-full-content">Hello World!</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3601">Post 28</a><div class="wp-block-latest-
 posts__post-full-content">Post content 0005016</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3600">Post 27</a><div class="wp-block-latest-
 posts__post-full-content">Post content 0005015</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3599">Post 26</a><div class="wp-block-latest-
 posts__post-full-content">Post content 0005014</div></li>\n
 <li><a class="wp-block-latest-posts__post-title"
 href="http://example.org/?p=3598">Post 25</a><div class="wp-block-latest-
 posts__post-full-content">Post content 0005013</div></li>\n
 </ul>' does not contain "Hello World!".

 /var/www/tests/phpunit/tests/rest-api/rest-posts-controller.php:1980
 /var/www/vendor/bin/phpunit:123
 }}}

 The test was added in [50717] and has to do with allowing authenticated
 users to read the contents of password protected posts if they have the
 `edit_post` meta capability for the post. While the test could use some
 improvement, as noted in comment:3:ticket:56681, it fails when running the
 full test suite on PHP 8.1 but passes when run in isolation, which
 indicates that it could be affected by some other test.

 After some investigation, the culprit was found:
 [source:tags/6.0.2/tests/phpunit/tests/comment-submission.php#L202
 test_submitting_comment_to_password_protected_post_succeeds()], which
 temporarily assigns the `$_COOKIE[ 'wp-postpass_' . COOKIEHASH ]` value.
 Due to how WordPress handles password protected posts, once that value is
 set, it affects all posts protected with the same password, see #16483.
 Both tests in question happen to use `password` as the password value.

 While the latter test unsets the `$_COOKIE[ 'wp-postpass_' . COOKIEHASH ]`
 value afterwards, the problem is that the test itself also triggers a PHP
 8.1 "null to non-nullable" deprecation notice that was temporarily
 silenced in [51968]. After some debugging it became apparent that once
 that notice is triggered, the test stops any further execution,
 unintentionally leaving the `$_COOKIE[ 'wp-postpass_' . COOKIEHASH ]` in
 place, which then affects the other test.

 Looking into the source of that deprecation notice, it is caused by the
 defaults in `wp_handle_comment_submission()`:
 {{{
 $comment_author       = null;
 $comment_author_email = null;
 $comment_author_url   = null;
 $comment_content      = null;
 }}}
 These values are all documented to be a string in various related filters,
 and core also expects them to be a string, so there is no reason for these
 defaults to be set to `null`. Setting them to an empty string instead
 resolves the issues:
 * The latter test can proceed as expected and unset the cookie value so
 that it does not affect the former test.
 * It allows us to remove six stopgap conditionals added in [51968] to
 silence the deprecation notices.

 The patch includes:
 * Setting the defaults in `wp_handle_comment_submission()` to an empty
 string.
 * Adding a dedicated unit test to verify the type of these default values.
 * Removing the deprecation notice silencing as no longer needed.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56712>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform