[TypeDeclaration] Make configurable on NarrowObjectReturnTypeRector to allow to not make change return on abstract/interface return

[samsonasik]

@Orest-Divintari @ruudk @calebdw per our discussion on the original PR:

• Narrow object return type #7575

the contract return was returned, but this cause some issue reported while interface return is valid, see issue:

• NarrowObjectReturnTypeRector replaces interface with implementing concrete class rector#9525
• Incorrect behavior of NarrowObjectReturnTypeRector rector#9489

This PR make it configurable, for allow to skip abstract/interface return, for whatever default value is, this imo should be configurable.

Closes rectorphp/rector#9525

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[TomasVotruba]

Looks good 👍

I just find this option name confusing. Could you name it INCLUDE_STANDALONE_CLASS?

[samsonasik]

That means the value need to be true, default false, but "include" term means "also", which a bit contradictive imo.

For better word, I think it can be ALLOW_CONTRACT_RETURN

It is about adding return type which return type is contract.

[TomasVotruba]

Include standalone class should be false by default.

[samsonasik]

It is nothing todo with standalone class or not, it is about returning abstract or interface as return type, whether class extends another class or not, if the method return interface, this config will skip.

[TomasVotruba]

I see. I've checked the original report again.

In that case we don't need any configuration. That case should be skipped, as it would create breaking code.

[samsonasik]

we had discussion about this on original PR with @Orest-Divintari and @ruudk , the use of replace interface/abstract class to concrete class is valid,

But as @calebdw and @WyriHaximus pointed, there are use case which keep interface/abstract as return type make sense, usually on unit test purpose which still use mock object everywhere.

[TomasVotruba]

Yes, but how do you make a boundary which interface should be replaced and which not? This rule replace e.g. all getRule() in phsptan tests with exact return type of the rule. It doesn't add any value, as parent return type is generic on purpose. It's part of contract.

Once we start adding such configuration, this rule turns into manual labour I want to avoid. This rule should only replace one object type with more specific one. Not all interface types with exact returns, which sparks last 2 reports.

[TomasVotruba]

I'll look into this tomorrow. Closing to avoid further configuration on this rule.

[samsonasik]

Common interface type to be skipped:

• Interface return with Psr\\ prefix
• Interface return with Interop\\ prefix

Common interface type should be allowed to be replaced:

• Doctrine\Common\Collections\Collection
• PhpParser\Node

This is what I proposed in old PR 3 weeks ago

• [TypeDeclaration] Skip return interface on NarrowObjectReturnTypeRector #7621

and closed.