Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#56712 closed defect (bug) (fixed)

Correct default values in wp_handle_comment_submission()

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.9
Component: Comments Keywords: has-patch php81 has-unit-tests
Focuses: Cc:

Description

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

Attachments (1)

56712.diff (6.4 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (5)

This ticket was mentioned in PR #3387 on WordPress/wordpress-develop by SergeyBiryukov.


21 months ago
#1

  • Keywords has-unit-tests added

#2 @mukesh27
21 months ago

Thanks @SergeyBiryukov

PR #3387 look good.

#3 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 54368:

Code Modernization: Correct default values in wp_handle_comment_submission().

This affects the following parameters subsequently passed to wp_new_comment():

  • comment_author
  • comment_author_email
  • comment_author_url
  • comment_content

The default values for these parameters were previously set to null, causing PHP 8.1 "null to non-nullable" deprecation notices when running sanitization filters on them via wp_filter_comment().

While the deprecation notices were temporarily silenced in the unit test suite, that caused an unexpected issue in a test for submitting a comment to a password protected post, where the $_COOKIE[ 'wp-postpass_' . COOKIEHASH ] value was no longer unset, as the test stopped any further execution once the deprecation notice was triggered.

Due to how WordPress handles password protected posts, once that value is set, it affects all posts protected with the same password, so this resulted in unintentionally affecting another test which happened to use the same password.

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.

This commit 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.

Follow-up to [34799], [34801], [51968].

Props jrf, desrosj, mukesh27, SergeyBiryukov.
Fixes #56712. See #56681, #55656.

SergeyBiryukov commented on PR #3387:


21 months ago
#4

Merged in r54368.

Note: See TracTickets for help on using tickets.