Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.4 package compatibility #2728

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Conversation

mikkamp
Copy link
Contributor

@mikkamp mikkamp commented Dec 16, 2024

Changes proposed in this Pull Request:

This PR updates all the Google packages we rely on for the Google Ads API packages to make them fully PHP 8.4 compatible.

Note that some of the changes to the Google Ads API packages itself have only been applied to V18 of the API. Currently we aren't fully ready to switch from V16 to V18 yet as we need to become compatible with the removal of the page_size parameter. Until we will need to continue using a small patch script to test changes with PHP 8.4.

Composer package updates
$ composer update googleads/google-ads-php --with-all-dependencies
  - Upgrading google/auth (v1.37.1 => v1.37.2)
  - Upgrading google/gax (v1.29.1 => v1.29.2)
  - Upgrading google/longrunning (0.4.1 => 0.4.6)
  - Upgrading google/protobuf (v3.25.3 => v3.25.5)
  - Upgrading googleads/google-ads-php (dev-legacy-v22.1.0 905fd78 => dev-legacy-v25.0.0 3f4ea59)
  - Upgrading monolog/monolog (2.9.3 => 2.10.0)
  - Upgrading symfony/polyfill-intl-normalizer (v1.29.0 => v1.31.0)
  - Upgrading symfony/polyfill-mbstring (v1.29.0 => v1.31.0)
$ composer update google/apiclient --with-all-dependencies
  - Upgrading google/apiclient (v2.16.0 => v2.16.1)
  - Upgrading paragonie/constant_time_encoding (v2.6.3 => v2.7.0)
  - Upgrading phpseclib/phpseclib (3.0.37 => 3.0.43)

We also updated the script to remove newer versions of the Ads API library V17 and V18, to keep the bundled size lower until we are ready to switch to V18.

Detailed test instructions:

  1. Checkout this branch
  2. Update packages rm -rf vendor && composer install
  3. 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'
);
  1. Patch packages php bin/fix-packages.php
  2. Setup the unit tests bin/install-wp-tests.sh <db> <db_user> <db_pass>
  3. 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
  4. Run unit tests with php 8.4 /usr/bin/php8.4 vendor/bin/phpunit
  5. 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 / 1329 (  4%)
.............................................................  122 / 1329 (  9%)
.............................................................  183 / 1329 ( 13%)
.............................................................  244 / 1329 ( 18%)
.............................................................  305 / 1329 ( 22%)
.............................................................  366 / 1329 ( 27%)
.............................................................  427 / 1329 ( 32%)
.............................................................  488 / 1329 ( 36%)
.............................................................  549 / 1329 ( 41%)
.............................................................  610 / 1329 ( 45%)
.............................................................  671 / 1329 ( 50%)
.............................................................  732 / 1329 ( 55%)
.............................................................  793 / 1329 ( 59%)
.............................................................  854 / 1329 ( 64%)
.............................................................  915 / 1329 ( 68%)
.............................................................  976 / 1329 ( 73%)
............................................................. 1037 / 1329 ( 78%)
............................................................. 1098 / 1329 ( 82%)
............................................................. 1159 / 1329 ( 87%)
............................................................. 1220 / 1329 ( 91%)
............................................................. 1281 / 1329 ( 96%)
................................................              1329 / 1329 (100%)

Time: 00:33.302, Memory: 213.00 MB

OK (1329 tests, 4185 assertions)
  1. Repeat same unit tests with other versions of PHP like 7.4

Additional details:

peeuvX-1Zh-p2

Changelog entry

  • Fix - PHP 8.4 package compatibility.

@mikkamp mikkamp self-assigned this Dec 16, 2024
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Dec 16, 2024
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.2%. Comparing base (af019af) to head (e69f661).
Report is 3 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2728      +/-   ##
============================================
+ Coverage       63.5%   66.2%    +2.7%     
- Complexity         0    4691    +4691     
============================================
  Files            337     479     +142     
  Lines           5193   19608   +14415     
  Branches        1273       0    -1273     
============================================
+ Hits            3299   12983    +9684     
- Misses          1721    6625    +4904     
+ Partials         173       0     -173     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 66.2% <ø> (?)

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

see 816 files with indirect coverage changes

@mikkamp mikkamp requested a review from a team December 16, 2024 15:14
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mikkamp LGTM

  • Tested in PHP8.4
  • Tested in PHP8.1
  • Tested in PHP7.4

@mikkamp mikkamp merged commit e849de8 into develop Dec 17, 2024
13 checks passed
@mikkamp mikkamp deleted the fix/php8.4-package-compatibility branch December 17, 2024 09:24
@eason9487 eason9487 mentioned this pull request Dec 18, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants