Rector: Class implementing interface but providing more methods than those implemented should not be swapped with interface

[Wirone]

Bug Report

| Subject | Details |
| :------------- | :---------------------------------------------------------------|
| Rector version | 0.8.56 |
| Installed as | composer dependency |

After upgrading Symfony from 5.1.8 to 5.2.0 Rector tries to enforce AuthorizationCheckerInterface of a used-on-purpose Security class. However, this class has more public methods than those implemented from interface, so such change would lead to other problems with IDE autocompletion and SA tools.

Minimal PHP Code Causing Issue

I have SentryListener based on @B-Galati's work and it uses Symfony\Component\Security\Core\Security in order to get user (Security::getUser()). Security class implements AuthorizationCheckerInterface which defines only isGranted(), so getUser() and getToken() are methods defined on Security class' level, hence class and interface are not interchangeable.

1) src/Infrastructure/Sentry/SentryListener.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -2,6 +2,7 @@

 namespace App\Infrastructure\Sentry;

+use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
 use Psr\Log\LoggerInterface;
 use ReflectionClass;
 use Sentry\State\HubInterface;
@@ -26,7 +27,7 @@
     private Security $security;
     private LoggerInterface $logger;

-    public function __construct(HubInterface $hub, Security $security, LoggerInterface $logger)
+    public function __construct(HubInterface $hub, AuthorizationCheckerInterface $security, LoggerInterface $logger)
     {
         $this->hub = $hub;
         $this->security = $security;
    ----------- end diff -----------


Applied rules:

 * Rector\SOLID\Rector\ClassMethod\UseInterfaceOverImplementationInConstructorRector
 * Rector\TypeDeclaration\Rector\ClassMethod\AddParamTypeDeclarationRector

and $this->security->getUser() is used inside SentryListener::onKernelRequest()

Expected Behaviour

Rector should not enforce interface if code uses object's methods defined outside of that interface (on class level or from traits). I don't know if it's possible to detect usage of an argument which type is about to be changed from concrete class to interface, but honestly such enforcement in my opinion looks too optimistic without such check. Class can have 1 interface implemented (one of the UseInterfaceOverImplementationInConstructorRector::refactor()'s condition) but still can have wider public API than defined in the interface.

[TomasVotruba]

Hi,

thanks for reporting, this makes sense.
We'll need a failing test case.

[Wirone]

@TomasVotruba I find it hard to prepare test case. I have added fixture but when I run tests I got:

vendor/bin/phpunit rules/solid/tests/Rector/ClassMethod/UseInterfaceOverImplementationInConstructorRector/UseInterfaceOverImplementationInConstructorRectorTest.php          
PHPUnit 9.5.0 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

........                                                            8 / 8 (100%)

Time: 00:03.427, Memory: 144.50 MB

OK (8 tests, 16 assertions)

However if I enforce only my fixture (by deleting other fixtures or by changing UseInterfaceOverImplementationInConstructorRectorTest::provideData() so it yields only my fixture) I got:

vendor/bin/phpunit rules/solid/tests/Rector/ClassMethod/UseInterfaceOverImplementationInConstructorRector/UseInterfaceOverImplementationInConstructorRectorTest.php
PHPUnit 9.5.0 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F                                                                   1 / 1 (100%)

Time: 00:04.259, Memory: 142.50 MB

There was 1 failure:

1) Rector\SOLID\Tests\Rector\ClassMethod\UseInterfaceOverImplementationInConstructorRector\UseInterfaceOverImplementationInConstructorRectorTest::test with data set #0 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
rules/solid/tests/Rector/ClassMethod/UseInterfaceOverImplementationInConstructorRector/Fixture/skip_class_that_implements_single_interface_but_defines_own_public_methods.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@

 class SkipClassThatImplementsSingleInterfaceButDefinesOwnPublicMethods
 {
-    public function __construct(ClassThatImplementsInterfaceAndDefinesOwnPublicMethods $someImplementation)
+    public function __construct(\Rector\SOLID\Tests\Rector\ClassMethod\UseInterfaceOverImplementationInConstructorRector\Source\InterfaceOne $someImplementation)
     {
     }
 }

What am I doing wrong?

[TomasVotruba]

I can only guess without code. Could you open PR so I see it?

Possibly create new classes/interface for your fixture in /Source, instead of re-using existing classes.

[Wirone]

@TomasVotruba added PR #4845 with test cases. I had to add 2 new interfaces along with 2 new classes because even though fixtures were using new classes, if those classes were using interfaces from other fixtures, those tests were false-positive. Hope those test cases are good to start with this.

[Wirone]

@TomasVotruba I think I fixed this in #4845 :tada: :slightly_smiling_face: Waiting for a review, cheers!

[TomasVotruba]

Thank you for issue and fix, that's joy to merge :)