Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Bump minimum version of PHP to 8.1#45377

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.1fromnicolas-grekas:php81
Feb 25, 2022
Merged

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedFeb 9, 2022
edited
Loading

QA
Branch?6.1
Bug fix?no
New feature?no
Deprecations?no
TicketsFix#44364
LicenseMIT
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 athttps://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?

mykiwi, mhujer, petk, 94noni, jgauthi, Nielsvanpach, Guikingone, sgrodzicki, okwinza, javiereguiluz, and 137 more reacted with thumbs up emojijackmcdade, robinmalburn, shadoWalker89, and arthurtavaresdev reacted with thumbs down emojiChemaclass, ging-dev, azjezz, KennedyTedesco, LavaToaster, Warxcell, kevin-emo, TomasVotruba, and LucaRed reacted with hooray emojifbourigault, jdreesen, fancyweb, xorik, bronek89, majermi4, nikolaynizruhin, Koc, dadangnh, Chemaclass, and 16 more reacted with rocket emojiderrabus, Chemaclass, ging-dev, emmanuelballery, and BafS reacted with eyes emoji
@javiereguiluz
Copy link
Member

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. 👍

OskarStark, 94noni, alexchuin, ogizanagi, Chemaclass, rvanlaak, houssemz, ging-dev, mortenebak, ansien, and 13 more reacted with thumbs up emoji

@94noni
Copy link
Contributor

According to:

~50% at the time of writing, is already running php v8.1 :)

@nicolas-grekasnicolas-grekasforce-pushed thephp81 branch 2 times, most recently from3ac3d9b tod86d027CompareFebruary 9, 2022 15:14
@GrahamCampbell
Copy link
Contributor

GrahamCampbell commentedFeb 9, 2022
edited
Loading

Also, looking athttps://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.

derrabus, shadoWalker89, eduardoalonsoalbella, and williamdes reacted with thumbs up emojiAdamKyle reacted with hooray emoji

@nicolas-grekasnicolas-grekasforce-pushed thephp81 branch 2 times, most recently from74e570a tof866031CompareFebruary 9, 2022 15:35
@jenkoian
Copy link
Contributor

Not sure if this will affect your decision or not butAWS 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).

mcareddu and akulmehta reacted with thumbs up emoji

@nicolas-grekasnicolas-grekasforce-pushed thephp81 branch 2 times, most recently frome8b4afb to705fbffCompareFebruary 9, 2022 16:12
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 9, 2022
edited
Loading

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 in9ce4531

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
MemberAuthor

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

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

@derrabus
Copy link
Member

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.

rvanlaak, kevinpapst, artyuum, shadoWalker89, jesusantguerrero, renejpeter, shalvah, cyberlightdev, RobinBastiaan, coffeeneer, and 5 more reacted with thumbs up emoji

@chr-hertel
Copy link
Contributor

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.

derrabus, dereuromark, and 7ochem reacted with thumbs up emoji

@wouterj
Copy link
Member

wouterj commentedFeb 9, 2022
edited
Loading

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.

linaori and 7ochem reacted with thumbs up emoji

@Nyholm
Copy link
Member

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.

OskarStark, chalasr, mynameisbogdan, yceruto, tucksaun, taylorotwell, Kocal, kevinpapst, renejpeter, RobinBastiaan, and 4 more reacted with thumbs up emoji

@catch56
Copy link

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, xjm, 7ochem, and joelpittet reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 9, 2022
edited
Loading

It's important to acknowledge the reason why I'm proposing this. Wedo 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.

andypost, Nyholm, derrabus, Ahummeling, mglaman, stloyd, wouterj, BoShurik, rvanlaak, mortenebak, and 6 more reacted with heart emoji

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 9, 2022
edited
Loading

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 is 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.

xorik, bastnic, Ahummeling, derrabus, rvanlaak, 7ochem, and LucaRed reacted with thumbs up emoji

@xjm
Copy link

xjm commentedFeb 20, 2022
edited
Loading

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. :)

williamdes reacted with eyes emoji

@mxr576
Copy link

mxr576 commentedFeb 21, 2022
edited
Loading

@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.)

OskarStark reacted with thumbs up emojirvanlaak reacted with heart emoji

@nicolas-grekas
Copy link
MemberAuthor

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

LucaRed, michaelKaefer, OskarStark, dadangnh, and xorik reacted with thumbs up emojiLucaRed and joelpittet reacted with hooray emoji

@nicolas-grekas
Copy link
MemberAuthor

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
Member

Thank you@nicolas-grekas.

nicolas-grekas, azjezz, derrabus, LucaRed, xbbshampoo, rvanlaak, and ging-dev reacted with hooray emojinicolas-grekas, azjezz, LucaRed, rvanlaak, dadangnh, and ging-dev reacted with heart emojinicolas-grekas, azjezz, derrabus, xbbshampoo, rvanlaak, ging-dev, and alexislefebvre reacted with rocket emoji

@fabpotfabpot merged commit08fa74a intosymfony:6.1Feb 25, 2022
@fabpotfabpot deleted the php81 branchFebruary 25, 2022 11:17
@fabpotfabpot mentioned this pull requestApr 15, 2022
@hacfihacfi mentioned this pull requestMay 4, 2022
fabpot added a commit that referenced this pull requestAug 14, 2022
This PR was merged into the 6.2 branch.Discussion----------[Messenger] Add HandlerArgumentsStamp| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |Fix#31075| License       | MIT| Doc PR        |symfony/symfony-docs#17174As discussed in#31075 sometimes it's desirable for the messenger handler to receive additional argument than just the message itself. I understand the voiced concerns about passing the entire envelope but instead of that we could use the approach from this PR which doesn't add any additional arguments by default but is actually even more powerful since it gives the user full control what should be sent to the handler if desired.This is just a prototype of course. With#45377 in mind I used a readonly property from PHP 8.1 but of course such details can be easily adjusted.Let me know if such feature is wanted in Symfony. If yes then I'll add tests and a doc PR.Commits-------d081267 Add HandlerArgumentsStamp
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@williamdeswilliamdeswilliamdes left review comments

@fabpotfabpotfabpot approved these changes

@derrabusderrabusderrabus approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@jderussejderusseAwaiting requested review from jderussejderusse is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixxlyrixx is a code owner

@OskarStarkOskarStarkAwaiting requested review from OskarStarkOskarStark is a code owner

@wouterjwouterjAwaiting requested review from wouterjwouterj is a code owner

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

Typehinted properties on missing class with preloading
27 participants
@nicolas-grekas@javiereguiluz@94noni@GrahamCampbell@jenkoian@derrabus@chr-hertel@wouterj@Nyholm@catch56@xjm@fbourigault@jderusse@azjezz@Seb33300@rvanlaak@JDDoesDev@GromNaN@xorik@OskarStark@beberlei@DarkGhostHunter@robinmalburn@mxr576@fabpot@williamdes@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp