-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support the nikic/php-parser
to 5.x
#41
Conversation
Hi @owenvoke, Can you help review when you have time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kayw-geek, thanks for the PR! 👍🏻 From a code side, this looks ok for 5.x (love the changes to createForNewestSupportedVersion()
), however it seems to break a lot of tests when running with PHP-Parser 4.x locally. 🤔 Would you be able to update this PR so that both are supported? 👀
The failing tests in CI just appeart to be related to Windows line endings. 😐
8ee5eb5
to
2fc10fe
Compare
Hi @owenvoke, I have already simultaneously supported two major versions. In order to solve the problem with PHPStan, I had to add two stub files. I don't know if this is the best practice because I hadn't considered compatibility with multiple versions before. |
Hi @owenvoke, Can you help review again? |
Hi all, is there anything left we can do to wrap up this PR? It's the only package left that's blocking the upgrade to PHPUnit 11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed the notification for this. The tests seem to pass locally for both 4.x and 5.x, and they are passing in CI. So, LGTM! Thanks for your work on this, @kayw-geek
Thanks both! |
This PR addresses the issue #40, support the
nikic/php-parser
to 5.x.Closes #40