Skip RenameMethodRector on interface wildcard used #5652
Conversation
|
I think the best thing to do would be to rollback #5622 as it does not what it should do. |
|
I'll work on a PR to see if I can implement it. /cc @samsonasik |
…rectorphp#5622)" This reverts commit 0b17b91.
I realized that I did not provide the test case in rectorphp#5564 correctly as I was running it in my project. By changing the existing test it fails again. Why? Because `OtherWildcardSubscriber` does match the wildcard `*WildcardSubscriber`. That one does not implement an interface. Because `$this->classManipulator->hasParentMethodOrInterface` just tries to find the first matching class that matches the wildcard, it checks that one always. That means that the implementation in rectorphp#5622 is not correct (and naive).
336a3fd to
a861eed
Compare
|
Master is broken... partial fix here > #5623 @samsonasik What do you think of this approach? |
|
I think new fixture is better instead of modifying existing fixture to keep existing behaviour. |
|
Yeah, not sure. As the old one did not solve my use case + I reverted the PR. |
|
Ran this on my project and now it finally works without issues 🎉 |
|
@TomasVotruba What do you think? |
|
I'm waiting for CI to be green, so I can review & merge. Also, could you separate the fixture into new file? Now it's extending an existing one and it's harder to see what is going on. |
|
First, thank you both for putting time into this. In the hindsight I see it was a mistake to go with wildcards on class type resolution. Here the PHPStorm refactoring or custom Rector rule should be used instead. Surpassed by #5693 (comment) |
rectorphp/rector-src@99a79f8 [TypeDeclaration] Fix abs() returns on ReturnTypeFromStrictTypedCallRector (#5652)
I realized that I did not provide the test case in #5564 correctly as I was running it in my project.
By changing the existing test it fails again.
Why? Because
OtherWildcardSubscriberdoes match the wildcard*WildcardSubscriber. That one does not implement an interface.Because
$this->classManipulator->hasParentMethodOrInterfacejust tries to find the first matching class that matches the wildcard, it checks that one always. And since that one does not implement an interface, it will allow refactoring. While it shouldn't.That means that the implementation in #5622 is not correct (and naive).
I'm trying to think of a way how to do this.
/cc @samsonasik