PHP 8.1 / 8.2 Compatibility #7225

swissspidy · 2022-08-25T09:01:39Z

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 ampproject/amp-toolbox-php#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).

- php: '8.2' wp: 'trunk' experimental: true

Using floats as array keys is deprecated in PHP 8.1

…per`

* origin/develop: (32 commits) Bump classnames from 2.3.1 to 2.3.2 Bump webpack-remove-empty-scripts from 1.0.0 to 1.0.1 Bump wp-cli/extension-command from 2.1.6 to 2.1.7 Bump postcss-preset-env from 7.8.1 to 7.8.2 Update jest snapshots after @wordpress/components package updates Update Gutenberg package dependencies Bump sirbrillig/phpcs-variable-analysis from 2.11.7 to 2.11.8 Fix failing tests on trunk due to webp support Remove avoidable isset() check Bump @babel/core from 7.18.13 to 7.19.0 Bump eslint from 8.23.0 to 8.23.1 Bump eslint-plugin-jest from 27.0.1 to 27.0.4 Bump eslint-plugin-react from 7.31.1 to 7.31.8 Bump webpack-remove-empty-scripts from 0.8.1 to 1.0.0 Bump css-minimizer-webpack-plugin from 4.0.0 to 4.1.0 Update Gutenberg package dependencies Bump postcss-preset-env from 7.8.0 to 7.8.1 Update test snapshots Update mask-type attribute with mask-type css prop Bump uuid from 8.3.2 to 9.0.0 ...

amp.php

Co-authored-by: Weston Ruter <westonruter@google.com>

swissspidy · 2022-09-20T06:56:44Z

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

.github/workflows/build-test-measure.yml

tests/php/src/Instrumentation/EventTest.php

swissspidy · 2022-10-26T12:21:58Z

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 · 2022-10-26T13:53:17Z

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 · 2022-10-26T18:29:17Z

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)

includes/validation/class-amp-validation-manager.php

includes/validation/class-amp-validation-error-taxonomy.php

westonruter · 2022-10-26T18:44:23Z

src/Admin/SiteHealth.php

+	 * @param string $header_value Header value.
+	 * @return bool Whether header contains the value "hit"
+	 */
+	private static function cache_hit_callback( string $header_value ): bool {


This was introduced in bd2ee42 apparently to deal with a PHPCS bug, and not due to any fault of the code?

westonruter · 2022-10-26T18:46:45Z

tests/php/bootstrap.php

-// Detect where to load the WordPress tests environment from.
-if ( false !== getenv( 'WP_TESTS_DIR' ) ) {
-	$_test_root = getenv( 'WP_TESTS_DIR' );
-} elseif ( false !== getenv( 'WP_DEVELOP_DIR' ) ) {
-	$_test_root = getenv( 'WP_DEVELOP_DIR' ) . '/tests/phpunit';
-} elseif ( file_exists( '/tmp/wordpress-tests/includes/bootstrap.php' ) ) {
-	$_test_root = '/tmp/wordpress-tests';
-} elseif ( file_exists( '/var/www/wordpress-develop/tests/phpunit' ) ) {
-	$_test_root = '/var/www/wordpress-develop/tests/phpunit';
-} else {
-	$_test_root = dirname( dirname( dirname( dirname( TESTS_PLUGIN_DIR ) ) ) ) . '/tests/phpunit';
-}
-
 // When run in wp-env context, set the test config file path.
 if ( ! defined( 'WP_TESTS_CONFIG_FILE_PATH' ) && false !== getenv( 'WP_PHPUNIT__TESTS_CONFIG' ) ) {
 	define( 'WP_TESTS_CONFIG_FILE_PATH', getenv( 'WP_PHPUNIT__TESTS_CONFIG' ) );
 }

 require_once TESTS_PLUGIN_DIR . '/vendor/yoast/wp-test-utils/src/WPIntegration/bootstrap-functions.php';
 $_tests_dir = Yoast\WPTestUtils\WPIntegration\get_path_to_wp_test_dir();
-require_once $_test_root . '/includes/functions.php';


Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter

Thanks for doing this!

Shall we wait to merge until Gutenberg 14.5 is released?

swissspidy · 2022-10-27T04:41:52Z

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

westonruter · 2022-10-27T17:31:16Z

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

swissspidy · 2022-10-27T19:18:09Z

You can also just temporarily skip these two tests

westonruter · 2022-11-09T18:04:55Z

Gutenberg updated in #7331. PHPUnit tests now all pass. 🎉

pavanpatil1 · 2023-02-06T16:23:00Z

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 · 2023-02-06T18:47:51Z

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:

amp-wp/includes/class-amp-http.php

Lines 78 to 98 in 68d9bb3

    
           public static function send_header( $name, $value, $args = [] ) { 
        
           	$args = array_merge( 
        
           		[ 
        
           			'replace'     => true, 
        
           			'status_code' => null, 
        
           		], 
        
           		$args 
        
           	); 
        
           	self::$headers_sent[] = array_merge( compact( 'name', 'value' ), $args ); 
        
           	if ( headers_sent() ) { 
        
           		return false; 
        
           	} 
        
           	header( 
        
           		sprintf( '%s: %s', $name, $value ), 
        
           		$args['replace'], 
        
           		$args['status_code'] 
        
           	); 
        
           	return true; 
        
           }

It looks like all that is required is:

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

westonruter · 2023-02-06T18:49:57Z

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

amp-wp/includes/validation/class-amp-validation-callback-wrapper.php

Lines 422 to 433 in 68d9bb3

    
           	/** 
        
           	 * Offset unset. 
        
           	 * 
        
           	 * @param mixed $offset Offset. 
        
           	 */ 
        
           	#[\ReturnTypeWillChange] 
        
           	public function offsetUnset( $offset ) { 
        
           		if ( ! is_array( $this->callback['function'] ) ) { 
        
           			return; 
        
           		} 
        
           		unset( $this->callback['function'][ $offset ] ); 
        
           	}

westonruter · 2023-02-06T18:51:40Z

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 ampproject/amp-toolbox-php#533.

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

westonruter · 2023-02-06T19:39:31Z

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 · 2023-02-06T20:10:36Z

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 · 2023-02-07T06:43:26Z

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.

swissspidy added 19 commits August 25, 2022 10:58

Run tests against PHP 8.1

d11fc69

Run tests against PHP 8.2

7902c3a

- php: '8.2' wp: 'trunk' experimental: true

Change php file to trigger CI

91c050b

Update PHPUnit depending on matrix

2443a05

Fix setup/teardown method names in BlockUniqidTransformerTest

3f37f08

Add ReturnTypeWillChange attribute in ElementList

d2c4c95

Remove conflicting Requests library

e09f922

Remove float key test case in EventTest

924be54

Using floats as array keys is deprecated in PHP 8.1

ignore platform reqs

3a98e8f

Add file_exists check

3c0badc

Don’t convert to exceptions for now

0ee41fd

Install PHPUnit 7.x on PHP 7.1

ab075f9

Fix string interpolation in CachedRemoteGetRequestTest

db8f0f7

Avoid passing null to parse_str

cf6c769

Add ReturnTypeWillChange attribute in `AMP_Validation_Callback_Wrap…

