Problem/Motivation

Drupal 12 will release on Symfony 8.

Steps to reproduce

Proposed resolution

Update to Symfony 8. Fix any breaks.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3570077

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3555534: Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead., +#3555532: Since symfony/http-foundation 7.4: Request::get() is deprecated, use properties ->attributes, query or request directly instead.

Currently blocked on

#3555534: Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead.
#3555532: Since symfony/http-foundation 7.4: Request::get() is deprecated, use properties ->attributes, query or request directly instead.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Also blocked by php-tuf/composer-stager not supporting Symfony 8 yet, opened https://github.com/php-tuf/composer-stager/pull/436

longwave opened merge request !14528

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Also blocked by:

https://github.com/mglaman/phpstan-drupal/pull/933
https://github.com/asm89/stack-cors/pull/109

but for now we can work around this by referencing forks in composer.json.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Needs work
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Update to Symfony 8 » [PP-upstream] Update to Symfony 8
Status: Needs work » Postponed

Tests should be green except for build tests, which are waiting on our dependencies to update. Postponing again for now.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

All of the mentioned dependency issues landed since they were linked. Are we awaiting releases? Let me check:

So looks like it is down to phpstan-drupal only?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

So the reason that was not yet released is phpstan 2.1.38 changed stub handling, which is breaking phpstan-drupal https://github.com/mglaman/phpstan-drupal/compare/claude/debug-phpstan-s... is a proposal to fix it by restoring some of the 2.1.37 behaviour and https://github.com/phpstan/phpstan/issues/14118 is the issue.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +12.0.0 release highlights
godotislate’s picture

godotislate
he/him
commented

Looks like part of #3569092: Update method signatures for addConstraint() and ConstraintManager::create() and remove HasNamedArguments use is in the MR here (or there's overlap at least), but the #[HasNamedArguments] attribute has been removed from Symfony 8, and is in the MR there.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@godotislate I don't think it's possible to get a green test run without making some of those changes here, but we should try to minimise them if possible.

godotislate’s picture

godotislate
he/him
commented

Oh, interesting, I thought all the #[HasNamedArguments] usage would be more of a problem, but I guess at core's PHPStan level, it's ignored?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -12.0.0 release highlights

@longwave Looks like more changes needed as RequestSanitizerTest fails

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +12.0.0 release highlights
andypost’s picture

andypost
he/him
Primary language Russian
commented

The failed test caused by changes https://symfony.com/blog/hardening-symfony-recent-security-improvements#...

EDIT it was deprecated in 7.4 https://symfony.com/blog/new-in-symfony-7-4-request-class-improvements#d...

andypost’s picture

andypost
he/him
Primary language Russian
commented

Found existing issue #3007578-13: Discuss how and if new functionality should be added to the RequestSanitizer but there's already expected deprecation in the functional test so I asked ClaudeCode to summarize and the commit message with all details

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@andypost Symfony deprecated some methods in that header in v7 and then removed support on V8, I think instead we need to update our test to match what Symfony does and leave RequestSanitizer alone.

andypost’s picture

andypost
he/him
Primary language Russian
commented

That's what I did () but the problem is that page_cache is running before sanitizer and it check only for POST requests, that's why second test expectations changed (no HIT anymore)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Reverted the changes to RequestSanitizer, this code really only affects OPTIONS now as Symfony sanitizes GET, HEAD, CONNECT and TRACE itself by ignoring the header for these methods and treating them as POST.

As soon as @mglaman releases a new version of phpstan-drupal we should be good to go here.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It's not enough as I pointed in my commit - see how functional test is broken

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@andypost isn't the test invalid now though? The header is ignored by Symfony 8, so the override should just be treated as another POST?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Added an additional assertion that proves the Symfony sanitizer does what we expect with the request method, I think this is better.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: [PP-upstream] Update to Symfony 8 » Update to Symfony 8
Status: Postponed » Needs review

mglaman/phpstan-drupal has a Symfony 8 compatible release, so this is now green.

catch made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks great. There is some bc removal for validators, and there's a fair amount of test changes for the request sanitizer change, but mostly because Symfony has partially fixed what we were fixing just in a different way. I kicked off a rebase just in case but I think we should go ahead here to unblock realistic contrib testing against 12.x a bit more.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Rebased following #3569092: Update method signatures for addConstraint() and ConstraintManager::create() and remove HasNamedArguments use

quietone’s picture

quietone commented
Parent issue: » #3449808: [12.x] [meta] Requirements for tagging 12.0.0-beta1
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Rebased again following #3552827: Replace all expectDeprecation() calls with expectUserDeprecationMessage()

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I am not qualified to sign off on the request sanitization parts of this MR, since I'm completely unfamiliar with that. But the dependency changes, and the alterations to validation constraints, appear kosher to me.

godotislate’s picture

godotislate
he/him
commented

One nit and one question about the QUERY method.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Needs work

Back to NW for MR comments.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Thanks for the review - added a return type and a test for QUERY.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

lgtm!

  • catch committed f450ee12 on main 
    task: #3570077 Update to Symfony 8

By: longwave
By: gábor hojtsy
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14528

dpi’s picture

dpi
Location Perth, Australia
commented

Tracking Drush and any CI Next-Major jobs broken by this @ https://github.com/drush-ops/drush/pull/6534

Status: Fixed » Closed (fixed)

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