PHP 8.1 / 8.2 Compatibility

[swissspidy]

Summary

Since we use parts of the AMP plugin in the Web Stories plugin, it would be reassuring for us if it works more or less seamlessly with PHP 8.1 and 8.2.

PHP 8.2 compatibility is currently blocked by work on WordPress core itself.

Even PHP 8.1 is not seamless due to the conflict with the Requests library.

See also https://github.com/ampproject/amp-toolbox-php/pull/533 (for which an update to dev-main is included in this PR)

Checklist

• [ ] My code is tested and passes existing tests.
• [ ] My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[swissspidy]

Something to fix in PHP-CSS-Parser:

PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 129
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 129
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 130
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 131

[westonruter]

There's currently an error for PHP 7.4 with coverage.

[swissspidy]

There's currently an error for PHP 7.4 with coverage.

Yeah I saw it too and it's pretty confusing.

Somehow the test never finishes, so there's no coverage file being created:

https://github.com/ampproject/amp-wp/actions/runs/3087716934/jobs/5021640946#step:21:55

A couple of PHP warnings:

PHP Warning:  Cannot modify header information - headers already sent by (output started at /tmp/wordpress-tests-lib/includes/bootstrap.php:261) in /tmp/wordpress/src/wp-includes/pluggable.php on line 1421

Warning: Cannot modify header information - headers already sent by (output started at /tmp/wordpress-tests-lib/includes/bootstrap.php:261) in /tmp/wordpress/src/wp-includes/pluggable.php on line 1424

Then I noticed that while CI is supposed to use PCOV for coverage ever since https://github.com/ampproject/amp-wp/commit/536dbe176596c375becaeadd1151833d9c7353e9, it has always been using XDebug.

See https://github.com/ampproject/amp-wp/actions/runs/2749592396/jobs/4314380797#step:20:17 from 2 months ago.

Note how it says "Runtime: PHP 7.4.30 with Xdebug"

So it was never using PCOV at all.

Now I've tried switching to XDebug again but it doesn't work either. Same error though 😞

No idea why. wp-includes/pluggable.php on line 1424 is the wp_redirect() function. So something is doing redirects when trying to install WordPress...

[swissspidy]

@westonruter I found the issue 🥳 🎉

The culprit was the test_handle_single_url_page_bulk_and_inline_actions test covering \AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions(). That method calls wp_safe_redirect and exit(), causing the tests to silently halt.

I update the test to short-circuit the redirect and wrapping the exit in an if so that it's never called during tests.

While at it, I noticed that test_prepare_response_throwing_error failed on trunk because on trunk there are currently tons of other PHP deprecation warnings, making the regex not match correctly. Making the regex less strict resolves the issue.

All tests should be passing now.

[github-actions[bot]]

Plugin builds for 2473fe6b39a822f0fa5dccd6d3935f7ed823f553 are ready :bellhop_bell:!

• Download development build
• Download production build

[swissspidy]

Slight correction, there are two more tests failing on trunk. They are unrelated to the changes here though.

1) AMP_Style_Sanitizer_Test::test_prioritized_stylesheets with data set "admin_bar_included" (Closure Object (...), Closure Object (...))
Failed asserting that '' contains "admin-bar".

Error: /tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-amp-style-sanitizer.php:3494
/tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-amp-style-sanitizer.php:3575

2) Test_AMP_Theme_Support::test_prepare_response_standard
'<link rel="preload" as="script" href="https://cdn.ampproject.org/v0/amp-dynamic-css-classes-0.1.js">' is not after '<link rel="dns-prefetch" href="//cdn.ampproject.org">'
Failed asserting that 364 is greater than 61757.

/tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-class-amp-theme-support.php:1767

Both fail because the HTML output is polluted with deprecation warnings like this from core:

Deprecated: strncmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /tmp/wordpress/wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php on line 1113

This bug was just fixed in Gutenberg yesterday: https://github.com/WordPress/gutenberg/pull/44829 So this will get resorted once the next GB version is released.

[swissspidy]

This bug was just fixed in Gutenberg yesterday: WordPress/gutenberg#44829 So this will get resorted once the next GB version is released.

FYI, while GB 14.4 was released today, that bugfix won't be included until version 14.5 (due by November 07, 2022)

[swissspidy]

Unit test (with coverage): PHP 7.4, WP latest

