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

Updated PHP Parser from 1.4 to 3.1 #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Harumaro
Copy link

@Harumaro Harumaro commented Oct 4, 2017

  • Migrated from PHP Parser 1.4 to PHP Parser 2.1

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

Tried non breaking approach using PREFER_PHP5.
Everything works.

  • Migrated from PHP Parser 2.1 to PHP Parser 3.1

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

  • fix src/NodeVisitor/IndirectVariableOrMethodAccessVisitor.php

  • fix src/NodeVisitor/YieldExpressionVisitor.php

  • fix src/NodeVisitor/YieldInExpressionContextVisitor.php

  • fix src/NodeVisitor/GlobalVariableVariableVisitor.php

  • fix src/NodeVisitor/InvalidOctalLiteralVisitor.php

  • fix src/NodeVisitor/NewAssignmentByReferenceVisitor.php

- Migrated from PHP Parser 1.4 to PHP Parser 2.1

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

Tried non breaking approach using PREFER_PHP5.
Everything works.

- Migrated from PHP Parser 2.1 to PHP Parser 3.1

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

Dropped the support for older versions of PHP.
Removed older PHP versions from CI
Reformatted code to CS
@Harumaro
Copy link
Author

Harumaro commented Oct 4, 2017

References #79 #99 #106 #110

@sstalle
Copy link
Owner

sstalle commented Oct 7, 2017

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

sstalle commented Oct 7, 2017

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants