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 to nikic/PHP-Parser 4.x #410

Merged
merged 3 commits into from
May 8, 2018
Merged

Conversation

kukulich
Copy link
Collaborator

No description provided.

@kukulich kukulich force-pushed the php-parser4 branch 4 times, most recently from 29b9d86 to 8e1b674 Compare February 28, 2018 23:51
@kukulich
Copy link
Collaborator Author

kukulich commented Mar 1, 2018

Build is failing because PHPStan doesn't support PHP-Parser 4.

@ondrejmirtes PHP-Parser 4 has been released 11 hours ago. What have you been doing the whole night? 😄

@carusogabriel
Copy link
Contributor

@kukulich @ondrejmirtes Have already create a issue there to track this migration 😂

phpstan/phpstan#859

@ondrejmirtes
Copy link
Contributor

Looks like my plans for Thursday have been sorted out for me 😂

@TomasVotruba
Copy link
Contributor

@kukulich You could use the shim https://github.com/phpstan/phpstan-shim to focus on package upgrade at a time

@kukulich
Copy link
Collaborator Author

kukulich commented Mar 3, 2018

@TomasVotruba I tried, it doesnt work.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Mar 3, 2018

That's how Rector can run on php-parser 4 and use phpstan with php-parser 3 at the same time:

What exactly did you try and what happened?

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Mar 4, 2018 via email

@TomasVotruba
Copy link
Contributor

Then composer create-project *

@theofidry
Copy link
Contributor

You can also go with barmani/composer-bin-plugin. Until a shim is officially provided or a PHAR is used it's a good solution.

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Mar 4, 2018 via email

@theofidry
Copy link
Contributor

@kukulich what's the status of the PR?

@kukulich
Copy link
Collaborator Author

kukulich commented May 8, 2018

@theofidry The same. PHPStan doesn’t support PHP-Parser 4. The only solution is to disable phpstan check temporarily.

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented May 8, 2018 via email

@theofidry
Copy link
Contributor

Can't we disable PHPStan temporary then? It's a very nice to have tool but it doesn't feel like it should be a blocker neither and according to @ondrejmirtes it's gonna take another 1-2 months

@kukulich
Copy link
Collaborator Author

kukulich commented May 8, 2018

I'll prepare PR without PHPStan (already on my localhost) but I need to fix #422 first.

@kukulich
Copy link
Collaborator Author

kukulich commented May 8, 2018

Ready to review. I would like to have it in master so I can work again on #373 without future conflicts.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Huge improvement! Just need one last bit around caching the expression evaluator

# env: DEPENDENCIES=""
# before_script:
# - travis_retry composer require --dev --prefer-dist --prefer-stable phpstan/phpstan:^0.9.2
# script: vendor/bin/phpstan analyse -l 5 -c phpstan.neon src
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it is impossible to analyse this codebase due to autoloading reflection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly.

if ($node instanceof Node\Scalar\MagicConst\Class_) {
return $this->compileClassConstant($context);
}
$constExprEvaluator = new ConstExprEvaluator(function (Node\Expr $node) use ($context) {
Copy link
Member

Choose a reason for hiding this comment

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

Can the evaluator instance be preserved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, because it needs the fallback evaluator with the right context :( I tried to implemented it without the fallback evaluator and call other evaluations in catch but it doesn't work. The test CompileNodeToValueTest::testClassConstantResolutionWithAnotherClassConstant was failing because the test is combination of multiplication (supported by PHP-Parser itself) and class constants (supported by BR).

@Ocramius Ocramius self-assigned this May 8, 2018
@Ocramius Ocramius added this to the 3.0.0 milestone May 8, 2018
@Ocramius
Copy link
Member

Ocramius commented May 8, 2018

Thanks @kukulich!

@TomasVotruba
Copy link
Contributor

When can we expect a tag? It would help other packages to bump on php-parser 4.0

@Ocramius
Copy link
Member

I was going to create a new tag, but I'll most likely first merge in #423 first

@TomasVotruba
Copy link
Contributor

Oh, that's great 👍 Sure!

@kukulich kukulich deleted the php-parser4 branch May 22, 2018 21:02
@Ocramius Ocramius changed the title Updated to PHP-Parser 4 Updated to nikic/PHP-Parser 4.x May 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants