Do not rename methods from interface when using wildcard #5564
Conversation
| @@ -25,6 +25,11 @@ | |||
| 'old', | |||
| 'new' | |||
| ), | |||
| new MethodCallRename( | |||
| 'Rector\Renaming\Tests\Rector\MethodCall\RenameMethodRector\Fixture\*WildcardSubscriber', | |||
There was a problem hiding this comment.
Could you provide https://3v4l.org proof that this fnmatch() works with expected class?
That would narrow the scope for the issue
There was a problem hiding this comment.
Good one. I checked, and it does work: https://3v4l.org/4tCj4
There was a problem hiding this comment.
The problem is that getOldClass() still contains the wildcard and is then passed to ClassManipulator::hasParentMethodOrInterface.
There was a problem hiding this comment.
I don't know.
@samsonasik added this:
rector/rules/renaming/src/Rector/MethodCall/RenameMethodRector.php
Lines 88 to 94 in 8d58a6d
But that doesn't have support for fnmatch.
I think that we should not use $methodCallRename->getOldClass() as it can be a FQCN or wildcard matching.
Instead, we should check if the Class $node has interfaces that implement $methodCallRename->getOldMethod() method. If that's the case, it should not rename. But I tried this already and I couldn't get it to work.
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 `interface WildcardSubscriber` matches the config.
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).
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).
Failing test for #5554
cc @samsonasik