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

Bump minimum version of PHP to 8.1 #45377

Merged
merged 1 commit into from Feb 25, 2022
Merged

Bump minimum version of PHP to 8.1 #45377

merged 1 commit into from Feb 25, 2022

Conversation

Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 9, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #44364
License MIT
Doc PR -

While reviewing #44380, I wondered: why should we maintain such a complex piece of logic while only PHP 8.0 is affected?
So here we are: what about bumping Symfony 6.1 to PHP 8.1 minimum?
That would free ourselves from maintaining this workaround.

The good news is that Ubuntu 22.04LTS will ship PHP 8.1, so that it's going to be easy to have it widely installed.

Also, looking at https://packagist.org/packages/symfony/framework-bundle/php-stats#6.0, early adopters of Symfony 6 are already using PHP 8.1 en masse, and it's growing fast.

Let's do it?

@javiereguiluz
Copy link

@javiereguiluz javiereguiluz commented Feb 9, 2022

In the past, the most common argument against bumping the PHP version was that some major Linux versions were using an older version. This time, as you said, at least Ubuntu LTS is aligning perfectly with our needs, so hopefully other major Linux distributions do the same. 👍

@94noni
Copy link

@94noni 94noni commented Feb 9, 2022

@GrahamCampbell
Copy link

@GrahamCampbell GrahamCampbell commented Feb 9, 2022

Also, looking at https://packagist.org/packages/symfony/framework-bundle/php-stats#6.0, early adopters of Symfony 6 are already using PHP 8.1 en masse, and it's growing fast.

This is only because Laravel 9 only was released only yesterday. Lots of those users will be on PHP 8.0.

@jenkoian
Copy link

@jenkoian jenkoian commented Feb 9, 2022

Not sure if this will affect your decision or not but AWS EB doesn't yet support PHP 8.1. I think most of the other PAAS providers offer PHP 8.1 though (except Google AppEngine which afaict doesn't even offer 8.0 support yet).

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 9, 2022

ldap_rename(): Passing null to parameter #4 ($new_parent) of type string is deprecated

This shoud be fixed on lower branches. PR welcome anyone, that's a good first contrib :) Fixed in 9ce4531

TypeError: Memcached::setMulti(): Argument #2 ($expiration) must be of type int, int given

🤦

Anyway, this PR is ready to go from a technical pov, remaining failures are false positives.

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 9, 2022

Not sure if this will affect your decision or not but AWS EB doesn't yet support PHP 8.1

Thanks for the info @jenkoian. That shouldn't be a blocker to me.

@derrabus
Copy link

@derrabus derrabus commented Feb 9, 2022

The Packagist statistics seem to be in your favor:

However, this is a major change in our release strategy: we haven't bumped the PHP language level in a minor release yet. If we take that road, I believe we should announce this a bit ahead of time. The fixed release cycles of PHP and Symfony should encourage development teams to schedule their framework and infrstructure upgrades ahead of time. Projects that chose to start with or upgrade to Symfony 6 might have done so trusting that they can remain on PHP 8.0 for a certain period of time. If we bump now, we basically force those projects to either reschedule infrastructure upgrades or to remain on Symfony 6.0 beyond its EOL.

If we would've discussed and announced that course of action before the 6.0 release, I would be all cool with it. But bumping to PHP 8.1 on such a short notice might make Symfony's release strategy appear erradic.

How would you feel about postponing this language level bump to the 6.2 release? That could give downstream projects a bit more time to adjust their development schedule accordingly.

@chr-hertel
Copy link

@chr-hertel chr-hertel commented Feb 9, 2022

For us that means that we need to delay our Symfony update to 6.0, since its maintenance is that limited and we'd be locked with an unmaintained version until we get 8.1 support done.
Get the motivation though, only sharing the impact for us.

@wouterj
Copy link

@wouterj wouterj commented Feb 9, 2022

Same for us. We usually skip the *.0 release of a new major, to have 6 months to fix the deprecations introduced in 5.4. So our infrastructure (PHP 8.0 at the moment) is not reflected in the Packagist statistics for 6.0.

