VisibilityManipulator adjustment#1362
Conversation
|
@TomasVotruba please review. My changes don't seem to be the cause for the rector-nette step to fail. :) |
|
|
||
| if ($isStatic) { | ||
| $this->makeStatic($node); | ||
| $this->addVisibilityFlag($node, Visibility::STATIC); |
There was a problem hiding this comment.
Why this change? Now we have to maintain the same code on 2 places, which might lead to change of one without another.
There was a problem hiding this comment.
I just tried to be as close to the previous code as possible without breaking other functionality.
We need to apparently strip the static tag so the visibility is correctly adjusted. (If I would not strip it other tests are failing)
What do you exactly mean by "maintain the same coe on 2 places"? Maybe I can find a way to improve this.
There was a problem hiding this comment.
The code seems 100 % same as in makeStatic().
What do you exactly mean by "maintain the same coe on 2 places"? Maybe I can find a way to improve this.
If there is bug in makeStatic(), now we have to fix in 2 places.
How does this fix the reported bug?
There was a problem hiding this comment.
I honestly copied from makeStatic and forgot that I did that when trying to look for the solution 😅 Adjusted the calls to use the makeStatic and makeNonStatic functions instead.
Previously we were removing the visibility modifier (which I don't quite understand why) because it somehow fixed rectorphp/rector#4941 in rectorphp/rector#4951 but it looks like this change is actually the only one required to fix the problem listed in rectorphp/rector#6829.
I didn't dig deep enough to find out why this is the case though.
|
Thank you for the test and fix 👏 amazing work. I'll check why the Nette package fails, last PRs today it was working fine. |
|
Could you rebase on current |
Bl00D4NGEL
force-pushed
the
visibility-manipulator-static-fix
branch
from
8d15fe6 to
962eb3f
Compare
|
Thank you, this looks solid 🤗 |
3 participants
Previously the VisibilityManipulator would strip public, protected and/or private visibility. This caused the ChangeMethodVisibilityRector to cause problems described in rectorphp/rector#6829
refs: rectorphp/rector#6829
closes: rectorphp/rector#6829