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

Add nikic/php-parser v5 support #11035

Open
wants to merge 12 commits into
base: 5.x
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

No description provided.

@VincentLanglet VincentLanglet deleted the nikic5 branch July 3, 2024 09:54
@VincentLanglet VincentLanglet changed the title Try to add nikic/php-parser v5 support Add nikic/php-parser v5 support Jul 25, 2024
composer.json Outdated Show resolved Hide resolved
Copy link
Contributor

@edsrzf edsrzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this over a code base that's still using php-parser 4. There's a problem with usePHPParserV4, which I've commented on, but with that fixed it seems to work.

Do we need to look at running tests against php-parser 4 as well?

src/Psalm/Internal/BCHelper.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Aug 10, 2024

I ran this over a code base that's still using php-parser 4. There's a problem with usePHPParserV4, which I've commented on, but with that fixed it seems to work.

Do we need to look at running tests against php-parser 4 as well?

Thanks. I added the CI "lowest" with php parser 4 and fixed the build.

@VincentLanglet
Copy link
Contributor Author

I think this i ready to be review @orklah @weirdan

Comment on lines +96 to +99
/**
* @var SplObjectStorage<PhpParser\Node\FunctionLike, null>
*/
private SplObjectStorage $closure_statements;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@chriskapp
Copy link

It would be really nice if we could merge this, currently it is not possible to install Psalm with PHPUnit 11 since it requires nikic/php-parser: ^5.0

@VincentLanglet
Copy link
Contributor Author

Friendly ping @weirdan if you have time to take a look (or can ping another maintainer ?).
This PR would be really helpful, by allowing to use both Psalm and PHPunit 11.

@weirdan
Copy link
Collaborator

weirdan commented Sep 8, 2024

Yeah, it's on my todo list. The thing is big, so it'll take me some time.

@weirdan
Copy link
Collaborator

weirdan commented Oct 12, 2024

Since we expose parts of PhpParser in our API this cannot go into 5.x line. So instead of merging this I'm going to fork 6.x from the current 5.x (not master) and then use this PR as a base for PhpParser 5.x upgrade. 6.x will be a transitional, hopefully short-lived, series with the same platform requirements as 5.x.

Thanks for all your work, this will make it easier.

@ghostwriter
Copy link

If the branch-alias in the master branch is preventing you from using the 6.x branch (causing it to pull the master branch instead of referencing the correct commit from the 6.x branch), add #{commit_sha} to specify the correct branch.

For example:

"vimeo/psalm": "6.x-dev#0f796f1"

Replace 0f796f1 with the latest commit SHA for accurate results.

@bensinclair
Copy link

Any update on this? I'm unable to install vimeo/psalm due to not support version 5 of nikic/php-parser.

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.

7 participants