sstalle / php7cc

PHP 7 Compatibility Checker
MIT License
1.52k stars 120 forks source link

Updated PHP Parser from 1.4 to 3.1 #139

Open Harumaro opened 6 years ago

Harumaro commented 6 years ago

Applied https://github.com/nikic/PHP-Parser/blob/v3.1.1/UPGRADE-2.0.md

Tried non breaking approach using PREFER_PHP5. Everything works.

Applied https://github.com/nikic/PHP-Parser/blob/v3.1.1/UPGRADE-3.0.md

Dropped the support for older versions of PHP, as PHP Parser v3.1 requires 5.5+. Removed older PHP versions from CI Reformatted code to CS

TODO

fix visitors broken by new AST

Harumaro commented 6 years ago

References #79 #99 #106 #110

sstalle commented 6 years ago

Thanks for taking the time to make and submit a PR on this long-standing issue.

I don't think that passing ParserFactory::PREFER_PHP5 or ParserFactory::PREFER_PHP7 constants to ParserFactory will work. For example, if we use ParserFactory::PREFER_PHP5 and the PHP 5 parser fails to parse the code which is valid in PHP 7, then PHP-Parser will fallback to the PHP 7 parser, which may produce a slightly different AST than the PHP 5 parser. So we'll have to maintain 2 sets of NodeVisitors to handle the differences between ASTs produced by different parsers.

Here is an example. Say we have a file with the following content:

<?php

$foo->$bar['baz'];

We run php7cc on it and get:

> Line 3: [Warning] Indirect variable, property or method access
    $foo->{$bar['baz']};

Checked 1 file in 0.002 second

But it we add a group use declaration with a trailing comma (which is valid in PHP 7.2, but is not valid for the PHP 5 parser):

<?php

use Foo\Bar\{
    Foo,
    Bar,
};

$foo->$bar['baz'];

And feed it to php7cc, we'll see no error:

Checked 1 file in 0.002 second

Considering the above, I think the only valid option is using ParserFactory::ONLY_PHP7 constant.

sstalle commented 6 years ago

Also, I'm not sure that we should require PHP-Parser ~3.1. There doesn't seem to be a lot of breaking changes between versions 2 and 3, so maybe ~2.0 || ~3.0 would suffice.

Harumaro commented 6 years ago

Thanks for your review.

I'm still working on it, to keep you updated I added a list of things breaking with the new AST in OP.