Rector: RenamePropertyToMatchTypeRector issue

[mssimi]

Bug Report RenamePropertyToMatchTypeRector

Rector version: v0.7.61
Installed as composer dependency

Vendor interface

namespace Some\Vendor\Namespace;

interface SomethingInterface 
{
    public function doSomething(Request $req): void;
}

Before RenamePropertyToMatchTypeRector

namespace App\Service;

use Some\Vendor\Namespace\SomethingInterface;

final class Something implements SomethingInterface 
{
    public function doSomething(Request $req): void {
        // ...
    }
}

After RenamePropertyToMatchTypeRector

namespace App\Service;

use Some\Vendor\Namespace\SomethingInterface;

final class Something implements SomethingInterface 
{
    public function doSomething(Request $request): void {
        // ...
    }
}

Expected Behaviour

Keep it as is. RenamePropertyToMatchTypeRector must not rename properties of methods that are implementation of vendor interface, as it will break code.

[TomasVotruba]

On second look, the change is correct.

Could you share https://3v4l.org snippet that breaks code?

[mssimi]

I have updated example in original msg, but point is simple, RenamePropertyToMatchTypeRector must not rename properties that are signatures of vendor interfaces.

[TomasVotruba]

I'm confused. I see params in the snippet, but here you talk about properties.

Could you provide failing test case to the rule you talk about? That would make it clear without words :+1:

[mssimi]

Can we just have a call?

[mssimi]

Well right, I talk about properties coz rector is named RenameProperty..., but it actually renames params.

[mssimi]

Test is not example of real world, how would I simulate vendor package?

[dobryy]

I see what you mean @mssimi. Pull Request with test fixture could be helpful :)

[mssimi]

@dobryy Thx, I would made test instead of issue, if I knew how to simulate interface is in vendor in that fixture. :)

[dobryy]

You can create Interface for testing purposes here and use it in Test Fixture in order to reproduce an issue.

[mssimi]

Example you given show how to simulate interface, but does not show how to simulate, that interface is part of vendor package.

[mssimi]

I think we still don't understand each other, sadly I don't know how can I provide more info.

[TomasVotruba]

vendor package?

That's a good question. As @dobryy said, put the vendor file into /Source directory

I try to put it visually, maybe better:

/Fixture # directory is autolaoded and parsed
/Source # directory is autoloaded, but not parsed, as /vendor in real-life
RectorTest.php

[dobryy]

Example you given show how to simulate interface, but does not show how to simulate, that interface is part of vendor package.

You're right... but I guess, it should skip not only classes/methods that implement interfaces from vendor folder but also interfaces outside of included and inside excluded paths configured in rector.php. Source folder is the one that is excluded thus if Test Class implements Interface from it it should skip the renaming. Am I right here @TomasVotruba?

I think we still don't understand each other, sadly I don't know how can I provide more info.

I definitely do understand what you mean.

[mssimi]

it should skip not only classes/methods that implement interfaces from vendor folder but also interfaces outside of included and inside excluded paths configured in rector.php

And that is what I think too, I am happy we understand each other.

@TomasVotruba thx for clarification, I did not know that. I will try provide test later today then.

[TomasVotruba]

@mssimi

Can we just have a call?

I've just noticed it. No need to ask, just call next time. The ringing of phone is question itself :)