01ab3e9

…per`

Do not install pcov/clobber for PHPUnit 8

e21f5a4

Avoid passing null to parse_str, take 2

84c83ce

Add missing attribute

0e0d64c

Fix another string interpolation issue

e42e2b3

swissspidy requested a review from westonruter August 25, 2022 12:34

swissspidy mentioned this pull request Sep 5, 2022

Run tests against PHP 8.2 GoogleForCreators/web-stories-wp#12161

Merged

8 tasks

westonruter added 2 commits September 19, 2022 17:48

Update amp-toolbox-php to 0.11.3-alpha

2517d89

westonruter reviewed Sep 20, 2022

View reviewed changes

amp.php Outdated Show resolved Hide resolved

westonruter requested review from thelovekesh and westonruter September 20, 2022 01:05

Remove temp change

97440f4

Co-authored-by: Weston Ruter <westonruter@google.com>

thelovekesh reviewed Sep 20, 2022

View reviewed changes

.github/workflows/build-test-measure.yml Show resolved Hide resolved

westonruter reviewed Sep 21, 2022

View reviewed changes

tests/php/src/Instrumentation/EventTest.php Show resolved Hide resolved

swissspidy mentioned this pull request Oct 26, 2022

PHP 8.2 Compatibility GoogleForCreators/web-stories-wp#12163

Closed

Merge branch 'develop' into try-php81-php82

dd4524c

westonruter reviewed Oct 26, 2022

View reviewed changes

Apply suggestions from code review

b81b485

Co-authored-by: Weston Ruter <westonruter@google.com>

westonruter approved these changes Oct 26, 2022

View reviewed changes

westonruter added this to the v2.3.1 milestone Oct 26, 2022

Merge branch 'develop' into try-php81-php82

2473fe6

westonruter merged commit 761c716 into develop Oct 27, 2022

westonruter deleted the try-php81-php82 branch October 27, 2022 21:43

westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023

westonruter mentioned this pull request Feb 6, 2023

Use zero instead of null as empty arg for header() for PHP 8.1+ compat #7452

Merged

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8.1 / 8.2 Compatibility #7225

PHP 8.1 / 8.2 Compatibility #7225

swissspidy commented Aug 25, 2022 •

edited by westonruter

Loading

swissspidy commented Sep 20, 2022

swissspidy commented Oct 26, 2022

swissspidy commented Oct 26, 2022

westonruter commented Oct 26, 2022

westonruter Oct 26, 2022

swissspidy Oct 26, 2022

westonruter Oct 26, 2022

westonruter left a comment

swissspidy commented Oct 27, 2022

westonruter commented Oct 27, 2022

swissspidy commented Oct 27, 2022

westonruter commented Nov 9, 2022

pavanpatil1 commented Feb 6, 2023

westonruter commented Feb 6, 2023

westonruter commented Feb 6, 2023

westonruter commented Feb 6, 2023 •

edited

Loading

westonruter commented Feb 6, 2023

thelovekesh commented Feb 6, 2023

pavanpatil1 commented Feb 7, 2023

PHP 8.1 / 8.2 Compatibility #7225

PHP 8.1 / 8.2 Compatibility #7225

Conversation

swissspidy commented Aug 25, 2022 • edited by westonruter Loading

Summary

Checklist

swissspidy commented Sep 20, 2022

swissspidy commented Oct 26, 2022

swissspidy commented Oct 26, 2022

westonruter commented Oct 26, 2022

westonruter Oct 26, 2022

Choose a reason for hiding this comment

swissspidy Oct 26, 2022

Choose a reason for hiding this comment

westonruter Oct 26, 2022

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

swissspidy commented Oct 27, 2022

westonruter commented Oct 27, 2022

swissspidy commented Oct 27, 2022

westonruter commented Nov 9, 2022

pavanpatil1 commented Feb 6, 2023

westonruter commented Feb 6, 2023

westonruter commented Feb 6, 2023

westonruter commented Feb 6, 2023 • edited Loading

westonruter commented Feb 6, 2023

thelovekesh commented Feb 6, 2023

pavanpatil1 commented Feb 7, 2023

swissspidy commented Aug 25, 2022 •

edited by westonruter

Loading

westonruter commented Feb 6, 2023 •

edited

Loading