Failing unit tests on PHP 8.1 and 8.2

[desrosj]

There are currently 2 unit tests that are failing when run on PHP 8.1 and 8.2. From an ​example workflow run:

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

2) Tests_Theme_wpGetGlobalStylesheet::test_variables_in_classic_theme_with_presets_using_variables
small font size is 18px
Failed asserting that 'body{--wp--preset--color--black: #000000;--wp--preset--color--cyan-bluish-gray: #abb8c3;--wp--preset--color--white: #ffffff;--wp--preset--color--pale-pink: #f78da7;--wp--preset--color--vivid-red: #cf2e2e;--wp--preset--color--luminous-vivid-orange: #ff6900;--wp--preset--color--luminous-vivid-amber: #fcb900;--wp--preset--color--light-green-cyan: #7bdcb5;--wp--preset--color--vivid-green-cyan: #00d084;--wp--preset--color--pale-cyan-blue: #8ed1fc;--wp--preset--color--vivid-cyan-blue: #0693e3;--wp--preset--color--vivid-purple: #9b51e0;--wp--preset--color--light: #f5f7f9;--wp--preset--color--dark: #000;--wp--preset--gradient--vivid-cyan-blue-to-vivid-purple: linear-gradient(135deg,rgba(6,147,227,1) 0%,rgb(155,81,224) 100%);--wp--preset--gradient--light-green-cyan-to-vivid-green-cyan: linear-gradient(135deg,rgb(122,220,180) 0%,rgb(0,208,130) 100%);--wp--preset--gradient--luminous-vivid-amber-to-luminous-vivid-orange: linear-gradient(135deg,rgba(252,185,0,1) 0%,rgba(255,105,0,1) 100%);--wp--preset--gradient--luminous-vivid-orange-to-vivid-red: linear-gradient(135deg,rgba(255,105,0,1) 0%,rgb(207,46,46) 100%);--wp--preset--gradient--very-light-gray-to-cyan-bluish-gray: linear-gradient(135deg,rgb(238,238,238) 0%,rgb(169,184,195) 100%);--wp--preset--gradient--cool-to-warm-spectrum: linear-gradient(135deg,rgb(74,234,220) 0%,rgb(151,120,209) 20%,rgb(207,42,186) 40%,rgb(238,44,130) 60%,rgb(251,105,98) 80%,rgb(254,248,76) 100%);--wp--preset--gradient--blush-light-purple: linear-gradient(135deg,rgb(255,206,236) 0%,rgb(152,150,240) 100%);--wp--preset--gradient--blush-bordeaux: linear-gradient(135deg,rgb(254,205,165) 0%,rgb(254,45,45) 50%,rgb(107,0,62) 100%);--wp--preset--gradient--luminous-dusk: linear-gradient(135deg,rgb(255,203,112) 0%,rgb(199,81,192) 50%,rgb(65,88,208) 100%);--wp--preset--gradient--pale-ocean: linear-gradient(135deg,rgb(255,245,203) 0%,rgb(182,227,212) 50%,rgb(51,167,181) 100%);--wp--preset--gradient--electric-grass: linear-gradient(135deg,rgb(202,248,128) 0%,rgb(113,206,126) 100%);--wp--preset--gradient--midnight: linear-gradient(135deg,rgb(2,3,129) 0%,rgb(40,116,252) 100%);--wp--preset--gradient--custom-gradient: linear-gradient(135deg,rgba(0,0,0) 0%,rgb(0,0,0) 100%);--wp--preset--duotone--dark-grayscale: url('#wp-duotone-dark-grayscale');--wp--preset--duotone--grayscale: url('#wp-duotone-grayscale');--wp--preset--duotone--purple-yellow: url('#wp-duotone-purple-yellow');--wp--preset--duotone--blue-red: url('#wp-duotone-blue-red');--wp--preset--duotone--midnight: url('#wp-duotone-midnight');--wp--preset--duotone--magenta-yellow: url('#wp-duotone-magenta-yellow');--wp--preset--duotone--purple-green: url('#wp-duotone-purple-green');--wp--preset--duotone--blue-orange: url('#wp-duotone-blue-orange');--wp--preset--font-size--small: 13px;--wp--preset--font-size--medium: 20px;--wp--preset--font-size--large: 36px;--wp--preset--font-size--x-large: 42px;--wp--preset--font-size--custom: 100px;--wp--preset--spacing--20: 0.44rem;--wp--preset--spacing--30: 0.67rem;--wp--preset--spacing--40: 1rem;--wp--preset--spacing--50: 1.5rem;--wp--preset--spacing--60: 2.25rem;--wp--preset--spacing--70: 3.38rem;--wp--preset--spacing--80: 5.06rem;}p{--wp--preset--color--light: #f5f7f9;}' contains "--wp--preset--font-size--small: 18px".

/var/www/tests/phpunit/tests/theme/wpGetGlobalStylesheet.php:153

This is not apparent because these two jobs are allowed to fail due to deprecation warnings still being worked through on the road to full PHP 8.0-8.2 compatibility.

I've tried to track down the exact commit where the failures started happening, but GitHub Action logs only persist for 90 days.

These should be investigated and understood before 6.1 to confirm sites with these PHP versions won't have problems.

[desrosj]

It looks like the test_variables_in_classic_theme_with_presets_using_variables() failure was introduced in [54162].

• ​Associated workflow with the failure.
• ​Workflow for the immediately preceding commit without this failure.

@hellofromTonya could you investigate to see if anything sticks out? Is this maybe related to an incomplete revert in [54160]?

[jrf]

I came here to open a ticket about the WP_Test_REST_Posts_Controller::test_get_post_draft_edit_context() test, only to find you already opened this ticket. 😁

I have investigated the WP_Test_REST_Posts_Controller::test_get_post_draft_edit_context() test failure and the failure is a little more problematic than a "plain" PHP compatibility issue.

• The test sets a $post_content of 'Hello World', then does some things and then verifies that the subsequent REST API response does not contain 'Hello World' via a assertStringNotContainsString().
• The problem is that the original post which was created with the 'Hello World' content, was created as a password protected post.
• I couldn't reproduce the test failure locally on PHP 8.1 and when I dug deeper, it turned out that it was because the REST API response returned contained 'This content is password protected.' instead of 'Hello World'.
• As the tests uses the negative assertion assertStringNotContainsString(), it would pass, but as far as I can see, it doesn't actually test what it should be testing.

My interim conclusion is that this needs further investigation on what the intention was behind the test and the test probably needs to be rewritten to make sure the test tests what it is supposed to test.

👉🏻 I'm proposing to skip that test for now (on all PHP versions) until it has been fixed, as, as it is, the test has no value.

[jrf]

As a FYI: with recent commits going in and with one patch still to be committed included, the test runs on PHP 8.1 and 8.2 are down to 3 errors and 1 failure (the one described above).

While those last three errors, of course, still needs to be addressed, with only 3 errors left, I have discussed with @SergeyBiryukov to set deprecation expectations for those 3 tests so we can remove the continue-on-error for PHP 8.1 and 8.2 from the GitHub Actions workflows to prevent more/new errors coming in.

I'm currently working on the patch for this and the intention is to commit this ASAP.

The current state of things can be seen via ​https://github.com/WordPress/wordpress-develop/pull/3173

Also note that the test failure for Tests_Theme_wpGetGlobalStylesheet::test_variables_in_classic_theme_with_presets_using_variables no longer shows, so this may have been solved by one of the recent commits.

With one prepared PHP 8.1 patch still to be committed (first two commits in this PR), we're down to 3 errors and 1 test failure on both PHP 8.1 + 8.2.

The test failure is being addressed in Trac ticket 56681.

The last remaining errors need further investigation, but in the mean time, we should actively prevent new PHP issues from being introduced in WP Core.

To that end, I'm proposing to skip those last four tests, either on all PHP versions (the failure, see the explanation in the ticket mentioned above) or on PHP 8.1+ (the three test errors).

With those test skips in place, we now have a 🟢 build for both PHP 8.1 + 8.2 and can remove the continue-on-error.

Trac ticket: https://core.trac.wordpress.org/ticket/55652

[desrosj]

Confirming that [54362] resolved the Tests_Theme_wpGetGlobalStylesheet::test_variables_in_classic_theme_with_presets_using_variables failure.

I'm fully in favor of removing continue-on-error so long as there are tickets for the tests we mark as skipped so they are properly followed up on.

I haven't reviewed the changes relating to the last remaining test failure, but the test skipping and GitHub Action workflow changes look good. 👍🏼

I have rebased the PR after [54364] was committed, which means the first two commits are now gone.

The last commit skipping three tests for PHP 8.1 has been replaced with a commit applying @SergeyBiryukov's suggestion.

[SergeyBiryukov]

In 54365:

Tests: Ensure prerequisites are met for draft length tests in Tests_L10n.

These three tests for wp_dashboard_recent_drafts() would run into a PHP 8.1 "passing null to non-nullable" deprecation for the call to ltrim() when the result of get_edit_post_link() is passed to esc_url().

Setting a deprecation expectation would not solve this as the returned value would still not match the expected value(s).

The recent drafts list is only displayed on the Dashboard screen for users with the edit_posts capability. By setting the current user to Editor, the prerequisites for wp_dashboard_recent_drafts() are met, which means the deprecation notice is avoided and the assertions will succeed.

This commit addresses a few errors in the test suite along the lines of:

1) Tests_L10n::test_length_of_draft_should_be_counted_by_words
ltrim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/formatting.php:4376
/var/www/src/wp-admin/includes/dashboard.php:657
/var/www/tests/phpunit/tests/l10n.php:449
/var/www/vendor/bin/phpunit:123

Follow-up to [45505], [52253], [52259].

Props jrf, desrosj, SergeyBiryukov.
See #56681, #55652, #55656.

[SergeyBiryukov]

@jrf I've investigated the last remaining test failure. 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. As you've noted, the test could indeed be improved to check specifically for the expected result, which in this case appears to be 'This content is password protected.' string, as the test creates a password protected post as an Editor and then tries to view it as a Contributor, which is (correctly) not allowed.

However, I found that this is not the reason why it fails, the problem actually lies elsewhere. See #56712 for details and ​PR 3387 for a passing build.

[SergeyBiryukov]

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]

In 54369:

Build/Test Tools: Remove PHP 8.1 and 8.2 from allowed failures.

With all known unit test failures now addressed, WordPress 6.1 aims to support PHP 8.1 and 8.2 as much as possible.

While full compatibility with PHP 8.1 and 8.2 is still a work in progress, this commit aims to actively prevent new PHP issues from being introduced in WordPress core.

All remaining known issues are deprecation notices. Please note, a deprecation notice is not an error, but rather an indicator of where additional work is needed for compatibility before PHP 9 (i.e. when the notices become fatal errors). With a deprecation notice, the PHP code will continue to work and nothing is broken.

Follow-up to [49077], [49162], [50299], [51588], [51604], [53922], [54072].

Props jrf, desrosj.
See #55652, #55656, #56009, #56681.

Thanks for the PR! Merged in r54365 and r54369.

The REST API test issue was further investigated and resolved in r54368.

[SergeyBiryukov]

Replying to SergeyBiryukov:

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. As you've noted, the test could indeed be improved to check specifically for the expected result, which in this case appears to be 'This content is password protected.' string, as the test creates a password protected post as an Editor and then tries to view it as a Contributor, which is (correctly) not allowed.

Thinking about this some more, the test actually looks correct, as it specifically checks that the content of a password protected post created by an Editor is not viewable by a Contributor.

While we could check for the 'This content is password protected.' string instead, the test would fail if that string is ever updated.

I think we can just add a comment here to clarify the assertion, see 56681.diff​.

[SergeyBiryukov]

In 54396:

Tests: Add comments to clarify a REST API test for password protected posts.

Authenticated users should only be allowed to read password protected content if they have the edit_post meta capability for the post. In other words, the content of a password protected post created by an Editor should not be viewable by a Contributor.

This commit aims to clarify the usage of a negative assertion assertStringNotContainsString() and describe the intention behind the test to avoid confusion.

Follow-up to [50717].

Fixes #56681.