Skip to content
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.

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

Symfony 7 support #761

Merged
merged 22 commits into from
Dec 5, 2023
Merged

Symfony 7 support #761

merged 22 commits into from
Dec 5, 2023

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Sep 13, 2023

This is a playground to act upon symfony/symfony#51563

Symfony 7 is coming this November, and it will include a type overhaul that will impact the ecosystem, and us too.

In this PR, I tried forcibly upgrading to it, and 968b232 lists all the changes that we need to do; I'll comment them accordingly.

Unfortunately, at least the cache adapters require a mixed return type, which is a breaking change. We would need to either duplicate and selectively autoload those classes (as we already did with DBAL v2 vs v3) or do a breaking change, requiring PHP 8.0 as a minimum.

composer.json Outdated Show resolved Hide resolved
@stof
Copy link

stof commented Oct 30, 2023

what is the goal of this Force Symfony 6.4-dev commit ?

@ste93cry
Copy link
Collaborator

ste93cry commented Nov 4, 2023

I don't know, but I reverted the changes and added Symfony 7.* to the test matrix

@javiereguiluz
Copy link

About this concern from the original PR description:

"Unfortunately, at least the cache adapters require a mixed return type, which is a breaking change. We would need to either [...] or do a breaking change, requiring PHP 8.0 as a minimum."

FYI, active support for PHP 8.0 ended a year ago and security support will end in 19 days. Also, active support for PHP 8.1 ends in 18 days. See https://www.php.net/supported-versions.php

So, it's probably OK to change the PHP requirements. Thanks!

@cleptric
Copy link
Member

cleptric commented Nov 6, 2023

No, it's not ok to raise the PHP requirement 🙂

~17% of our users are still below PHP 8.0 https://packagist.org/packages/sentry/sentry-symfony/php-stats#4.

@janklan
Copy link

janklan commented Nov 16, 2023

~17% of our users are still below PHP 8.0

Isn't a new major version release a solution to introducing bc breaks? Why not release a 5.x and let the sloths be sloths?

@cleptric
Copy link
Member

~17% of our users are still below PHP 8.0

Isn't a new major version release a solution to introducing bc breaks? Why not release a 5.x and let the sloths be sloths?

Because this is not how Sentry operates, have a look at https://develop.sentry.dev/sdk/philosophy/#compatibility-is-king.

@janklan
Copy link

janklan commented Nov 16, 2023

Because this is not how Sentry operates, have a look at https://develop.sentry.dev/sdk/philosophy/#compatibility-is-king.

Yeah, I get that, but releasing a new major version with breaking changes doesn't prevent anyone from maintaining the 4.x branch with legacy code support.

Either way, I assume you guys will work something out to support Symfony 7. Compatibility is king, like you said.

composer.json Outdated Show resolved Hide resolved
@cleptric cleptric mentioned this pull request Nov 28, 2023
@tacman tacman mentioned this pull request Dec 3, 2023
@janklan
Copy link

janklan commented Dec 4, 2023

You're on fire, @ste93cry! 🤘

@Jean85
Copy link
Collaborator Author

Jean85 commented Dec 4, 2023

We're getting failures due to deprecations on a --prefer-lowest job, maybe we should silence deprecations in that case?

@ste93cry ste93cry marked this pull request as ready for review December 5, 2023 12:25
Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

we should silence deprecations in that case?

There's no need to, I reinstated the dependency on the symfony/dom-crawler package that I removed earlier, without realizing that it was required explicitly to avoid those deprecations. Changes look good now, CI is finally 🟢, so LGTM for me

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

Thanks @Jean85 & @ste93cry!

@cleptric cleptric merged commit 7fec186 into master Dec 5, 2023
33 checks passed
@cleptric cleptric deleted the symfony-7 branch December 5, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants