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

Fix PHP 8.4 deprecation #230

Merged
merged 36 commits into from
Dec 16, 2024
Merged

Fix PHP 8.4 deprecation #230

merged 36 commits into from
Dec 16, 2024

Conversation

Luc45
Copy link
Member

@Luc45 Luc45 commented Dec 14, 2024

This PR:

  • Updates DI52 to ^4, which fixes the PHP 8.4 deprecation
  • Adds a workflow to run our PHPUnit and Phan in all versions we support (7.2-8.4)
  • Make some small tweaks to our tests so that they run on PHP 7.2
  • This workflow will run on PR pushes

Testing Instructions

  • Run make check-php-versions, assert that it builds a docker image for each PHP version
  • Assert that each docker image has the expected PHP version
  • Then run make tests-all and assert it runs the PHPUnit and Phan in each of the supported versions, and that it passes on all
  • Double-check the PHPUnit tests on CI in this PR

There's only one Deprecated being triggered in the test itself in PHP 8.4, but it's safe to ignore:

Deprecated: Spatie\Snapshots\MatchesSnapshots::assertMatchesSnapshot(): Implicitly marking parameter $driver as nullable is deprecated, the explicit nullable type must be used instead in /app/src/vendor/spatie/phpunit-snapshot-assertions/src/MatchesSnapshots.php on line 48

We currently use on an older version of spatie/phpunit-snapshot-assertions that supports PHP 7.2. However, this version triggers a deprecation warning when running on PHP 8.4. While upgrading to the latest release of the package would resolve the issue, it requires a minimum of PHP 8.1, which would break compatibility with older environments.

Given these constraints, the most practical solution is to simply ignore this deprecation notice under PHP 8.4. Since the warning appears only during tests.

@Luc45 Luc45 requested a review from a team December 14, 2024 13:26
@Luc45 Luc45 self-assigned this Dec 14, 2024
@Luc45 Luc45 marked this pull request as draft December 14, 2024 14:09
@Luc45 Luc45 removed the request for review from a team December 14, 2024 14:09
@Luc45
Copy link
Member Author

Luc45 commented Dec 14, 2024

Since this is under the woocommerce org, I also tried to be conscious about CI timings, especially when running all these different versions in parallel, so I've made sure that the Docker image used in the PHP tests are cached.

@Luc45 Luc45 requested a review from a team December 14, 2024 17:46
@Luc45 Luc45 marked this pull request as ready for review December 14, 2024 17:46
@Luc45
Copy link
Member Author

Luc45 commented Dec 14, 2024

Here's an AI-generated summary if it helps reviewing this:

  • Run Phan and PHPUnit in all supported PHP versions (7.2 to 8.4)
  • Updates DI52 to ^4, which fixes the PHP 8.4 deprecation
  • Introduces a composite action (restore-docker-cache) for Docker image caching, reducing duplication and speeding up builds.
  • Consolidates Dockerfiles into a single flexible build using an ARG for PHP versions.
  • Expands GitHub Actions test matrices to multiple PHP versions (7.2 to 8.4), ensuring broader compatibility.
  • Improves caching (composer and Docker) for faster CI runs.
  • Updates the Makefile to run tests inside a versioned Docker image.
  • Adds new dev dependencies (Phan) and updates code standards tooling.
  • Cleans up and refines test code.

@zhongruige
Copy link
Contributor

check-php-version worked great for me, though when I went to run test-all, I got this:

make tests-all
/Applications/Xcode.app/Contents/Developer/usr/bin/make phpcs
/Applications/Xcode.app/Contents/Developer/usr/bin/make phpcbf || true
Docker image not found. Building qit-cli-tests:8.3...
[+] Building 31.6s (8/8) FINISHED                               docker:orbstack
 => [internal] load build definition from Dockerfile                       0.1s
 => => transferring dockerfile: 585B                                       0.0s
 => [internal] load metadata for docker.io/library/php:8.3-cli             1.7s
 => [internal] load .dockerignore                                          0.1s
 => => transferring context: 2B                                            0.0s
 => [1/4] FROM docker.io/library/php:8.3-cli@sha256:c89669bd006347b4bfe87  5.2s
 => => resolve docker.io/library/php:8.3-cli@sha256:c89669bd006347b4bfe87  0.0s
 => => sha256:797a5415690172ed23c5ea387d3743a92a231c8b448 2.69kB / 2.69kB  0.0s
 => => sha256:dfc980920c054a940b9793a107341acd51b059aed 97.94MB / 97.94MB  3.0s
 => => sha256:c89669bd006347b4bfe87c2d161e5cf6ea7932252 10.35kB / 10.35kB  0.0s
 => => sha256:bb3f2b52e6af242cee1bc6c19ce79e05544f8a1d1 28.06MB / 28.06MB  1.1s
 => => sha256:593b3b5e04c8b4e6110fb0e212055b9c7eb2aaa1cf8779f 224B / 224B  0.3s
 => => sha256:3e4f73509d2da274e4c779ac4838773aca11ee2a190 8.35kB / 8.35kB  0.0s
 => => sha256:7130ec09764042703bb02f30348dab7ba0380d619963d04 223B / 223B  0.8s
 => => sha256:14310aa309f408c03e58fedaf8744d3c2bba275fd 12.63MB / 12.63MB  1.4s
 => => extracting sha256:bb3f2b52e6af242cee1bc6c19ce79e05544f8a1d13f5a6c1  0.6s
 => => sha256:7b28bbb22eee1dcfe2b4467fc7f036eea805bc9d1d2fef1 487B / 487B  1.4s
 => => sha256:054defd136ea8d35e090b57bcdc9af79d5b4a091300 2.45kB / 2.45kB  1.6s
 => => sha256:ac2195c11c809b4f6791cbd3ee82875dfed2c3f8f 35.84MB / 35.84MB  2.5s
 => => sha256:5621dd5fe731667c6def9f0cff48510f68ee287ea2ab6dc 246B / 246B  1.8s
 => => extracting sha256:593b3b5e04c8b4e6110fb0e212055b9c7eb2aaa1cf8779fd  0.0s
 => => extracting sha256:dfc980920c054a940b9793a107341acd51b059aed873177e  1.4s
 => => extracting sha256:7130ec09764042703bb02f30348dab7ba0380d619963d04a  0.0s
 => => extracting sha256:14310aa309f408c03e58fedaf8744d3c2bba275fd52ed2d0  0.0s
 => => extracting sha256:7b28bbb22eee1dcfe2b4467fc7f036eea805bc9d1d2fef18  0.0s
 => => extracting sha256:ac2195c11c809b4f6791cbd3ee82875dfed2c3f8f6aff697  0.4s
 => => extracting sha256:054defd136ea8d35e090b57bcdc9af79d5b4a091300dd49a  0.0s
 => => extracting sha256:5621dd5fe731667c6def9f0cff48510f68ee287ea2ab6dc0  0.0s
 => [2/4] RUN apt-get update     && apt-get install -y libzip-dev     &&   7.4s
 => [3/4] RUN pecl install ast && docker-php-ext-enable ast                4.0s
 => [4/4] RUN if [ "" != "true" ] && [ "8" -ge 8 ]; then         pecl in  13.0s
 => exporting to image                                                     0.1s
 => => exporting layers                                                    0.1s
 => => writing image sha256:0c2d3bf2d37b6bed48cf6d74847b2230d68f1d1ad5b15  0.0s
 => => naming to docker.io/library/qit-cli-tests:8.3                       0.0s
............... 15 / 15 (100%)



No fixable errors were found

Time: 667ms; Memory: 14MB

............... 15 / 15 (100%)


Time: 639ms; Memory: 14MB

/Applications/Xcode.app/Contents/Developer/usr/bin/make phpstan
Result cache not used because the metadata do not match: phpVersion, projectConfig, phpExtensions
 104/104 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 3 secs/3 secs

Result cache is saved.
 ------ --------------------------------------------------------------------- 
  Line   src/Commands/Environment/UpEnvironmentCommand.php                    
 ------ --------------------------------------------------------------------- 
  379    Call to static method parse() on an unknown class Dotenv\Dotenv.     
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 


 [ERROR] Found 1 error                                                          

Used memory: 760 MB
make[1]: *** [phpstan] Error 1
make: *** [tests-all] Error 2

@Luc45
Copy link
Member Author

Luc45 commented Dec 14, 2024

Sorry, forgot: composer install on src

@zhongruige
Copy link
Contributor

Ah awesome, that fixed it, thanks @Luc45!

Screenshot 2024-12-14 at 5 02 49 PM

@zhongruige
Copy link
Contributor

I get this deprecation notice when on this branch (such as running qit run:activation bistro) so wanted to double check if this is one we still expect:

PHP Deprecated:  _HumbugBoxf25c155a2566\lucatume\DI52\Builders\ReinitializableBuilderInterface::reinit(): Implicitly marking parameter $afterBuildMethods as nullable is deprecated, the explicit nullable type must be used instead in phar:///Users/gregbell/workspace/qit-cli/qit/vendor/lucatume/di52/src/Builders/ReinitializableBuilderInterface.php on line 7

@Luc45
Copy link
Member Author

Luc45 commented Dec 16, 2024

@zhongruige Can you please pull and try again?

@zhongruige
Copy link
Contributor

Perfect, thanks @Luc45! using the build worked and the notices are no longer showing.

Copy link
Contributor

@zhongruige zhongruige left a comment

Choose a reason for hiding this comment

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

LGTM and tested great, thanks for keeping our projects compliant with the latest versions of PHP and for adding in tests across the range of PHP versions that we support, too.

@Luc45 Luc45 merged commit cc7903c into trunk Dec 16, 2024
17 checks passed
@Luc45 Luc45 deleted the 24-12/php-84-deprecation branch December 16, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants