Fix initial php 8.4 compatibility #2714

mikkamp · 2024-12-05T16:16:59Z

Changes proposed in this Pull Request:

This PR is the initial start for resolving the issues that are in #2647

There are still a few dependent packages we are waiting for provide full compatibility:

WooCommerce: PHP 8.4 compatibility woocommerce#52081
google/gax: Test package is available but not released yet
google/auth: Test package is available but not released yet
google/apiclient: Backporting PHP 8.4 compatibility googleapis/google-api-php-client#2640
googleads/google-ads-php: Deprecation notices when using with PHP 8.4 googleads/google-ads-php#1056
automattic/jetpack-connect: is resolved but we'll need to bump to 6.0.0 minimum (they still support PHP < 7.4)

Resolved in this PR

Fix all PHP code within the extension that is not compatible with PHP 8.4
Update the guzzle packages and fix namespace replacements

$ composer update guzzlehttp/guzzle --with-dependencies
   
  - Upgrading guzzlehttp/guzzle (7.8.1 => 7.9.2)
  - Upgrading guzzlehttp/promises (2.0.2 => 2.0.4)
  - Upgrading guzzlehttp/psr7 (2.6.2 => 2.7.0)
  - Upgrading psr/http-factory (1.0.2 => 1.1.0)

Update the phpunit packages for testing (not bundled in release)

$ composer update phpunit/phpunit --with-dependencies
   
  - Upgrading myclabs/deep-copy (1.11.1 => 1.12.1)
  - Upgrading nikic/php-parser (v4.17.1 => v5.3.1)
  - Upgrading phar-io/manifest (2.0.3 => 2.0.4)
  - Upgrading phpunit/php-code-coverage (9.2.29 => 9.2.32)
  - Upgrading phpunit/phpunit (9.6.13 => 9.6.21)
  - Upgrading sebastian/cli-parser (1.0.1 => 1.0.2)
  - Upgrading sebastian/complexity (2.0.2 => 2.0.3)
  - Upgrading sebastian/diff (4.0.5 => 4.0.6)
  - Upgrading sebastian/exporter (4.0.5 => 4.0.6)
  - Upgrading sebastian/global-state (5.0.6 => 5.0.7)
  - Upgrading sebastian/lines-of-code (1.0.3 => 1.0.4)
  - Upgrading sebastian/resource-operations (3.0.3 => 3.0.4)
  - Upgrading theseer/tokenizer (1.2.1 => 1.2.3)

Update Jetpack packages to highest available v2 package

$ composer update automattic/jetpack-connection --with-dependencies

  - Upgrading automattic/jetpack-a8c-mc-stats (v2.0.0 => v2.0.4)
  - Upgrading automattic/jetpack-admin-ui (v0.3.2 => v0.4.6)
  - Locking automattic/jetpack-assets (v2.3.14)
  - Upgrading automattic/jetpack-connection (v2.3.2 => v2.12.6)
  - Upgrading automattic/jetpack-constants (v2.0.0 => v2.0.5)
  - Upgrading automattic/jetpack-redirect (v2.0.0 => v2.0.3)
  - Upgrading automattic/jetpack-roles (v2.0.0 => v2.0.4)
  - Upgrading automattic/jetpack-status (v2.1.0 => v3.3.5)

Update validator package and revert pattern to remain strict for image URL validation

$ composer update symfony/validator --with-dependencies

  - Upgrading symfony/polyfill-ctype (v1.29.0 => v1.31.0)
  - Upgrading symfony/polyfill-php73 (v1.28.0 => v1.31.0)
  - Upgrading symfony/polyfill-php80 (v1.29.0 => v1.31.0)
  - Upgrading symfony/polyfill-php81 (v1.29.0 => v1.31.0)
  - Upgrading symfony/translation-contracts (v2.5.2 => v2.5.3)
  - Upgrading symfony/validator (v5.4.30 => v5.4.47)

Update league/container package and provide support for V4 (we remain using psr/container 1.1)

$ composer update league/container --with-dependencies

  - Upgrading league/container (3.4.1 => 4.2.4)

Detailed test instructions:

Checkout this branch
Update packages rm -rf vendor && composer install
Temporarily use test packages:

composer require "google/auth:dev-1.37.x-php84 as 1.37.2"
composer require "google/gax:dev-1.29.1-php74 as 1.29.2"

Create a temporary script to patch remaining packages in bin/fix-packages.php

<?php

function replace_all( $path, $search, $replace ) {
	foreach ( glob( $path ) as $file ) {
		$content = preg_replace(
			'/' . preg_quote( $search, '/' ) . '/',
			$replace,
			file_get_contents( $file )
		);
		file_put_contents( $file, $content );
	}
}

replace_all(
	'vendor/googleads/google-ads-php/src/Google/Ads/GoogleAds/V16/Services/Client/*.php',
	'string $template = null',
	'?string $template = null'
);

replace_all(
	'vendor/automattic/jetpack-connection/src/class-manager.php',
	'Jetpack_XMLRPC_Server $xmlrpc_server = null',
	'?Jetpack_XMLRPC_Server $xmlrpc_server = null'
);

replace_all(
	'vendor/google/apiclient/src/Client.php',
	'ClientInterface $authHttp = null',
	'?ClientInterface $authHttp = null'
);

replace_all(
	'vendor/google/apiclient/src/Client.php',
	'ClientInterface $http = null',
	'?ClientInterface $http = null'
);

replace_all(
	'vendor/google/apiclient/src/Service/Exception.php',
	'Exception $previous = null',
	'?Exception $previous = null'
);

Patch packages php bin/fix-packages.php
Setup the unit tests bin/install-wp-tests.sh <db> <db_user> <db_pass>
Replace the copy of WooCommerce from /tmp/woocommerce-9.4.3/plugins/woocommerce with the contents of the zip file provided below
woocommerce-9.4.3-patched-for-php-8-4.zip
Run unit tests with php 8.4 /usr/bin/php8.4 vendor/bin/phpunit
Ensure all tests complete without errors

$ /usr/bin/php8.4 vendor/bin/phpunit

Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Installing WooCommerce...
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

.............................................................   61 / 1327 (  4%)
.............................................................  122 / 1327 (  9%)
.............................................................  183 / 1327 ( 13%)
.............................................................  244 / 1327 ( 18%)
.............................................................  305 / 1327 ( 22%)
.............................................................  366 / 1327 ( 27%)
.............................................................  427 / 1327 ( 32%)
.............................................................  488 / 1327 ( 36%)
.............................................................  549 / 1327 ( 41%)
.............................................................  610 / 1327 ( 45%)
.............................................................  671 / 1327 ( 50%)
.............................................................  732 / 1327 ( 55%)
.............................................................  793 / 1327 ( 59%)
.............................................................  854 / 1327 ( 64%)
.............................................................  915 / 1327 ( 68%)
.............................................................  976 / 1327 ( 73%)
............................................................. 1037 / 1327 ( 78%)
............................................................. 1098 / 1327 ( 82%)
............................................................. 1159 / 1327 ( 87%)
............................................................. 1220 / 1327 ( 91%)
............................................................. 1281 / 1327 ( 96%)
..............................................                1327 / 1327 (100%)

Time: 00:32.341, Memory: 217.00 MB

OK (1327 tests, 4163 assertions)

Repeat same unit tests with other versions of PHP like 7.4
Run smoke tests with the extensions to confirm it doesn't have any issues with the container code

Additional details:

peeuvX-1Zh-p2

Changelog entry

Fix - Initial php 8.4 compatibility.

codecov · 2024-12-05T16:20:41Z

Codecov Report

Attention: Patch coverage is 61.36364% with 17 lines in your changes missing coverage. Please review.

Project coverage is 66.2%. Comparing base (7cd33a8) to head (a0e163d).
Report is 41 commits behind head on develop.

Files with missing lines	Patch %	Lines
...DependencyManagement/ThirdPartyServiceProvider.php	0.0%	6 Missing ⚠️
...ernal/DependencyManagement/CoreServiceProvider.php	0.0%	5 Missing ⚠️
src/Container.php	0.0%	2 Missing ⚠️
.../Exception/RuntimeExceptionWithMessageFunction.php	0.0%	1 Missing ⚠️
...l/DependencyManagement/AbstractServiceProvider.php	75.0%	1 Missing ⚠️
...nternal/DependencyManagement/DBServiceProvider.php	0.0%	1 Missing ⚠️
...rnal/DependencyManagement/ProxyServiceProvider.php	0.0%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #2714      +/-   ##
============================================
+ Coverage       63.4%   66.2%    +2.8%     
- Complexity         0    4691    +4691     
============================================
  Files            340     479     +139     
  Lines           5210   19608   +14398     
  Branches        1275       0    -1275     
============================================
+ Hits            3305   12983    +9678     
- Misses          1730    6625    +4895     
+ Partials         175       0     -175

Flag	Coverage Δ
js-unit-tests	`?`
php-unit-tests	`66.2% <61.4%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/Assets/BaseAsset.php	`74.0% <ø> (ø)`
src/Assets/ScriptAsset.php	`21.1% <ø> (ø)`
src/Assets/ScriptWithBuiltDependenciesAsset.php	`77.8% <ø> (ø)`
src/Assets/StyleAsset.php	`12.0% <ø> (ø)`
src/DB/Table/BudgetRecommendationTable.php	`48.2% <100.0%> (ø)`
src/Exception/ExceptionWithResponseData.php	`81.8% <100.0%> (ø)`
src/Google/InvalidCouponEntry.php	`66.7% <ø> (ø)`
src/Infrastructure/View.php	`100.0% <ø> (ø)`
...nal/DependencyManagement/GoogleServiceProvider.php	`95.0% <100.0%> (ø)`
...ternal/DependencyManagement/JobServiceProvider.php	`0.0% <ø> (ø)`
... and 13 more

... and 796 files with indirect coverage changes

mikkamp · 2024-12-05T17:20:08Z

This would also supersede #2663

bin/GoogleAdsCleanupServices.php

puntope · 2024-12-06T09:03:13Z

I get still one error in Unit Tests

puntope · 2024-12-06T09:05:37Z

Create a temporary script to patch remaining packages in bin/fix-packages.php

Why not include this as the rest of the scripts like prefix-vendor-namespace.php?

puntope · 2024-12-06T09:14:06Z

replace_all(
'vendor/googleads/google-ads-php/src/Google/Ads/GoogleAds/V16/Services/Client/*.php',
'string $template = null',
'?string $template = null'
);

The first time you run this. It works. But if you run it another time It will result in:

'??string $template = null'

mikkamp · 2024-12-06T12:26:29Z

I get still one error in Unit Tests

Is that consistently reproducible? I'm not seeing that happen here on any PHP version. Can you confirm if you run just the individual test on a specific PHP version if you get it to fail:
/usr/bin/php8.4 vendor/bin/phpunit --filter=test_add_subsize_image

Based on the results it means that it won't create a 20x20 thumbnail image. I wonder if there is a PHP image module that is different between versions of PHP. Can you also run:
diff <(/usr/bin/php8.4 -m) <(/usr/bin/php8.3 -m)

Just to see if there is a difference in PHP modules.

Why not include this as the rest of the scripts like prefix-vendor-namespace.php?

This means we are patching upstream libraries manually, it is an option, but instead we've asked them for compatible versions. Once we get the updated versions we won't need a script like that anymore. Considering that they are deprecation notices (shouldn't in theory be shown on production sites), I thought the better way was to wait till we get updated packages.

The first time you run this. It works. But if you run it another time It will result in:
'??string $template = null'

Yes, I should have mentioned that, it's a quick search/replace script so only intended for running once.

puntope · 2024-12-09T07:04:20Z

@mikkamp Re Image test

It does fail when running the test isolatedly.

Find also the output for diff <(/usr/bin/php8.4 -m) <(/usr/bin/php8.2 -m) (Notice the 8.2)

1a2
> bcmath
12a14
> gd
16c18,20
< imap
---
> igbinary
> imagick
> intl
18d21
< ldap
20a24,26
> memcache
> memcached
> msgpack
31a38
> redis
35a43
> soap
43a52
> xdebug
45a55
> xmlrpc
52a63
> Xdebug

maybe_add_subsize_image doesn't return any error. It just doesn't set the subsize in the metadata.

Update:

@mikkamp I was doing some debugging and found the error in wp_get_image_editor inside _wp_make_subsizes ( this is being called in wp_update_image_subsizes)

Test 'Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\Utility\ImageUtilityTest::test_add_subsize_image' ended
object(WP_Error)#7361 (3) {
  ["errors"]=>
  array(1) {
    ["image_no_editor"]=>
    array(1) {
      [0]=>
      string(28) "No editor could be selected."
    }
  }
  ["error_data"]=>
  array(0) {
  }
  ["additional_data":protected]=>
  array(0) {
  }
}

Update 2:

Solved installing php8.4-imagick (as one of the editors)

puntope · 2024-12-09T07:16:00Z

This is my php-v output btw

PHP 8.4.1 (cli) (built: Nov 25 2024 18:03:47) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.4.1, Copyright (c) Zend Technologies
    with Zend OPcache v8.4.1, Copyright (c), by Zend Technologies

puntope

Hi @mikkamp I solved the problem with the test. See ( #2714 (comment) )

So no more blockers left.

mikkamp · 2024-12-09T09:49:09Z

Thanks for clarifying. Either the gd or imagick image library is needed, I assume it failed because neither was available. I thought WordPress had a fallback for those cases, but generally most servers will have at least one available.

Going to merge this, now that it's running fine.

mikkamp added 12 commits December 5, 2024 11:44

Explicitly mark parameter types as nullable

1994480

Pass parameters to str_getcsv

b6ba522

Explicitly mark parameter types as nullable in tools

3758300

Update guzzle package and direct replacements

ad729fd

Update phpunit packages

c843d6f

Update jetpack connection package

0850d23

Update validator packages

d1d1f58

Use pattern which accepts only some unicode characters

cd860f3

Update league/container

e4efa7a

Add support for league container v4

0d2955d

Add support for league container v4 in tests

0cbe3a4

Fix for PHP 7.4

a0e163d

mikkamp self-assigned this Dec 5, 2024

github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Dec 5, 2024

mikkamp requested a review from a team December 5, 2024 17:11

puntope reviewed Dec 6, 2024

View reviewed changes

bin/GoogleAdsCleanupServices.php Show resolved Hide resolved

mikkamp requested a review from puntope December 6, 2024 12:34

puntope approved these changes Dec 9, 2024

View reviewed changes

mikkamp merged commit 55cd0b1 into develop Dec 9, 2024
14 checks passed

mikkamp deleted the fix/initial-php-8.4-compat branch December 9, 2024 09:49

This was referenced Dec 11, 2024

fix: add PHP 8.4 compatibility to last release compatible with PHP 7.4 googleapis/gax-php#600

Merged

fix: add PHP 8.4 compatibility to last release compatible with PHP 7.4 googleapis/google-auth-library-php#592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix initial php 8.4 compatibility #2714

Fix initial php 8.4 compatibility #2714

mikkamp commented Dec 5, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading

mikkamp commented Dec 5, 2024

puntope commented Dec 6, 2024

puntope commented Dec 6, 2024

puntope commented Dec 6, 2024

mikkamp commented Dec 6, 2024

puntope commented Dec 9, 2024 •

edited

Loading

puntope commented Dec 9, 2024

puntope left a comment

mikkamp commented Dec 9, 2024

Fix initial php 8.4 compatibility #2714

Fix initial php 8.4 compatibility #2714

Conversation

mikkamp commented Dec 5, 2024

Changes proposed in this Pull Request:

Resolved in this PR

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

mikkamp commented Dec 5, 2024

puntope commented Dec 6, 2024

puntope commented Dec 6, 2024

puntope commented Dec 6, 2024

mikkamp commented Dec 6, 2024

puntope commented Dec 9, 2024 • edited Loading

puntope commented Dec 9, 2024

puntope left a comment

Choose a reason for hiding this comment

mikkamp commented Dec 9, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading

puntope commented Dec 9, 2024 •

edited

Loading