-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
[PHPStan] Set compatible with upcoming PHPStan 1.6.x with set NodeConnectingVisitor tags #2014
Conversation
All checks have passed 🎉 @TomasVotruba @ondrejmirtes it is ready for review. @ondrejmirtes until the compatible code for create new Visitor per specific data for parent, next, prev, do you think it is enough to make it compatible? |
Yes, I think so, feel free to try this out with phpstan/phpstan 1.6.x-dev dependency. |
On the other hand, this is for the Symfony DIC, right? This isn't for the DIC that's used by DIC. So I think this will still be broken for 1.6.x-dev. I think a possible fix instead is to require phpstan/phpstan 1.6.x-dev and do this: conditionalTags:
PhpParser\NodeVisitor\NodeConnectingVisitor:
phpstan.parser.richParserNodeVisitor: true |
@ondrejmirtes on rector scoped, phpstan is required in composer.json as ^1.5, not part of the scoped vendor: How to make it compatible for both phpstan 1.5 and 1.6.x-dev ? |
What if I:
conditionalTags:
PhpParser\NodeVisitor\NodeConnectingVisitor:
phpstan.parser.richParserNodeVisitor: true Then nothing will be blocked and Rector would be compatible even with 1.6 out of the box like this. Would you agree? But first, please make sure that the point number 3) fixes your test suite on PHPStan 1.6.x-dev. |
@ondrejmirtes that's seems will work I think /cc @TomasVotruba @ondrejmirtes I updated requirement to 1.6.x-dev and conditional tags config, and it seems phpunit got erorr now:
|
@ondrejmirtes by update to phpstan 1.6.x-dev in packages tests workflow, and temporary set 1.6.x-dev in target-repository build, it seems make CI green 🎉 Do you think the bleeding edge need to be enabled?
|
@ondrejmirtes remove the |
After I release 1.5.5 I'm gonna send a PR here and make sure it works. |
I'm gonna release 1.5.5 today, but it DOES NOT contain what I promised here, you're gonna have to wait a few more days for 1.5.6 :) Thanks for understanding. |
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, PHPStan 1.5.6 is out with detailed instructions: https://phpstan.org/blog/preprocessing-ast-for-custom-rules
This PR is OK - you can require ^1.5.6
instead of 1.6.x-dev - your code will work for both ^1.5.6
and 1.6.0
.
Please release new Rector version so that users are ready and Rector doesn't stop working for them once I release PHPStan 1.6.0.
This also probably applies to custom rules in the Symplify organization (https://github.com/symplify).
e7e11e8
to
6427c85
Compare
@ondrejmirtes I rebased and updated require to phpstan ^1.5.6 |
All checks have passed 🎉 @TomasVotruba I think it is ready. |
same with deprecated-packages/symplify#4029, I am merging it ;) |
@ondrejmirtes thank you for review 👍 |
Thank you both for great co-operation 👍 |
@ondrejmirtes @TomasVotruba I try to set compatible for upcoming PHPStan 1.6.x with bleeding edge feature. I tried to register
NodeConnectingVisitor
toconfig/phpstan/static-reflection.neon
and it cause error Multiple services definition:so I add to existing Service->set() definition in the
config/services.php
with define its tag there:ref rectorphp/rector#7088