Skip to content

Update CI workflow to use current versions of PHP and only Symfony versions still under maintenance #38

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.

Sign up for GitHub

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

Open
mpdude wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Update CI workflow to use current versions of PHP and only Symfony versions still under maintenance #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eedfe24
ci: add tests for PHP > 8.3
Chris8934 Nov 7, 2025
4d051fa
ci: add tests for PHP > 8.3
Chris8934 Nov 7, 2025
3517e91
ci: add tests for PHP > 8.3
Chris8934 Nov 7, 2025
67adb9a
ci: add tests for PHP > 8.3
Chris8934 Nov 7, 2025
b648d7b
ci: add tests for PHP > 8.3
Chris8934 Nov 8, 2025
58eb188
Update current PHP and Symfony versions
mpdude Dec 17, 2025
d260a41
Avoid using Flex
mpdude Dec 17, 2025
91a118c
Update eligible versions of PHPSpec
mpdude Dec 17, 2025
a06cf4b
Merge branch 'master' into update-workflows
mpdude Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ name: Test
on:
push:
pull_request:
types: [opened, synchronize, edited, reopened]
Copy link

@aik099 aik099 Dec 18, 2025

Choose a reason for hiding this comment

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

Why this line was removed?

Copy link
Author

@mpdude mpdude Dec 18, 2025

Choose a reason for hiding this comment

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

I had the impression that it would trigger action runs unnecessarily, and most (all?) other open source repos I have experience with don't use it either. It will "do the right thing" by default, run the workflows whenever changes are pushed to the branch.

Copy link

@aik099 aik099 Dec 21, 2025

Choose a reason for hiding this comment

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

I see.

I'll wait for @stof comments to be addressed before continuing with my review.


jobs:
test:
Expand All @@ -14,21 +13,19 @@ jobs:
fail-fast: false
matrix:
php-version:
- '7.4'
- '8.0'
Comment on lines -17 to -18
Copy link

@aik099 aik099 Dec 18, 2025

Choose a reason for hiding this comment

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

We do support PHP 7.4. Why have you removed them from the build?

Copy link
Author

@mpdude mpdude Dec 18, 2025

Choose a reason for hiding this comment

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

Do we still want to support it in the next minor release? That would be an opportunity to change it, and PHP 7.4 had its EOL three years ago.

Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

dropping support for some PHP versions should be done in a dedicated PR as this is a change that deserves dedicated communication. It cannot be hidden in a PR saying "update CI".

- '8.1'
- '8.3'
- '8.4'
# PHPSpec does not yet (2025/12) support PHP 8.5
# - '8.5'
symfony-version:
Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

I suggest refactoring the CI setup to have normal job in the matrix with no enforcement of the Symfony version (letting composer resolve them as would be done in normal projects) and then have only specific jobs included to force testing against Symfony LTS versions (if worth it, not sure for that package)

Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

And I would probably not add jobs for the 4.4 and 5.4 LTS versions.

- '4.4.*'
- '5.4.*'
- '6.4.*'
- '7.4.*'
exclude:
- php-version: '7.4'
symfony-version: '6.4.*'
- php-version: '8.0'
symfony-version: '6.4.*'
include:
- php-version: '8.2'
symfony-version: '7.0.*'
# Symfony 7.4 requires PHP >= 8.2
- php-version: '8.1'
symfony-version: '7.4.*'
steps:
- name: Checkout
uses: actions/checkout@v6
Expand All @@ -39,16 +36,28 @@ jobs:
coverage: none
ini-values: "memory_limit=-1"
php-version: ${{ matrix.php-version }}
tools: composer:v2
tools: composer:v2, flex

- name: Validate composer.json
run: composer validate --no-check-lock

- name: Restore cached dependencies
uses: actions/cache@v4
with:
path: vendor
key: composer-${{ runner.os }}-${{ matrix.php-version }}-${{ matrix.symfony-version }}-${{ hashFiles('composer.json') }}
restore-keys: |
composer-${{ runner.os }}-${{ matrix.php-version }}-${{ matrix.symfony-version }}-
Comment on lines +44 to +50
Copy link

@aik099 aik099 Dec 18, 2025

Choose a reason for hiding this comment

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

Why?

Copy link
Author

@mpdude mpdude Dec 18, 2025

Choose a reason for hiding this comment

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

To cache downloaded packages, to make the planet a bit more 🌱?

Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

the question is whether our CI runs often enough to benefit from the caching of downloads (or whether we consume resources to store the cache but then don't benefit from cache hits)

Copy link
Author

@mpdude mpdude Dec 18, 2025

Choose a reason for hiding this comment

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

If it helps us to get this PR along, I’ll remove it


# We currently cannot use Flex to control versions of Symfony and its components, since that would also impose constraints
# that cannot anymore be fulfilled by Goutte.
Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

Redefining requirements means we don't actually respect the requirements in composer.json, which is bad. We should be using Flex here (maybe removing the goutte-driver dependency conditionally in those jobs, and ensuring that tests that depend on it are properly skipped)

- name: Configure Symfony version
run: composer require --no-update "symfony/config:${{ matrix.symfony-version }}" "symfony/dependency-injection:${{ matrix.symfony-version }}"

- name: Install dependencies
- name: Install dependencies with targeted Symfony version
Copy link
Member

@stof stof Dec 18, 2025

Choose a reason for hiding this comment

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

I would not change the step name here

run: composer install --prefer-dist --no-progress
# env:
# SYMFONY_REQUIRE: ${{ matrix.symfony-version }}

- name: PHPSpec
run: vendor/bin/phpspec run -f pretty
Expand Down
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
},
"require-dev": {
"behat/mink-goutte-driver": "^1.1 || ^2.0",
"phpspec/phpspec": "^6.0 || ^7.0 || 7.1.x-dev",
"phpspec/phpspec": "^7.0 || ^8.0",
Copy link

@aik099 aik099 Dec 18, 2025

Choose a reason for hiding this comment

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

Why the ^6.0 version was removed and 8.1.x-dev version wasn't added?

Copy link
Author

@mpdude mpdude Dec 18, 2025

Choose a reason for hiding this comment

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

My guess was that back at the time this was written and with the PHP versions current back then, it was necessary to run tests with either a 6.x or an unstable 7.1.* version.

As of today (and with the PHP version change above), stable 7.x or 8.x versions work just fine, so why would we want anything else?

"mink/webdriver-classic-driver": "^1.0@dev"
},
"replace": {
Expand Down