This required status check needs to be removed/changed, since now the coverage is done with PHP 8.0 instead.

[westonruter]

Unit test (with coverage): PHP 7.4, WP latest

This required status check needs to be removed/changed, since now the coverage is done with PHP 8.0 instead.

Removed (to be changed after merge)

[swissspidy]

I don‘t see an issue with merging this already, but up to you

[westonruter]

The issue is the failing tests will make it hard to identify problems with other PRs being opened in the next ~2 weeks.

[swissspidy]

You can also just temporarily skip these two tests

[westonruter]

Gutenberg updated in https://github.com/ampproject/amp-wp/pull/7331. PHPUnit tests now all pass. 🎉

[pavanpatil1]

Tested it with PHP 8.1 and 8.2, Plugin functionality seems working fine with both the PHP versions. However, there are some deprecation warnings are visible in both versions.

PHP8.1

PHP Deprecated:  header(): Passing null to parameter #3 ($response_code) of type int is deprecated in C:\Users\win 10\Local Sites\php8amp\app\public\wp-content\plugins\amp\includes\class-amp-http.php on line 95

PHP8.2

PHP Deprecated:  Return type of AMP_Validation_Callback_Wrapper::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/htdocs/current/wp-content/plugins/amp/includes/validation/class-amp-validation-callback-wrapper.php on line 425
PHP Deprecated:  Creation of dynamic property AmpProject\Optimizer\Configuration\MinifyHtmlConfiguration::$minifyAmpScript is deprecated in /var/www/htdocs/current/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Configuration/BaseTransformerConfiguration.php on line 40
PHP Deprecated:  header(): Passing null to parameter #3 ($response_code) of type int is deprecated in /var/www/htdocs/current/wp-content/plugins/amp/includes/class-amp-http.php on line 92

[westonruter]

In regards to:

PHP Deprecated:  header(): Passing null to parameter #3 ($response_code) of type int is deprecated in C:\Users\win 10\Local Sites\php8amp\app\public\wp-content\plugins\amp\includes\class-amp-http.php on line 95

This is the code in question:

https://github.com/ampproject/amp-wp/blob/68d9bb3f977967fa05fb09f5f52da561dc85bd72/includes/class-amp-http.php#L78-L98

It looks like all that is required is:

- 				'status_code' => null,
+ 				'status_code' => 0,

[westonruter]

In regards to:

PHP Deprecated:  Return type of AMP_Validation_Callback_Wrapper::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/htdocs/current/wp-content/plugins/amp/includes/validation/class-amp-validation-callback-wrapper.php on line 425

I'm confused since it does have #[\ReturnTypeWillChange]:

https://github.com/ampproject/amp-wp/blob/68d9bb3f977967fa05fb09f5f52da561dc85bd72/includes/validation/class-amp-validation-callback-wrapper.php#L422-L433

[westonruter]

And finally, in regards to:

PHP Deprecated:  Creation of dynamic property AmpProject\Optimizer\Configuration\MinifyHtmlConfiguration::$minifyAmpScript is deprecated in /var/www/htdocs/current/wp-content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Configuration/BaseTransformerConfiguration.php on line 40

Looks like this is something that was missed in https://github.com/ampproject/amp-toolbox-php/pull/533.

See example at https://github.com/ampproject/amp-toolbox-php/pull/533/files#diff-12262a92eb1759cfc297901a94d3e5ab9edc9862154fb003e21f7b80719a7fc2

[westonruter]

Actually, it seems like this was tested without the latest amp-toolbox-php. Line 40 in BaseTransformerConfiguration.php is the following in v0.11.2:

$this->$key = $this->validate($key, $value);

Whereas in main it is the following:

$this->allowedKeys = $this->getAllowedKeys();

I just tried downloading the most recent development build and I see that the former is present as expected.

@pavanpatil1 Could you please re-test, making sure you have the latest development build installed (as for all current QA)?

[thelovekesh]

I checked with PHP 8.2 but was unable to get any deprecation warnings other than the

[06-Feb-2023 19:38:52 UTC] PHP Deprecated:  header(): Passing null to parameter #3 ($response_code) of type int is deprecated in /app/public/content/plugins/amp/includes/class-amp-http.php on line 92

[pavanpatil1]

QA passed ✅

Tested it with the latest development build. Functionality was working as expected before now the deprecation warnings are not visible now with both v8.1 and 8.2. solved in this PR.