Problem/Motivation

Following all the work in #3161889: [META] Symfony 6 compatibility, update the Drupal 10 branch to Symfony 6.0.

Proposed resolution

Update to Symfony 6 on the Drupal 10 branch.

Switch to friends-of-behat/mink to resolve the lack of Symfony 6 compatible releases from behat/mink, see #3263841: [Symfony 6] Swap behat/mink for friends-of-behat/mink.

symfony/console must for now stay on Symfony 5.4, until there is a Symfony 6 compatible release of Composer. See #3264918: Update symfony/console to Symfony 6.

Dependency evaluation for friends-of-behat

Maintainance

friends-of-behat has three active maintainers, one of whom is also a behat maintainer.

The project provides several behat plugins that aren't included in the main package, such as Symfony integration, which we don't use.

Additionally, friends-of-behat maintain 'friendly forks' of behat, which apply PRs for PHP 8/Symfony updates before they're available in the main branch, and also tag releases at a faster rate. They don't make any changes other than upstream compatibility fixes:

No updates other than Symfony compatibility will be provided, but this fork might be synchronised with the original repository in the future.

(from https://github.com/FriendsOfBehat/MinkExtension)

Security policy

None, this is a development dependency, but likely to get any security release that mink does.

Code quality

The same as mink with only minimal changes for upstream compatibility.

Remaining tasks

Do it.

User interface changes

None.

API changes

Drupal can use Symfony 6 APIs, but has broken compatibility with anything that was deprecated in Symfony 4 or 5.

Data model changes

None.

Release notes snippet

Drupal 10 updated all Symfony components to Symfony 6, except symfony/console which remains on Symfony 5 with forward compatibility with Symfony 6 while we await a new release of Composer.

CommentFileSizeAuthor
#58 webform versions.png88.64 KBimclean
#57 sms framework versions.png57.81 KBimclean
#35 interdiff.3252757.31-35.txt4.48 KBlongwave
#35 3252757-35.patch82.82 KBlongwave
#31 3252757-30.patch78.8 KBlongwave
#29 3252757-28.patch74.98 KBcatch
#27 3252757-27.patch74.98 KBcatch
#25 3252757-25.patch73.77 KBcatch
#20 3252757-20.patch114.03 KBcatch
#16 3252757-16.patch117.26 KBcatch
#6 3252757-6.patch82.68 KBlongwave
#5 3252757-5.patch85.29 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Postponed

Postponed until #3197482: Update Drupal 10 to depend on Symfony 5.4 (as a stepping stone to Symfony 6, for deprecation checking support) lands.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented
Status: Postponed » Active

The blocker has landed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Active » Postponed

Unil the pieces land in #3161889: [META] Symfony 6 compatibility this cannot be done though, so postponing on that (the children of that to be more precise).

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Postponed » Needs review
FileSize
3252757-5.patch85.29 KB

There aren't any blockers left in Drupal core itself. The two outstanding issues we have are with our dependencies:

  • Composer 2.2 is not compatible with Symfony 6 - we need a new release
  • Behat 1.9 is not compatible with Symfony 6 - we need a new release or switch to a fork

This patch upgrades all dependencies to Symfony 6, except symfony/console for Composer compatibility, and switches Behat for a forked version as per #3263841: [Symfony 6] Swap behat/mink for friends-of-behat/mink

+-------------------------------+--------+--------+
| Production Changes            | From   | To     |
+-------------------------------+--------+--------+
| asm89/stack-cors              | v2.0.5 | v2.1.1 |
| psr/container                 | 1.1.2  | 2.0.2  |
| symfony/console               | v5.4.2 | v5.4.3 |
| symfony/dependency-injection  | v5.4.2 | v6.0.3 |
| symfony/deprecation-contracts | v2.5.0 | v3.0.0 |
| symfony/error-handler         | v5.4.2 | v6.0.3 |
| symfony/event-dispatcher      | v5.4.0 | v6.0.3 |
| symfony/http-foundation       | v5.4.2 | v6.0.3 |
| symfony/http-kernel           | v5.4.2 | v6.0.4 |
| symfony/mime                  | v5.4.2 | v6.0.3 |
| symfony/process               | v5.4.2 | v6.0.3 |
| symfony/routing               | v5.4.0 | v6.0.3 |
| symfony/serializer            | v5.4.2 | v6.0.3 |
| symfony/service-contracts     | v2.5.0 | v3.0.0 |
| symfony/string                | v6.0.2 | v6.0.3 |
| symfony/validator             | v5.4.2 | v6.0.3 |
| symfony/var-dumper            | v5.4.2 | v6.0.3 |
| symfony/yaml                  | v5.4.2 | v6.0.3 |
+-------------------------------+--------+--------+

+------------------------+--------+---------+
| Dev Changes            | From   | To      |
+------------------------+--------+---------+
| behat/mink             | v1.9.0 | REMOVED |
| symfony/browser-kit    | v5.4.2 | v6.0.3  |
| symfony/css-selector   | v5.4.2 | v6.0.3  |
| symfony/dom-crawler    | v5.4.2 | v6.0.3  |
| symfony/filesystem     | v5.4.0 | v6.0.3  |
| symfony/finder         | v5.4.2 | v6.0.3  |
| symfony/lock           | v5.4.2 | v6.0.3  |
| symfony/phpunit-bridge | v5.4.0 | v6.0.3  |
| friends-of-behat/mink  | NEW    | v1.10.0 |
+------------------------+--------+---------+
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3252757-6.patch82.68 KB

composer.lock was broken somehow, trying again.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Hm, I thought at least #3258380: [Symfony 6][Second try] A number of methods of the class Drupal\Core\TypedData\Validation\ExecutionContext are considered internal and Drupal should not override them. was ouststanding? Maybe that would get a solution outside Drupal too?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

@Gábor Hojtsy #3258380: [Symfony 6][Second try] A number of methods of the class Drupal\Core\TypedData\Validation\ExecutionContext are considered internal and Drupal should not override them. is an annoyance, but it's not a blocker. We reverted the original patch, and are stuck with the solution of suppressing those messages, but since they're @internal rather than @deprecated it's in the 'OK until it's not' category - same problem whether we're on Symfony 5 or Symfony 6. The upstream solutions we proposed stalled, so we might end up doing #3054535: Discuss whether to decouple from Symfony Validator instead, but there's no clear path forward there either.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

As we are already compatible with Symfony 5, do we want to release with constraints of ^5.4 || ^6.0? Or is that too dangerous, given that we don't have min-max testing?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

I think we wanted to release with SF 6 clearly, to make sure everyone can develop against that without the need to double check / extra work of supporting SF 5 setups.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

We need a followup for when "symfony/console" is ready to update to 6.0. Now blocked by composer.

Edit: The patch came from #3161889: [META] Symfony 6 compatibility.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Is there a reference for Symfony 6 not being compatible with Composer 2? That is... alarming.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I was at least able apply the patch locally and run composer install and COMPOSER_ROOT_VERSION=10.0.x-dev composer update drupal/core on Composer 2.2.6 without any issues. What's the compatibility problem? Edit: Ah, I see the patch keeps smyfony/console on 5.4. That seems an OK workaround if it's temporary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs followup, +Needs issue summary update

Before committing this, I think we need a followup for Upgrading symfony/console that references whatever upstream issue will hopefully fix that, and possibly a followup for switching back to behat/mink proper if there is an upstream blocker to solve there as well.

Meanwhile, we should probably do a dependency evaluation for the forked version of the package.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch also no longer applies.

Thanks everyone!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
3252757-16.patch117.26 KB

Issue for switching back from friends-of-mink #3264903: Switch from friends-of-behat/mink back to behat/mink once it's Symfony 6 compatible.

Composer now allows symfony/console ^6.0, so I've gone ahead and updated it in the patch.

Will look at a quick dependency evaluation for friends-of-mink

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I've added a dependency evaluation on #3263841: [Symfony 6] Swap behat/mink for friends-of-behat/mink.

IMO we should leave updating https://www.drupal.org/about/core/policies/core-dependency-policies/core... to #3264903: Switch from friends-of-behat/mink back to behat/mink once it's Symfony 6 compatible - i.e. only update the documentation if we have to keep using the fork into beta/rc. I've parented that issue under the beta criteria meta.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
Issue tags: -Needs reroll
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

#16 has downgraded Composer to 2.1:

             "name": "composer/composer",
-            "version": "2.2.4",
+            "version": "2.1.12",

Composer 2.1 technically declares support for symfony/console 6, but from what I could see it doesn't work properly. I think the issue is that Symfony 6 has added return types and so Composer can't do this in a backward-compatible way (in the same way that we have had issues with this).

See https://github.com/composer/composer/issues/10321 and then https://github.com/composer/composer/issues/10322

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3252757-20.patch114.03 KB

OK that is confusing but I see what's going on. Restoring symfony/console: ^5.4 brings composer back to 2.2.

Follow-up here: #3264918: Update symfony/console to Symfony 6.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Some minor tweaks to the IS, but it's up to date with the current plan as far as I can see.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 20: 3252757-20.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
  2x: Passing an escaped locator to the named selector is deprecated as of 1.7 and will be removed in 2.0. Pass the raw value instead.
    2x in DisplayTest::testDisplayPlugin from Drupal\Tests\views\Functional\Plugin

That's a new one to me.

+++ b/composer.lock
@@ -5076,22 +5069,24 @@
             "name": "drupal/coder",
-            "version": "8.3.13",
+            "version": "8.3.14",

We have a separate issue to upgrade this as it has a bunch of new dependencies, I think this issue should only update Symfony and leave the other dependencies alone.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Needs review
FileSize
3252757-25.patch73.77 KB

Reverting the coder update, good spot. Instead of trying to re-roll from the composer.lock, removed it and did the composer update again specifying only symfony/*, behat/*, friends-of-behat/*.

Status: Needs review » Needs work

The last submitted patch, 25: 3252757-25.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Needs review
FileSize
3252757-27.patch74.98 KB

Making this hard for myself :(

Another try.

Status: Needs review » Needs work

The last submitted patch, 27: 3252757-27.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Needs review
FileSize
3252757-28.patch74.98 KB

Status: Needs review » Needs work

The last submitted patch, 29: 3252757-28.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3252757-30.patch78.8 KB

#29 was missing changes to the dev dependencies metapackage and also downgraded symfony/service-contracts:

+++ b/composer.lock
@@ -3811,26 +3797,25 @@
             "name": "symfony/service-contracts",
-            "version": "v2.5.0",
+            "version": "v2.4.1",

Fixed the metapackage, upgraded psr/container which also lets us upgrade symfony/service-contracts, and removed the reference to dealerdirect/phpcodesniffer-composer-installer as that is for the Coder issue to deal with

+-------------------------------+--------+--------+
| Production Changes            | From   | To     |
+-------------------------------+--------+--------+
| psr/container                 | 1.1.2  | 2.0.2  |
| symfony/console               | v5.4.2 | v5.4.3 |
| symfony/dependency-injection  | v5.4.2 | v6.0.3 |
| symfony/deprecation-contracts | v2.5.0 | v3.0.0 |
| symfony/error-handler         | v5.4.2 | v6.0.3 |
| symfony/event-dispatcher      | v5.4.0 | v6.0.3 |
| symfony/http-foundation       | v5.4.2 | v6.0.3 |
| symfony/http-kernel           | v5.4.2 | v6.0.4 |
| symfony/mime                  | v5.4.2 | v6.0.3 |
| symfony/process               | v5.4.2 | v6.0.3 |
| symfony/routing               | v5.4.0 | v6.0.3 |
| symfony/serializer            | v5.4.2 | v6.0.3 |
| symfony/service-contracts     | v2.5.0 | v3.0.0 |
| symfony/string                | v6.0.2 | v6.0.3 |
| symfony/validator             | v5.4.2 | v6.0.3 |
| symfony/var-dumper            | v5.4.2 | v6.0.3 |
| symfony/yaml                  | v5.4.2 | v6.0.3 |
+-------------------------------+--------+--------+

+------------------------+--------+---------+
| Dev Changes            | From   | To      |
+------------------------+--------+---------+
| behat/mink             | v1.9.0 | REMOVED |
| symfony/browser-kit    | v5.4.2 | v6.0.3  |
| symfony/css-selector   | v5.4.2 | v6.0.3  |
| symfony/dom-crawler    | v5.4.2 | v6.0.3  |
| symfony/filesystem     | v5.4.0 | v6.0.3  |
| symfony/finder         | v5.4.2 | v6.0.3  |
| symfony/lock           | v5.4.2 | v6.0.3  |
| symfony/phpunit-bridge | v5.4.0 | v6.0.3  |
| friends-of-behat/mink  | NEW    | v1.10.0 |
+------------------------+--------+---------+

Status: Needs review » Needs work

The last submitted patch, 31: 3252757-30.patch, failed testing. View results

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review 
+++ b/core/composer.json
@@ -19,18 +19,18 @@
diff --git a/core/lib/Drupal/Core/Composer/Composer.php b/core/lib/Drupal/Core/Composer/Composer.php

diff --git a/core/lib/Drupal/Core/Composer/Composer.php b/core/lib/Drupal/Core/Composer/Composer.php
index 1b6bf97b46..e436bf22e9 100644

index 1b6bf97b46..e436bf22e9 100644
--- a/core/lib/Drupal/Core/Composer/Composer.php

--- a/core/lib/Drupal/Core/Composer/Composer.php
+++ b/core/lib/Drupal/Core/Composer/Composer.php

+++ b/core/lib/Drupal/Core/Composer/Composer.php
+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -16,13 +16,13 @@

@@ -16,13 +16,13 @@
 class Composer {
 
   protected static $packageToCleanup = [
-    'behat/mink' => ['tests', 'driver-testsuite'],
     'behat/mink-selenium2-driver' => ['tests'],
     'composer/composer' => ['bin'],
     'drupal/coder' => ['coder_sniffer/Drupal/Test', 'coder_sniffer/DrupalPractice/Test'],
     'doctrine/instantiator' => ['tests'],
     'easyrdf/easyrdf' => ['scripts'],
     'egulias/email-validator' => ['documentation', 'tests'],
+    'friends-of-behat/mink' => ['tests'],
     'friends-of-behat/mink-browserkit-driver' => ['tests'],
     'guzzlehttp/promises' => ['tests'],
     'guzzlehttp/psr7' => ['tests'],
 

You are missing this change.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Back to NW.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3252757-35.patch82.82 KB
interdiff.3252757.31-35.txt4.48 KB

Argh, we will get there in the end! We also need to upgrade asm89/stack-cors for Symfony 6 compatibility.

+-------------------------------+--------+--------+
| Production Changes            | From   | To     |
+-------------------------------+--------+--------+
| asm89/stack-cors              | v2.0.5 | v2.1.1 |
| psr/container                 | 1.1.2  | 2.0.2  |
| symfony/console               | v5.4.2 | v5.4.3 |
| symfony/dependency-injection  | v5.4.2 | v6.0.3 |
| symfony/deprecation-contracts | v2.5.0 | v3.0.0 |
| symfony/error-handler         | v5.4.2 | v6.0.3 |
| symfony/event-dispatcher      | v5.4.0 | v6.0.3 |
| symfony/http-foundation       | v5.4.2 | v6.0.3 |
| symfony/http-kernel           | v5.4.2 | v6.0.4 |
| symfony/mime                  | v5.4.2 | v6.0.3 |
| symfony/process               | v5.4.2 | v6.0.3 |
| symfony/routing               | v5.4.0 | v6.0.3 |
| symfony/serializer            | v5.4.2 | v6.0.3 |
| symfony/service-contracts     | v2.5.0 | v3.0.0 |
| symfony/string                | v6.0.2 | v6.0.3 |
| symfony/validator             | v5.4.2 | v6.0.3 |
| symfony/var-dumper            | v5.4.2 | v6.0.3 |
| symfony/yaml                  | v5.4.2 | v6.0.3 |
+-------------------------------+--------+--------+

+------------------------+--------+---------+
| Dev Changes            | From   | To      |
+------------------------+--------+---------+
| behat/mink             | v1.9.0 | REMOVED |
| symfony/browser-kit    | v5.4.2 | v6.0.3  |
| symfony/css-selector   | v5.4.2 | v6.0.3  |
| symfony/dom-crawler    | v5.4.2 | v6.0.3  |
| symfony/filesystem     | v5.4.0 | v6.0.3  |
| symfony/finder         | v5.4.2 | v6.0.3  |
| symfony/lock           | v5.4.2 | v6.0.3  |
| symfony/phpunit-bridge | v5.4.0 | v6.0.3  |
| friends-of-behat/mink  | NEW    | v1.10.0 |
+------------------------+--------+---------+
dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Status: Needs review » Reviewed & tested by the community
  1. Checked out latest 10.0.x core (commit ).
  2. Patch applies cleanly.
  3. Ran composer install. Success.
  4. vendor/symfony/http-foundation/CHANGELOG.md says "6.0"
  5. Testbot is still happy.
  6. Reviewed the rest of the patch (it's ~97% composer.lock diff) and didn't spot any trouble.

Not sure what else to review. RTBC for me!

Thanks,
-Derek

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented

https://www.drupal.org/node/3252757/revisions/view/12558411/12558412 looks great, thanks for including that. It resolves a point @xjm brought up in Slack about the relationship to #3263841: [Symfony 6] Swap behat/mink for friends-of-behat/mink. I believe that's now going to be marked duplicate with this.

Anyway, even more RTBC now. 😉 Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

symfony/console | v5.4.2 | v5.4.3 |

Is there follow-up to upgrade it too?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes
Related issues: +#3264918: Update symfony/console to Symfony 6

Fixed IS for it #3264918: Update symfony/console to Symfony 6

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Rtbc +1

  • catch committed 2add9d8 on 10.0.x 
    Issue #3252757 by catch, longwave, Gábor Hojtsy, xjm, daffie, andypost,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

I only contributed (broken) rerolls here so I think I'm still OK to commit.

Committed 2add9d8 and pushed to 10.0.x. Thanks!

Symfony 6.1 next ;)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +10.0.0 release notes
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Drush cannot be composer required/updated now


  Problem 1
    - enlightn/security-checker[v1.0, ..., v1.1] require symfony/finder ^4.0|^5.0 -> found symfony/finder[v4.0.0-BETA1, ..., 4.4.x-dev, v5.0.0-BETA1, ..., 5.4.x-dev] but it conflicts with your root composer.json require (^6.0).
    - enlightn/security-checker[v1.2, ..., v1.9.0] require symfony/finder ^3|^4|^5 -> found symfony/finder[v3.0.0-BETA1, ..., 3.4.x-dev, v4.0.0-BETA1, ..., 4.4.x-dev, v5.0.0-BETA1, ..., 5.4.x-dev] but it conflicts with your root composer.json require (^6.0).
    - drush/drush 11.x-dev requires enlightn/security-checker ^1 -> satisfiable by enlightn/security-checker[v1.0, ..., v1.9.0].
    - Root composer.json requires drush/drush 11.x-dev -> satisfiable by drush/drush[11.x-dev].
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

https://github.com/drush-ops/drush/issues/5077

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

Drush 11 work-in-progress on Drupal 10 / Symfony 6 compatibility: https://github.com/drush-ops/drush/pull/5078

TR’s picture

TR CreditAttribution: TR commented

Again, this is another huge change, can we at least get a change notice?

This broke D9 compatibility in my contrib module because of Symfony 6's use of the 'mixed' return type hint, which is only available in PHP 8.

(EDIT: Actually, the bigger issue is Symfony 6's used of the 'mixed' function parameter. The return type hint error can be ignored with #[\ReturnTypeWillChange], but there's no mechanism to do that for a function parameter.)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@TR the CR is https://www.drupal.org/node/3236232 but the issue has release notes snippet (which is more meaningful)

Some messages could be improved, for example #3265419: Improve deprecation message for RequestStack::getMasterRequest()

TR’s picture

TR CreditAttribution: TR commented

@andypost: No, that's not the same. That CR is for introducing return type hints into D9 in preparation for Symfony 5.4/6. This issue is about doing away with Symfony 5.4 entirely and switching to Symfony 6. And specifically the thing that's causing me a problem here is SessionInterface, which doesn't show up in any CR at all and which had backwards-incompatible changes between Symfony 5.4 and Symfony 6.

My code broke with the change from Symfony 5.4 to Symfony 6. Specifically, because Symfony 6 uses 'mixed' function parameters and return type hints - these are only available in PHP 8. While I can use #[\ReturnTypeWillChange] to deal with the return type hints, there's nothing I can do about the function parameters apparently. I now need different code because something that runs in D9 will not run in D10, and vice versa. This is a big change!

While some breakage is to be expected, it would be nice to have a CR of what this change was, so I can figure out WHY the code broken and what else to expect with the change.

Also, if the expectation is that a module should be able to be compatible with both the latest D9 and with D10.0.0, (like we did with D8.9 and D9.0), then because of this change that is no longer true in general. If this is intentional, then fine, but it should be explicitly noted in a CR.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

@TR I've added a change record, but it doesn't say any more than the release note. You can add a change record yourself for an issue if you notice one that's missing.

While I can use #[\ReturnTypeWillChange] to deal with the return type hints, there's nothing I can do about the function parameters apparently.

Is this because the API is different, or because you'd need to increase your PHP requirement?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I now need different code because something that runs in D9 will not run in D10, and vice versa.

Can you give an example here? Parameter type contravariance should mean you don't have to add these types until you are ready: https://3v4l.org/LdCsl

(Return types are covariant - the other way around - which is why we are forced to add return types in places)

TR’s picture

TR CreditAttribution: TR commented

https://3v4l.org/BabgE

Can you make that implementation work in both PHP 7 and PHP 8?

EDIT. Here's the example, for when the link dies:

<?php

interface I {
    // In Symfony 5.4 this is:
    // public static function test($s, $i, $m);
    // In Symfony 6.0 this is:
    public static function test(string $s, int $i, mixed $m): mixed;
}

class A implements I {
    public static function test($s, $i, $m) {
        var_dump($s, $i, $m);
        return NULL;
    }
}

A::test('string', 1, NULL);
TR’s picture

TR CreditAttribution: TR commented

I've added a change record

Thank you.

Is this because the API is different, or because you'd need to increase your PHP requirement?

Symfony 6 added some return type hints and parameter type hints. I would have to require PHP 8 in Drupal 9 to get my code to work in both D9 and D10 at the same time. I am unwilling to do that because most D9 sites now and for years into the future will be using PHP 7. See #53 for an example.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

@TR you have three options:

1. Require PHP 8 - it's entirely valid to require PHP 8, PHP 7.4 will be EOL in nine months.
2. Conditionally define your class depending on the Drupal version, not really suited to 3v4l.org but a rough example here (ignore the PHP 7.4 error, the Drupal 10 code doesn't need to run on 7.4 https://3v4l.org/QnVTu)
3. Two branches

TR’s picture

TR CreditAttribution: TR commented

Yes, I'm doing 2).

I used conditional code for 2 years to support both D8 and D9, and now that D8 is no longer supported I was happy to get rid of that conditional code ... sadly, that has only lasted about three months, and now I need more :-(

imclean’s picture

imclean CreditAttribution: imclean at Digital Ink commented
FileSize
sms framework versions.png57.81 KB

I've seen a few modules use the general approach in point 3. It gets complicated when multiple PHP and Drupal versions need to be supported. SMS Framework is one example.

Recurring Dates Field is another.

imclean’s picture

imclean CreditAttribution: imclean at Digital Ink commented
FileSize
webform versions.png88.64 KB

Webform takes a similar approach.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle as a volunteer commented

Supporting two Drupal major versions with 1 contrib module release is not really an option, if the module is using lot of the core apis, or want to use latest core functionalities.
The easy solution is to always create a separate module branch for core majors (D9, D10 ..), otherwise the module versioning will become a mess as the above screenshots show.
Same recommendation as I wrote 3 years ago:
#2807145-27: [policy, no patch] Allow contrib projects to specify multiple major core branches

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented

otherwise the module versioning will become a mess as the above screenshots show.

As author of one of the screenshot mess' above, supporting two major versions of a dependency will depend on what happens over time (indicated by "Future" / TBD text). At worst I would end up supporting the old core version in a minor, and the newer one in a later minor. Personally I'm aggressive with cutting off support for the older core version so I'm not backporting features, only bugs and sec. But if you intend to support multiple versions of Drupal core for a while then you may think of your own versioning strategy. A new major version of your project is not always required, but it is "The easy solution".

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.