This is a deviation from the standard release schedule. We usually do not "push" the industry to upgrade their infrastructure, but instead consider our minimum PHP version when the current one has reached eol (or is about to), giving the largest amount of time for everyone to upgrade a single thing at the time.

I think it comes down to weighing this deviation with postponing typed properties for another 6 months.

@Nyholm
Copy link

@Nyholm Nyholm commented Feb 9, 2022

I do love to remove code and I also appreciate using the latest version of PHP.

This goes against what Symfony has done in the past (for better or for worse). I would suggest to be boring and keep 8.0 compatibility. At least for SF6.1. Im more open to drop PHP 8.0 it in SF6.2 if we still think we have a strong case for it.

@catch56
Copy link

@catch56 catch56 commented Feb 9, 2022

Just briefly from Drupal's standpoint:

We're planning to release Drupal 10 on Symfony 6 this year. We've set our minimum requirement at PHP 8.0.2 because that's the current minimum of Symfony 6.0, however we're already recommending PHP 8.1.

We have active discussions happening about requiring PHP 8.1 or not (https://www.drupal.org/project/drupal/issues/3223435 / https://www.drupal.org/project/drupal/issues/3173787). And we've notified people that we'll announce an 8.1 bump at least five months in advance: https://www.drupal.org/about/core/blog/drupal-10-php-requirements-announcements

There are several reasons we'd like to require PHP 8.1, but we're also balancing it against distro/hosting availability (which is somewhere between promising and mixed). Obviously this issue was just opened in the past 24 hours so that's new information for us that hasn't fed into the current discussions yet.

If Symfony definitely goes ahead with this, it would be good to know as soon as possible. Based on the state of the discussions we're having, I don't think it's likely we'd try to stop you, but we also haven't definitely decided to require PHP 8.1 ourselves independently yet.

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 9, 2022

It's important to acknowledge the reason why I'm proposing this. We do have a strong case here, and it's not going to go away. PHP 8.0+preloading+typed props is broken, see linked issue.

I'm personally fine postponing to 6.2 if that's useful for the ecosystem. We'll need to reverse once more the types added to private properties before tagging 6.1.

On the other hand, we are months ahead the release of 6.1. We're not rushing anything here.

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 9, 2022

Talking with others from the core-team on Slack, it looks like we have two ways forward:

  • postpone this to 6.2 and revert typed properties from 6.1. There's an issue with this approach: ppl on PHP8.0+SF6.1 will remain with an unmaintained version of SF once 6.1 is EOL. Considering security issues, that's not great. Hoping that nobody will end up in this situation in a wild dream I fear. To fix this trap, we might need to extend the security maintenance of 6.2 by 8 months to overlap with 6.3.
  • which leads to this alternative: do this in 6.1, and extend the security maintenance of 6.0 to the EOL of 6.1.

My preference is for the latter: it allows sending the message sooner and provides a smooth way forward while saving maintainers' time.

@xjm
Copy link

@xjm xjm commented Feb 20, 2022

So this was resolved/decided on the Drupal side sooner than here.

@mxr576 , Drupal's decision is not unrelated to this PR. ;) Also, the Drupal community had already been agonizing over the decision of PHP 8.0 vs. 8.1 for 6 months, and we were fortunate enough to be in the development phase of a new major. I think we can all be patient for a couple weeks here. :)

@mxr576
Copy link

@mxr576 mxr576 commented Feb 21, 2022

@xjm my previous comment was just an FYI for ppl involved in this decision here. It was not positive or negative. I am sorry if someone can felt otherwise.

(Personally, I can fully support that decision , it is a future proof one.)

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 25, 2022

In case you missed it, we're going to merge this PR.
See https://symfony.com/blog/symfony-6-1-will-require-php-8-1

@nicolas-grekas
Copy link
Author

@nicolas-grekas nicolas-grekas commented Feb 25, 2022

PR is ready, failures are:

  • a false-positive on GHA
  • a legit failure on appveyor, but the fix should land on branch 4.4 (I'm going to have a look).

@fabpot
Copy link

@fabpot fabpot commented Feb 25, 2022

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 08fa74a into symfony:6.1 Feb 25, 2022
6 of 9 checks passed
@fabpot fabpot deleted the php81 branch Feb 25, 2022
@fabpot fabpot mentioned this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment