Php-cs-fixer: [Meta] PHP 7.3 support

[julienfalque]

This issue lists PHP 7.3 changes that are relevant to this project and tracks related issues and PR.

Syntax changes that require new tests to make sure the tool is compatible with PHP 7.3:

• [x] [Flexible Heredoc and Nowdoc Syntaxes](https://wiki.php.net/rfc/flexible_heredoc_nowdoc_syntaxes) (#3915)
• [x] [list() Reference Assignment](https://wiki.php.net/rfc/list_reference_assignment)
• [x] instanceof now allows literals as the first operand, in which case the result is always false.
• [x] [Allow a trailing comma in function calls](https://wiki.php.net/rfc/trailing-comma-function-calls)

Progress tracker: https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/4184

Relevant changes that could lead to new features (not required to make the tool compatible with PHP 7.3):

• [ ] hrtime function has been added
• [ ] is_countable function has been added => #4236
• [ ] add trailing comma to multiline function calls => #4237

Sources:

• https://github.com/php/php-src/blob/PHP-7.3/NEWS
• https://github.com/php/php-src/blob/PHP-7.3/UPGRADING

[ntzm]

Would be nice to get a fixer to pass the new JSON_THROW_ON_ERROR flag to json_ methods when 7.3 is released

[julienfalque]

Could be nice indeed, but that should be a separate feature request. This issue mostly tracks changes making _existing_ code compatible with PHP 7.3 :)

[keradus]

and then, clean up manual handling of error, huh? gonna be tricky ;(

[SpacePossum]

• is_countable function
• Allow a trailing comma in function calls

The last one will be a bit of work here I think.
It also seems to be missing from the releases note, maybe that gets corrected before the release though.

[julienfalque]

Thanks @SpacePossum, I edited the list. Note that I think we don't need to care about is_countable() to be compatible with PHP 7.3. Having rules for it would be a separate feature request, and one was just submitted by @ntzm: #3711 ;)

[keradus]

whenever we care about native functions, we need to take care of is_countable now as well.

[SpacePossum]

I wonder if we could help people mediate issues like these https://externals.io/message/102147 when going from PHP7.2 to 7.3, tricky part is to identify when the fix needs to be applied and not re-apply it on a second run on PHP7.3.....

[ntzm]

net_get_interfaces is added in 7.3 as well: https://github.com/php/php-src/blob/master/NEWS

[ntzm]

Alpha 1 is out, spotted a bug in token_get_all with the TOKEN_PARSE flag that was breaking a lot of php-cs-fixer, reported here

[gharlan]

Flexible Heredoc and Nowdoc Syntaxes

I want to create a fixer for heredoc/nowdoc indentation.

Input:

function foo() {
    return [
        'foo',
        <<<'EOF'
abcdef
    ghijkl
mnopqr
EOF
        ,
        'bar',
        'baz',
    ];
}

Which indentation would you prefer?

v1:

function foo() {
    return [
        'foo',
        <<<'EOF'
        abcdef
            ghijkl
        mnopqr
        EOF
        ,
        'bar',
        'baz',
    ];
}

v2:

function foo() {
    return [
        'foo',
        <<<'EOF'
            abcdef
                ghijkl
            mnopqr
            EOF
        ,
        'bar',
        'baz',
    ];
}

And should the same fixer replace the comma (EOF,), or should it be done by another (new) fixer, or by an existing fixer about spaces around ,?

[ntzm]

I would personally prefer V1, and I think the comma change should be done on fixer no_whitespace_before_comma_in_array on 7.3+ of course

[julienfalque]

I'd prefer v2. Indentation should probably be configurable. Handling of the comma should be done by no_whitespace_before_comma_in_array indeed.

[ntzm]

hrtime function has also been added

(edit by SpacePossum: PR)

[keradus]

nightly job is green now

[Slamdunk]

Sources:

https://github.com/php/php-src/blob/master/NEWS
https://github.com/php/php-src/blob/master/UPGRADING

Correct links are:

• https://github.com/php/php-src/blob/PHP-7.3/NEWS
• https://github.com/php/php-src/blob/PHP-7.3/UPGRADING

[DeepDiver1975]

Facing this issue: already addressed or reported?

```
Deprecated: strrchr(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior in /home/travis/build/sabre-io/dav/vendor/friendsofphp/php-cs-fixer/Symfony/CS/Utils.php on line 115

[ntzm]

@DeepDiver1975 You are using PHP-CS-Fixer 1.x, please update to 2.x by reading the guide here: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.12/UPGRADE.md#upgrade-guide-from-1x-to-20

[TomasVotruba]

Just FIY, blocking PHP 7.3 makes fail builds that want to use it any way:
https://travis-ci.org/phpmyadmin/phpmyadmin/jobs/460537777

[TomasVotruba]

Workaround for all other closed PHP 7.3 composer.json PRs: https://github.com/lmc-eu/steward/pull/230
Thanks @OndraM :+1:

[keradus]

well, we gave you very same hint when you asked month ago ;)
https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/4051#issuecomment-430970261

[TomasVotruba]

I didn't follow the conversation after many unhelpful comments :)

[winkbrace]

Note that you can force execution on php 7.3 by setting PHP_CS_FIXER_IGNORE_ENV env var.

[jakzal]

Are all the items from the above list actually necessary to implement in order to get php-cs-fixer working on PHP 7.3? The build is passing after all. Suggestion to ignore platform requirements also hints that php-cs-fixer actually works on PHP 7.3.

Forgive me if I've missed anything, but why not add 7.3 to supported versions?

[theofidry]

@JLHwung why the thumb down?

I also don't understand why PHP-CS-Fixer should block the 7.3 support release because there is new fixers that could be added for 7.3

[julienfalque]

@jakzal @theofidry Please see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3323#issuecomment-355007563

[theofidry]

That makes sense, but AFAICS it's still about making sure it will not break any PHP 7.3 code. The following for example should not be blockers:

• hrtime function has been added
• is_countable function shall be handled as all other native functions

[julienfalque]

Indeed, this issue is about PHP 7.3 support, that is:

1. making sure all existing fixers are compatible with new PHP 7.3 syntaxes;
2. list new features we can implement regarding those new syntaxes.

So yes, only 1 is required to allow installing the tool on PHP 7.3 :)

[dmvdbrugge]

@julienfalque maybe update the initial post to show the separation? (Just for clarification.)

[andrerom]

Or split in two issues to prioritize to unblock (clean) php 7.3 usage?

[SpacePossum]

From my last checks a few days ago there are a bunch of nice-to-haves for better 7.3 support.
Allow a trailing comma in function calls is this one that is a must-be-checked-and-fixed.
After that it is a matter of minors that might still block 7.3 support but nothing major AFAIK.

[JLHwung]

@theofidry the thumbdown is for:

• winkbrace has gave us a workaround here, I see no reason it is actually "blocking 7.3 support".
• Test is not breaking in PHP 7.3 does not means that the support for PHP 7.3 is ready. As you can see there is some new feature of PHP 7.3 that cs-fixer does not support yet. I am glad the the developer are very cautious about the platform requirement.

As an OSS the php-cs-fixer owns us nothing to any prompt support for any new version of PHP. Don't pressure on maintainers and if you can and you would, raise a PR to contribute the project.

[theofidry]

@JLHwung thanks for clarifying, as you can see our beef was more against new fixers/rules which are unnecessary for having a 7.3 support.

Don't pressure on maintainers and if you can and you would, raise a PR to contribute the project.

Sure, but it's better to be clear about what is needed first right :)

In any case, thanks for expanding your PoV, it is more helpful than just a thumb down 😄

[jakzal]

winkbrace has gave us a workaround here, I see no reason it is actually "blocking 7.3 support"

The workaround won't work for me unfortunately. As in my case php-cs-fixer is installed as a composer dependency and I don't want to ignore platform reps for other packages. As much as I'd love to install php-cs-fixer as a phar, it's not happening on this project.

So yeah, it is blocking my move to PHP 7.3, but please don't treat it as putting a pressure on maintainers. I asked a genuine question as I don't understand why a workaround which is equivalent to allowing 7.3 in composer.json is better than actually allowing php 7.3.

[julienfalque]

@jakzal Maybe an alternative solution could be that you install the tool with a dedicated composer.json in another directory (e.g. tools/php-cs-fixer). Another benefit from doing this is that PHP CS Fixer's dependencies won't interfere with your project's dependencies anymore.

[derrabus]

Suggestion to ignore platform requirements also hints that php-cs-fixer actually works on PHP 7.3.

It probably boils down to this: You can safely use CS Fixer on PHP 7.3 as long as the code that you're fixing is not using any language features of PHP 7.3 yet.

[enumag]

It probably boils down to this: You can safely use CS Fixer on PHP 7.3 as long as the code that you're fixing is not using any language features of PHP 7.3 yet.

Yeah that's exactly it. Also this issue comes out about CS Fixer every time a new PHP version is released. I think there are only two ways to solve it at the moment:

1. The current way of disallowing the installation unless --ignore-platform-reqs is used.
2. Allow the installation but show an error when running on a version with not fully supported syntax stating that CS Fixer will only work if you don't use those new features and maybe urging the user to send a PR.

Both ways have pros and cons and semver-wise the first one is definitely more correct. The second one would be more practical for any OSS library creators (like myself) who are using CS fixer in their CI - we usually only want to add PHP 7.3 to CI to check that nothing fails but don't immmediately use the new syntax.

[localheinz]

The easiest workarounds are

• remove friendsofphp/php-cs-fixer on builds running on PHP 7.3
• require friendsofphp/php-cs-fixer on a build not running PHP 7.3

There is only a need to run code style checks and fixes on a single build, not all of them.

[andrerom]

@localheinz This is not just about builds, it's also about being able to to install applications in the first place _(mainly talking about composer create-project as way to instal apps, but also git clone + composer install, in cases you need dev packages for dev usage, where we really want to error out if requirements are not meet)_

Allow the installation but show an error when running on a version with not fully supported syntax stating that CS Fixer will only work if you don't use those new features and maybe urging the user to send a PR.

This is definitely the right way to go with this.

[winkbrace]

I agree with @andrerom that a warning would be better for most cases. We have php cs fixer in our grumphp settings as a pre-commit hook, so I'm not really able to remove it from my project (and too stubborn to downgrade to php 7.2) ^_^
But as long as I commit from the command line where I've set the export PHP_CS_FIXER_IGNORE_ENV=1 in my .bashrc and run composer with --ignore-platform-reqs I'm good.

[Nothing-Works]

@winkbrace can I set PHP_CS_FIXER_IGNORE_ENV=1 as default somewhere? so I can use it with vscode extension?

[winkbrace]

@Nothing-Works It's en environment variable that must be set. I'm not familiar with vscode, but in phpstorm I have to use php cs fixer as an external tool that I call with a prefix: PHP_CS_FIXER_IGNORE_ENV=1; /Users/winkbrace/.composer/vendor/bin/php-cs-fixer

[Zheaoli]

excuse me, is there any roadmap to support PHP 7.3?

[theofidry]

@Zheaoli the description of the issue

[kelunik]

It probably boils down to this: You can safely use CS Fixer on PHP 7.3 as long as the code that you're fixing is not using any language features of PHP 7.3 yet.

This. Requirements should be runtime requirements and not state the code that can be fixed. If you write a fixer for Ruby code, you wouldn't write Ruby into the requirements, no?

[andrerom]

@teohhanhui No judgment given, but care to expand on why you vote down several comments which seems to make sense from package consumer perspective?

[devdrops]

Hi!

I've just stumbled into this scenario (installing friendsofphp/php-cs-fixer on PHP 7.3) and noticed this issue.

Based on the list in the first message, is the list up-to-date? If so, I'd like to help by checking some of these items :wink:

[SpacePossum]

Hi!

I would say that my last comment is the most up-to-date.
Basically it boils down to:

• finding fixers/code that do not handle the new Allow a trailing comma in function calls syntax well
• fix the issues

everything else from PHP 7.3 are nice-to-haves we can add later on :)

[keradus]

way too late probably, but just to clarify the policy.

Also this issue comes out about CS Fixer every time a new PHP version is released.

I agree with @andrerom that a warning would be better for most cases.

No. Warning is not enough. PHP CS Fixer must not break the code unless it was told otherwise. So, running the Fixer when there are high chances to break the codebase of user with just informing him "btw, I don't know what I'm doing, maybe I broke sth" is not acceptable. Especially for that, for two behaviors we have that could change/break the codebase, we have 2 special flags - allow risky and ignore env - that user of tool must explicitly allow the Tool to run.

Saying that, stability of the Tool is top prio. When user is not allowing the risky behaviour, we promise to not break the codebase and we do our best on that.
(btw, thanks @JLHwung for nice words)

I do know that topic of 7.3 is burning - but please, keep in mind that this tool is driven by community and made in free time of ppl. For that, resources are simply limited, and all the help is appreciated (not only raising the PRs, but also reviewing them, looking for missing edge cases, doing regression tests, ...).
Thankfully, there was some movement recently about adding support for new syntax, thus topic is not frozen.

[julienfalque]

@SpacePossum I see you marked the following tasks as completed:

• list() Reference Assignment
• instanceof now allows literals as the first operand, in which case the result is always false

I don't remember seeing related PRs for those topics, are you sure they are covered enough by tests?

[SpacePossum]

We already supported list reference assignment as I thought it was part of PHP7.2... ^_^
Some other edge I found when verifying this can be found here; https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/4134 together with additional tests. Please let me know if I missed any cases.

The second one for instanceof I could not find any issues with the current fixers. But indeed no tests for it. I'll uncheck the item.

[curtisbelt]

I use VSCode extension junstyle.php-cs-fixer, and was able to get up and running again by setting export PHP_CS_FIXER_IGNORE_ENV=1 in my equivalent of a .bash_profile, and then restarting VSCode.

Maybe this will help others who are patiently waiting for the next release but are OK with the risk and unsupported features :+1:

P.S. Don't forget to remove the flag from your .bash_profile once 7.3 is supported.

[keradus]

Once more, just to stress that.
While it is possible to bypass all guards and run PHP CS Fixer anyway, fixing code written in 7.3 may lead to crash, eg
PHP Parse error: syntax error, unexpected ',' after fixing. Gladly, PHP CS Fixer by default check if file is valid after the process, and if not - it doesn't save it.

[keradus]

I have created an integration test for 7.3: #4184
it shows how far we are to let our Fixers understand code written in 7.3 - after all the fixes we already made, there are almost 20 Fixers that still needs adjustment.

If anyone is willing to help, please visit #4184 and simply pick one of listed fixers, example code that is failing for given Fixer is provided in that PR as well.

If anyone is willing to ask why there is no support of PHP 7.3 and s/he wants to have it, please follow :point_up_2: paragraph ;)

[kelunik]

I don't care about PHP 7.3 syntax support (for now, as the current target is lower PHP versions), just about being able to install php-cs-fixer via Composer without the need for --ignore-platform-reqs. I'm fine with the runtime environment flag, but installation currently isn't possible without ignoring _all_ platform requirements. If I ignore all, I also won't get extension requirements. I think requiring the runtime flag is enough, there's no need for the additional installation prevention via "require" of a lower PHP version.

[keradus]

So, you are basically saying that tool shall check env only on runtime. If you say that, why to apply it only for PHP CS Fixer, and not everything you are installing?
Your proposal violates the concept of Composer doing the checks in first place - for me, personally, this is wrong. If one cannot use the tool, he shall not be able to install it.
But don't worry, you can install the tool with sth else than Composer, which would not do the checking. Eg with PHIVE (https://github.com/FriendsOfPHP/PHP-CS-Fixer#locally-phive)

[kelunik]

@keradus You're entirely right, IMO all packages should only have a lower bound for PHP versions. That's also why we use only lower bounds for our packages, no upper bounds. PHP is pretty stable and unlike Composer dependencies, it's not installed as a dependency by Composer, but already installed on the system.

Besides that, the issue seems to be only PHP 7.3 _syntax_ that's not yet fully supported. Processing of PHP 7.2 and lower syntax seems to work fine, even on PHP 7.3, no?

[SpacePossum]

There are a lot different ideas that could be considered, like setting upper bound PHP support on fixer/rule level, etc. In the end however, all this takes up time that could be used spend on fixing the issues themselves.

Feel free to raise new concerns about what doesn't work when fixing PHP 7.3 code, but lets end the discussion about how to bypass the safety-mechanics preventing installing PHP-CS-Fixer on a PHP7.3 setup with composer (instructions are within the comments above), as no new arguments have been made for a while and no new insights have come from this to change the current ways.

Last days a lot of progress has been made towards PHP 7.3 support and I would really like to not loose momentum and focus because of the discussion here. We try to resolve the issues and prepare for a release with PHP 7.3 support so please help out with that :)

[localheinz]

@keradus

🚀

[danger89]

@SpacePossum I agree, face-it, you can better have small iterations and have progress, instead of standing still too long. Let's have 7.3 support!

[SpacePossum]

Thanks to all who helped out getting support for PHP7.3 done over the last week or so!
@kubawerlos , @keradus , @guilliamxavier @gharlan and others and reviewers/reporters!
Very much appreciated : D
Enjoy the new releases!

(release notes/change log will be available soon)