[4.x] feat: Add Symfony 8 support #1687
Conversation
|
you can also switch to php-cs-fixer/shim and not be blocked by dependencies of Fixer |
|
Oh yeah you're right, I forgot about this package. Thanks! :) |
a8a3a0c to
9f4f929
Compare
|
Seems like you forgot one place return type hint @Kocal |
dec2ab7 to
f2c6a86
Compare
|
Oops, thanks, it should be fine now! :) There is still a small issue here, someone from Symfony must create the tag |
|
Constraint will be relaxed symfony/symfony#62336 |
acoulton
left a comment
There was a problem hiding this comment.
@Kocal thanks for doing this.
There are a few classes where adding the void return typehint required by the Symfony 8 CompilerPassInterface interface will technically break BC in Behat.
In some of these cases I'm not actually sure why our class isn't final as most of them are and they don't look like things we'd want users to extend. So the risk of breaking BC on those is fairly small.
However the one that's not so visible in the diff (except in the feature examples) is obviously our Extension interface itself, because that extends the symfony one.
So any/every third-party Behat extension a project is using will also need to be updated to add the return typehints before it can be used with symfony 8 (and may not itself have a dependency on the symfony version, since they are apparently only implementing "our" interface).
I'm therefore leaning to thinking this has to be a new major Behat version. We could make that a very small release, with just this and a few other "shouldn't affect anyone but could do" internal things that we've been holding off on, to make it easy for end-users to update.
There are bigger plans for "a future Behat major" but IMO we should probably keep them separate from one that's required for dependency version support.
This would mean that people will need to wait for third-party extensions to update their composer requirements to allow the new Behat major - but since they'd also need to have the return typehints to work in a project with symfony 8 that probably isn't a big deal...
I'm still deciding what I think about this, opinions/arguments very welcome.
.github/workflows/build.yml
Outdated
| SYMFONY_REQUIRE: ${{ matrix.symfony-version }}.* | ||
| run: composer update ${{ matrix.php == '8.5' && '--ignore-platform-req=php+' || '' }} | ||
| SYMFONY_REQUIRE: ${{ matrix.symfony-version }} | ||
| run: composer update ${{ (matrix.php == '8.4' || matrix.php == '8.5') && '--ignore-platform-req=php+' || '' }} |
There was a problem hiding this comment.
Do we actually still need to ignore platform requirements on PHP 8.4? It's passing without this in master currently and I don't think symfony 8 will add any non-php-84 dependencies?
There was a problem hiding this comment.
I'm still not clear on the rationale for this change. I've just tried installing locally on 8.4 with the SYMFONY_REQUIRE set to 8.0.* and I'm getting the same packages with & without the --ignore-platform-req flag.
AFAICS we only need that on 8.5 to allow us to install with dependencies that don't officially support 8.5 yet.
There was a problem hiding this comment.
I don't really remember why TBH, I will remove it
src/Behat/Testwork/EventDispatcher/ServiceContainer/EventDispatcherExtension.php
Show resolved
Hide resolved
|
@acoulton I'm in favor of doing a new major version focused on native typing on extension-impacting APIs (and potentially making some of our classes final when they should be) instead of shipping such BC breaks in a minor. Our big plans for a huge rewrite of Behat based on the cucumber pickle concept in a next major version does not need to be named 4.0. It can be a later major version number. |
3.6.1 was tagged @Kocal |
992eafd to
4f798cb
Compare
|
The PR is finally green! Indeed, creating a new major release and marking some classes as final/internal would be much safer to prevent any potential BC in user-land. |
|
OK, we'll get a 4.x branch going / updated for us to start working towards a (small) 4.0 - will get this merged as soon as we have something clean to merge to (the current 4.x is very out of date / essentially abandoned and we need to work out the best way to solve that - see #1692). |
Extracted from work by @Kocal in Behat#1687 - these return types will be required in 4.x for symfony8 support, but can be added in 3.x without any BC impact as they are either final classes or internal test code. This will reduce potential for future conflicts merging 3.x up to 4.x.
|
Oops - github automatically closed this when I deleted & recreated the 4.x branch. This can now be rebased. |
|
Will do tonight, thanks for cherry-pickings :) |
a2ad621 to
da50c2c
Compare
|
PR rebased! |
.github/workflows/build.yml
Outdated
| os: ubuntu-latest | ||
| composer-mode: update | ||
| composer-minimum-stability: beta | ||
| symfony-version: '^8.0.0-BETA-1' |
There was a problem hiding this comment.
I think with the minimum-stability set to beta, we don't need to explicitly specify it in the version constraint as well - if I install locally on this branch with SYMFONY_REQUIRE=8.0.* I'm getting the same packages as the actions run installed here.
| symfony-version: '^8.0.0-BETA-1' | |
| symfony-version: '8.0.*' |
.github/workflows/build.yml
Outdated
| SYMFONY_REQUIRE: ${{ matrix.symfony-version }}.* | ||
| run: composer update ${{ matrix.php == '8.5' && '--ignore-platform-req=php+' || '' }} | ||
| SYMFONY_REQUIRE: ${{ matrix.symfony-version }} | ||
| run: composer update ${{ (matrix.php == '8.4' || matrix.php == '8.5') && '--ignore-platform-req=php+' || '' }} |
There was a problem hiding this comment.
I'm still not clear on the rationale for this change. I've just tried installing locally on 8.4 with the SYMFONY_REQUIRE set to 8.0.* and I'm getting the same packages with & without the --ignore-platform-req flag.
AFAICS we only need that on 8.5 to allow us to install with dependencies that don't officially support 8.5 yet.
Symfony 8 added return types to all interfaces and classes. This leaks through the Behat API in places where Behat used classes from Symfony, for example the Event Dispatcher implementation or the `\Behat\Testwork\ServiceContainer\Extension` interface which extends `\Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface`. The Behat maintainers work on creating a new Behat release supporting Symfony 8, but it will also affect extensions like this one (Behat/Behat#1687).
|
Is there a list of items that need to be worked on for this minimal-changes 4.0 release? |
|
@mpdude sort of - I have started a 4.0 milestone, with some of the existing issues that I think are relevant - but there are some things (e.g. adding strict types throughout) that still exist in our heads / in comments on other issues. I'll try to create a proper 4.0 release planning issue later today, or tomorrow. |
|
Thank you! I was asking because I'd like to help extension maintainers to take care of the necessary changes and make new releases compatible to a future 4.x themselves – something that I expect to take some time and that blocks some of my projects from doing "clean" Symfony 8 upgrades (without installing pending PR branches of things through Composer). Selling changes to extension maintainers, especially when it comes to declaring compat with Behat 4 would be much easier if 4.0 were already a real thing 😉 . Keep up the good work! It's amazing to see how much things have been moving here over the past weeks. |
|
For extension maintainers, the main necessary change will be to use native types when they override/implement methods coming from Behat (and requiring at least PHP 7.2+ so that they can benefit from PHP variance rules to be compatible with both 3.x and 4.x of Behat) |
|
Would you recommend that extension authors declare (Yes, I have seen #1316 and I see that there might be better solutions that make the problem go away.) |
|
I think that's probably one of the first questions we need to research/answer. My strong preference would be for us to define our own clean interfaces (which would coincidentally be the same as the current symfony ones, but might diverge in future). And then extension developers would not be tied to any particular symfony dependency unless they were using symfony features directly. However it needs someone to dig in to how complex that would be to implement / support in the Behat codebase in the first instance. |
Symfony 8 added return types to all interfaces and classes. This leaks through the Behat API in places where Behat used classes from Symfony, for example the Event Dispatcher implementation or the `\Behat\Testwork\ServiceContainer\Extension` interface which extends `\Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface`. The Behat maintainers work on creating a new Behat release supporting Symfony 8, but it will also affect extensions like this one (Behat/Behat#1687). We cannot yet tell what Behat 4 will look like and whether any other changes will be required, but this change here is a fix that can be applied as of today
The extension code already has the required return types on process() methods, making it compatible with both Behat 3.x and 4.x interfaces. Changes: - Update behat/behat constraint to ^3.22 || ^4.0 - Update symfony/* dependencies to include ^8.0 - Add CI matrix entry for Symfony 8.0 with Behat 4.x-dev Note: Symfony 8 requires Behat 4.x-dev as Behat 3.x doesn't declare Symfony 8 compatibility in its composer.json constraints. Relates to Behat/Behat#1687
Related to FriendsOfBehat/SymfonyExtension#218
This PR increase version constraints on
symfony/*packages to allow Symfony 8, which will be released this month.The CI fails because no version of PHP-CS-Fixer is compatible with Symfony 8, I'm on it.EDIT: switched to
php-cs-fixer/shimpackage