Clean MethodToMethodCallRector

[SelrahcD]

Hi all!

I was playing with Rector, trying to implement a new Rector and needed to make a replacement for a method from the current class. I managed to do it using AbstractRector::isObjectType and AbstractRector::isName and it felt simpler from what I saw in the MethodCallToMethodCallRector.

I decided to give it a try and see if I could clean the rule a little, and apparently, I managed to do it, as all tests are still passing. I removed some unnecessary checks and some of the logic that seems to not be mandatory.

Still, maybe I don't know something and the previous implementation might be better.

[samsonasik]

Thank you for the contribution, since it can cause issue that never exist before, it should not be applied.

[TomasVotruba]

Hi, possibly test coverage needs to be improved :) if you can add tests cases from the feedback of @samsonasik , and types are strict and passing, we can move forward.

@samsonasik Let's leave it open to give space for feedback loop.

[SelrahcD]

Hi!

First, thanks for reopening the PR. I saw the closing message this morning and I was a bit surprised. No harm done and let's move

Adding the missing test cases and moving the method call outside the loop was my plan.

I'll probably need some clarification as I don't understand one of the comments :)

[SelrahcD]

In the end, I'm a bit lost on what that rule should do. Should it replace all calls to a method call or just in some cases ?

What is the expected behavior if the dependency comes from a function call, like #2554 (comment)?
How should we handle a call to a method defined in parent or child classes?

On one end it seems that having a rule that does all replacements could be useful, and on the other, I get that doing so might be problematic in some codebases, mostly with the replacement of methods that were overridden in child classes.

[SelrahcD]

I have added back the check for only replacing methods from the current class and not from parent and a few test cases.

In the end we probably all don't have the same understanding of what that rule should be doing. Maybe it would be better to just stick to the previous behavior of only modifying method calls to injected service, as @samsonasik understood it. The more behavior we add to the rule the more it will be difficult to decide what it should be doing or not.

Anyway, I've learned a few things, so thank you for your time and feedbacks :)

[SelrahcD]

I made the requested changes :)

[samsonasik]

Not really necessary on PR, but you can fix cs via command:

composer fix-cs -- rules/Transform/Rector/MethodCall/MethodCallToMethodCallRector.php

[TomasVotruba]

Thank you 👍