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

mpdude · 2025-12-17T08:37:48Z

No description provided.

…rsions still under maintenance Squash-merge of FriendsOfBehat#38 Squashed commit of the following: commit 91a118c Author: Matthias Pigulla <mp@webfactory.de> Date: Wed Dec 17 09:35:55 2025 +0100 Update eligible versions of PHPSpec commit d260a41 Author: Matthias Pigulla <mp@webfactory.de> Date: Wed Dec 17 09:29:15 2025 +0100 Avoid using Flex commit 58eb188 Author: Matthias Pigulla <mp@webfactory.de> Date: Wed Dec 17 09:20:16 2025 +0100 Update current PHP and Symfony versions commit b648d7b Author: Christopher Georg <christopher.georg@sr-travel.de> Date: Sat Nov 8 08:40:43 2025 +0100 ci: add tests for PHP > 8.3 commit 67adb9a Author: Christopher Georg <christopher.georg@sr-travel.de> Date: Fri Nov 7 15:41:28 2025 +0100 ci: add tests for PHP > 8.3 commit 3517e91 Author: Christopher Georg <christopher.georg@sr-travel.de> Date: Fri Nov 7 15:40:19 2025 +0100 ci: add tests for PHP > 8.3 commit 4d051fa Author: Christopher Georg <christopher.georg@sr-travel.de> Date: Fri Nov 7 15:39:17 2025 +0100 ci: add tests for PHP > 8.3 commit eedfe24 Author: Christopher Georg <christopher.georg@sr-travel.de> Date: Fri Nov 7 15:36:47 2025 +0100 ci: add tests for PHP > 8.3

aik099 · 2025-12-18T11:49:39Z

.github/workflows/test.yaml

-                    - '7.4'
-                    - '8.0'


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

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.

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".

aik099 · 2025-12-18T11:49:52Z

.github/workflows/test.yaml

 on:
    push:
    pull_request:
-        types: [opened, synchronize, edited, reopened]


Why this line was removed?

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.

I see.

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

aik099 · 2025-12-18T11:50:33Z

.github/workflows/test.yaml

+            - 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 }}-


To cache downloaded packages, to make the planet a bit 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)

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

aik099 · 2025-12-18T11:51:21Z

composer.json

    "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",


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

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?

stof · 2025-12-18T13:34:56Z

.github/workflows/test.yaml

-                    - '7.4'
-                    - '8.0'


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".

stof · 2025-12-18T13:36:16Z

.github/workflows/test.yaml

+            - 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 }}-


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)

stof · 2025-12-18T13:40:49Z

.github/workflows/test.yaml

+                    - '8.4'
+                    # PHPSpec does not yet (2025/12) support PHP 8.5
+                    # - '8.5'
                symfony-version:


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)

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

stof · 2025-12-18T14:11:24Z

.github/workflows/test.yaml

+                      composer-${{ runner.os }}-${{ matrix.php-version }}-${{ matrix.symfony-version }}-
+
+            # 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.


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)

stof · 2025-12-18T14:11:47Z

.github/workflows/test.yaml

              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


I would not change the step name here

mpdude · 2025-12-19T06:56:19Z

I think it would be much easier if we accept that the workflow is as it is, maybe even with failures for some jobs.

Let’s focus on merging what needs to go into 2.x.

Then agree on minimum requirements for 3.x, drop stuff and clean up the workflow when we have no outdated dependencies getting in our way.

WDYT?

stof · 2025-12-19T09:45:01Z

the existing workflow fails, so we need to fix things in 2.x, not in 3.x only.

mpdude · 2025-12-19T17:35:29Z

The Symfony 4.4 workflow fails because it installs Behat 3.15 with Gherkin 4.16.

Behat 3.15 made particular assumptions about how files are organized inside the Gherkin package, and that broke due to a file reorg in Gherkin.

Behat/Behat@230c0f5 fixed the issue for Behat 3.20 and above, but we cannot get that fix (it's Symfony 5.4+).

Seriously, please let's not waste time trying to fix that workflow for Symfony 4.4.

Instead, put all the changes we need into the 2.8 release and raise dependencies to Symfony 5.4 or even above and carry on.

Agree?

stof · 2025-12-19T17:36:44Z

See my previous comment there: #38 (comment)

mpdude · 2025-12-19T17:39:20Z

Please disregard this PR for the time being. Maybe I'll resume here later, maybe I'll close it.

mpdude · 2025-12-19T17:48:14Z

I don't get it. Could you please make a list which combinations of PHP and Symfony you'd like to see tested on 2.x (2.7?), before we possibly raise requirements in 2.8?

Like so? https://github.com/mpdude/MinkExtension/pull/3/changes

Chris8934 and others added 8 commits November 7, 2025 15:36

ci: add tests for PHP > 8.3

eedfe24

ci: add tests for PHP > 8.3

4d051fa

ci: add tests for PHP > 8.3

3517e91

ci: add tests for PHP > 8.3

67adb9a

ci: add tests for PHP > 8.3

b648d7b

Update current PHP and Symfony versions

58eb188

Avoid using Flex

d260a41

Update eligible versions of PHPSpec

91a118c

aik099 suggested changes Dec 18, 2025

View reviewed changes

Merge branch 'master' into update-workflows

a06cf4b

stof reviewed Dec 18, 2025

View reviewed changes

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

mpdude commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented Dec 19, 2025

Uh oh!

stof commented Dec 19, 2025

Uh oh!

mpdude commented Dec 19, 2025

Uh oh!

stof commented Dec 19, 2025

Uh oh!

mpdude commented Dec 19, 2025

Uh oh!

mpdude commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mpdude commented Dec 19, 2025 •

edited

Loading