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

PHP8.1 support #6152

Merged
merged 1 commit into from Dec 10, 2021
Merged

PHP8.1 support #6152

merged 1 commit into from Dec 10, 2021

Conversation

SpacePossum
Copy link
Contributor

@kubawerlos do you maybe have time to run the tests with the top X PHP projects to see if there is regression we overlooked?

@SpacePossum SpacePossum added the topic/PHP8.1 Related to features available in PHP 8.1+ label Dec 7, 2021
php-cs-fixer Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 7, 2021

Coverage Status

Coverage increased (+0.2%) to 93.028% when pulling c86c22a on SpacePossum:master_81_in_bin into 3de1d1b on FriendsOfPHP:master.

Copy link

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Since php-cs-fixer declares ^8.0 as composer.json supported version:

https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/4ab8dc5a52e6bdf183865cc6093780ac6774c1fa/composer.json#L17

Then crashes at runtime (message changed in this patch):

PHP needs to be a minimum version of PHP 7.2.5 and maximum version of PHP 8.0.*.

I'd say that changing composer.json to restrict to "php": "^7.2.5 || ~8.0.0 || ~8.1.0" is a required addition to this patch: as it currently stands, the fact that the tool installs cleanly, then fails at a later point in time, is not really useful.

@keradus
Copy link
Member

keradus commented Dec 8, 2021

@Ocramius we had tons of ppl crying that they cannot update their project to use new PHP because PHP CS Fixer is not yet compatible. EVERY SINGLE TIME new PHP is out. Same time, not so many ppl are eager to help making PHP CS Fixer understanding new PHP syntax or be compatible with modified tokenizer.
We don't like either of those situations, yet we decided to go with one in the end - runtime check. Feel free to check previous discussions around this topic.

@Ocramius
Copy link

Ocramius commented Dec 8, 2021

I tell them to wait: having a tool that you install (then crashes at startup) is really no better, and it just gives false confidence (and introduces instability in the overall ecosystem).

The fact that dependency ranges don't match actual compatibility is mostly something that some communities have erroneously started advocating for, because solving the compatibility is less important than having users upgrade (and experience instability) 🤷

See also https://github.com/laminas/technical-steering-committee/blob/c7f6babfb1773d9cb9371208102cbb8b27980bd5/meetings/minutes/2021-08-02-TSC-Minutes.md#locked-dependencies-for-all-supported-php-versions for more context, previously discussed by the @laminas TSC.

In practice, for my own packages:

  • I pin to minor PHP: {"require": {"php": "~7.4.0 || ~8.0.0 || ~8.1.0"}} (no ~8.2.0 until I've tested it in RC!!!)
  • I tell users that they are free to shoot themselves in the foot with composer update --ignore-platform-req=php

@keradus
Copy link
Member

keradus commented Dec 8, 2021

Unfortunately, while we were following the approach you described, we faced many issues/complaints.

Anyway, feel free to open a new discussion, as this is not the blocker for this PR (as we have a current practice in place for a year) and I would prefer to not hijack this thread but focus on 8.1 here.

unfortunate note

I also spent quite a time discussing all pros and cons about having ~8.0 vs ^8 vs >=8. TBH, I rather prefer to spend time ensuring PHP8.x compatibility rather than spending time in a discussion that year ago started to be similar to tabs vs spaces.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Bilge
Copy link

Bilge commented Dec 9, 2021

Waiting on this. Guess we'll have to disable PHP-CS-Fixer for now.

@vbasky
Copy link

vbasky commented Dec 9, 2021

@Ocramius we had tons of ppl crying that they cannot update their project to use new PHP because PHP CS Fixer is not yet compatible. EVERY SINGLE TIME new PHP is out. Same time, not so many ppl are eager to help making PHP CS Fixer understanding new PHP syntax or be compatible with modified tokenizer.
We don't like either of those situations, yet we decided to go with one in the end - runtime check. Feel free to check previous discussions around this topic.

My project is also held up too, I'd love to help/contribute but unsure where to start on the repo

Copy link
Contributor

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

@SpacePossum finally the check finished, nothing related to PHP 8.1, only stuff that are already released (PR with fix in progress).

EDIT: this one.

@SpacePossum
Copy link
Contributor Author

Thanks so much @kubawerlos 👍

I think that is it, all feedback processed. If the tests are green I plan on merging. If anyone sees some related issue still open please comment :)

@keradus keradus added the RTM Ready To Merge label Dec 10, 2021
@keradus
Copy link
Member

keradus commented Dec 10, 2021

kudos for executing the regression, @kubawerlos !
@SpacePossum , we are good to go ;)

@SpacePossum SpacePossum merged commit 241b491 into PHP-CS-Fixer:master Dec 10, 2021
@SpacePossum SpacePossum deleted the master_81_in_bin branch December 10, 2021 10:07
@keradus keradus removed the RTM Ready To Merge label Dec 11, 2021
SpacePossum added a commit that referenced this pull request Dec 13, 2021
…e directives (kubawerlos)

This PR was squashed before being merged into the master branch (closes #6165).

Discussion
----------

DeclareEqualNormalizeFixer - fix for declare having multiple directives

One more edgy edge case found running [regression](#6152 (comment)).

Commits
-------

03f5eff DeclareEqualNormalizeFixer - fix for declare having multiple directives
@iwiznia
Copy link

iwiznia commented Dec 28, 2021

Sorry if I am not understanding this correctly, but it seems all issues to support php 8.1 are done but composer.json still does not allow 8.1.
What is missing to change that to allow php 8.1?

@Ocramius
Copy link

@iwiznia ^8.0 means >=8.0.0,<9.0.0

@iwiznia
Copy link

iwiznia commented Dec 28, 2021

Oh 🤦 my bad... I realize now that I was getting that error because platform was overriden in the composer.json file. Thanks @Ocramius!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/PHP8.1 Related to features available in PHP 8.1+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants