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

[Meta] PHP 7.3 support #3697

Closed
4 of 7 tasks
julienfalque opened this issue Apr 14, 2018 · 60 comments
Closed
4 of 7 tasks

[Meta] PHP 7.3 support #3697

julienfalque opened this issue Apr 14, 2018 · 60 comments
Labels
kind/enhancement kind/meta Set of actions to be broken down into single issues/PR's

Comments

@julienfalque
Copy link
Member

julienfalque commented Apr 14, 2018

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:

Progress tracker: #4184

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

Sources:

@ntzm
Copy link
Contributor

ntzm commented Apr 16, 2018

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
Copy link
Member Author

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
Copy link
Member

keradus commented Apr 16, 2018

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

@SpacePossum
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

keradus commented Apr 19, 2018

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

@SpacePossum
Copy link
Contributor

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
Copy link
Contributor

ntzm commented Jun 8, 2018

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

@ntzm
Copy link
Contributor

ntzm commented Jun 9, 2018

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
Copy link
Contributor

gharlan commented Jun 10, 2018

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
Copy link
Contributor

ntzm commented Jun 10, 2018

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
Copy link
Member Author

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

@SpacePossum SpacePossum added the kind/meta Set of actions to be broken down into single issues/PR's label Jun 28, 2018
@ntzm
Copy link
Contributor

ntzm commented Aug 2, 2018

hrtime function has also been added

(edit by SpacePossum: PR)

@keradus
Copy link
Member

keradus commented Aug 8, 2018

nightly job is green now

@Slamdunk
Copy link
Contributor

Sources:

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

Correct links are:

@DeepDiver1975
Copy link

DeepDiver1975 commented Sep 25, 2018

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
Copy link
Contributor

ntzm commented Sep 25, 2018

@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
Copy link

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
Copy link

Workaround for all other closed PHP 7.3 composer.json PRs: lmc-eu/steward#230
Thanks @OndraM 👍

@kelunik
Copy link
Contributor

kelunik commented Dec 25, 2018

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
Copy link

@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
Copy link

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 😉

@SpacePossum
Copy link
Contributor

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 added a commit that referenced this issue Dec 28, 2018
This PR was merged into the 2.14-dev branch.

Discussion
----------

Add HeredocIndentationFixer

#3697 / https://wiki.php.net/rfc/flexible_heredoc_nowdoc_syntaxes

Requires PHP 7.3

Input:

```php
<?php
    $a = [
        <<<'EOD'
abcdef
    ghi
jkl
EOD
    ];
```

is fixed to:

```php
<?php
    $a = [
        <<<'EOD'
            abcdef
                ghi
            jkl
            EOD
    ];
```

Commits
-------

61242ca Add HeredocIndentationFixer
@keradus
Copy link
Member

keradus commented Dec 28, 2018

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
Copy link
Member Author

@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
Copy link
Contributor

SpacePossum commented Dec 28, 2018

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; #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
Copy link

curtisbelt commented Dec 30, 2018

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 👍

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

@keradus
Copy link
Member

keradus commented Dec 31, 2018

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
Copy link
Member

keradus commented Dec 31, 2018

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 👆 paragraph ;)

@kelunik
Copy link
Contributor

kelunik commented Dec 31, 2018

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
Copy link
Member

keradus commented Dec 31, 2018

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
Copy link
Contributor

kelunik commented Dec 31, 2018

@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
Copy link
Contributor

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

kelunik added a commit to amphp/php-cs-fixer-config that referenced this issue Dec 31, 2018
We won't install php-cs-fixer via Composer anymore, see
PHP-CS-Fixer/PHP-CS-Fixer#3697 for context.
keradus added a commit that referenced this issue Jan 4, 2019
…xavier)

This PR was merged into the 2.12 branch.

Discussion
----------

[7.3] Test that "LITERAL instanceof X" is valid

Refs #3697

Commits
-------

5390f84 [7.3] Test that "LITERAL instanceof X" is valid
keradus added a commit that referenced this issue Jan 4, 2019
This PR was merged into the 2.14-dev branch.

Discussion
----------

Add official support for PHP 7.3

Closes #3697
Closes #4184

Commits
-------

e79a640 Add official support for PHP 7.3
@keradus keradus closed this as completed Jan 4, 2019
@localheinz
Copy link
Member

@keradus

🚀

@melroy89
Copy link

melroy89 commented Jan 5, 2019

@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
Copy link
Contributor

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)

@SpacePossum SpacePossum unpinned this issue Jan 5, 2019
SpacePossum added a commit that referenced this issue Apr 5, 2020
… level (gharlan)

This PR was merged into the 2.17-dev branch.

Discussion
----------

HeredocIndentationFixer - config option for indentation level

We discussed the best indentation level for the HeredocIndentationFixer [here](#3697 (comment)) and [here](#3915).

But now since I'm actually using it, I changed my mind.
I would like to write code like this:

```php
    public function foo()
    {
        return $this->createQuery(<<<'DQL'
            SELECT foo
            FROM App\Entity\Foo foo
            WHERE ...
        DQL)->getResult();
    }
```

and NOT:

```php
    public function foo()
    {
        return $this->createQuery(<<<'DQL'
            SELECT foo
            FROM App\Entity\Foo foo
            WHERE ...
            DQL)->getResult();
    }
```

So I suggest to add a config option `indentation` with possible values `start_plus_one` (default; same as current behavior) and `same_as_start` (new behavior). (Ideas for better names?)

The fixer will remain non-risky, so it does not change the actual string result.
This input:

```php
    $dql = <<<'DQL'
SELECT foo
FROM App\Entity\Foo foo
DQL;
```

will be changed with the new behavior (`same_as_start`) to:

```php
    $dql = <<<'DQL'
    SELECT foo
    FROM App\Entity\Foo foo
    DQL;
```

Adding one more indentation level inside string content is not part of this fixer (as it would be risky).

Commits
-------

b03554d HeredocIndentationFixer - config option for indentation level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement kind/meta Set of actions to be broken down into single issues/PR's
Projects
None yet
Development

No branches or pull requests