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

Use setup-php instead of php-actions so that all CI steps are run using the same PHP version #1649

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

osma
Copy link
Member

@osma osma commented Aug 15, 2024

Reasons for creating this PR

While working on PR #1648 , I noticed a problem with the way we run PHP-CS-Fixer under GitHub Actions. We use php-actions/composer and php-actions/phpunit for running Composer and PHPUnit using selected PHP versions (currently 8.0, 8.1 and 8.2) inside a separate container. However, PHP-CS-Fixer is run using the PHP version provided by the Ubuntu base system, which is currently 8.1. This causes a problem when Composer, running under PHP 8.2, installs some dependencies (in this case Symfony 7 modules) that are not compatible with PHP 8.1, causing the PHP-CS-Fixer step to fail.

It seems that it's not easy to run PHP-CS-Fixer using php-actions containers, as there is no action specific to this tool. So instead this PR changes our GitHub Actions CI workflow to use the setup-php action instead, which works a bit differently than php-actions. Setup-php installs the desired version of PHP within the base system and then the subsequent steps in the workflow can run Composer, PHP-CS-Fixer and PHPUnit all using that same PHP version.

The workflow seems to take around 6 minutes to run, which is about the same as before, possibly a little bit faster, but there is a lot of variation so it's a bit hard to say. At least it's not significantly slower!

Link to relevant issue(s), if any

Description of the changes in this PR

  • Replaced php-actions/composer with shivammathur/setup-php for setting up PHP and installing Composer dependencies using composer install.
  • Replaced php-actions/phpunit with a direct command to run PHPUnit tests using ./vendor/bin/phpunit.
  • Added PHP extensions and coverage tool configuration in the setup-php step.

Known problems or uncertainties in this PR

Not 100% sure about whether the caching behavior is correct, but the only way to find out is to use this for a while..

I noticed that some of the actions we use are older versions that probably should be upgraded. Also we could consider upping the PHP versions (drop 8.0, add support for 8.3) and use a newer Node.js version in the workflow. However, those are a bit out of scope for this PR that just switches to setup-php.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma self-assigned this Aug 15, 2024
@osma osma requested a review from joelit August 15, 2024 08:29
Copy link

sonarcloud bot commented Aug 15, 2024

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

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

All I can say is that the new dependency seems pretty well maintained and documented. LGTM

@osma osma merged commit 3559996 into main Aug 15, 2024
9 of 10 checks passed
@osma osma deleted the use-setup-php-action branch August 15, 2024 12:43
@osma osma added this to the 3.0 milestone Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

2 participants