PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
805 stars 47 forks source link

Needed interface parameters meaning is lost if class extends class that implements that interface #490

Closed func0der closed 2 months ago

func0der commented 2 months ago

Describe the bug

Given an interface (MailProviderInterface) that requires a certain method signature (getConfig). Given an (abstract) class (AbstractMailProvider) implementing that interface. Given a class that (LocalMailProvider) extends the abstract class.

If not all parameters in LocalMailProvider::getConfig() are used, you get an error.

Code sample

<?php

namespace Test;

interface MailProviderInterface
{
    public static function getConfig(string $mailer, array $config): array;
}
<?php

namespace Test;

abstract class AbstractMailProvider implements MailProviderInterface
{
    public static function getConfig(string $mailer, array $config): array
    {
        if ($mailer === 'smtp') {
            return [];
        }

        return $config;
    }
}
<?php

namespace Test;

class LocalMailProvider extends AbstractMailProvider
{
    public static function getConfig(string $mailer, array $config): array
    {
        return $config;
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Test">
  <rule ref="Generic.CodeAnalysis.UnusedFunctionParameter"/>
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a the classes from the example code above
  2. Run phpcs test.php ...
  3. See error message displayed

FILE: LocalMailProvider.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

7 | WARNING | The method parameter $mailer is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClassBeforeLastUsed)



## Expected behavior

The implementation of the interface from parent classes should not be forgotten.

## Versions (please complete the following information)

|                                       |                                                                                                           |
|--------------------------|----------------------------------------------------------------------  |
| Operating System           | Linux                                                      |
| PHP version                    | 8.1.27 |
| PHP_CodeSniffer version | 3.9.1 |
| Standard                         | Generic.CodeAnalysis.UnusedFunctionParameter                                                      |
| Install type                      | composer |

## Additional context

-

## Please confirm

- [x ] I have searched the issue list and am not opening a duplicate issue.
- [x ] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
- [x ] I have verified the issue still exists in the `master` branch of PHP_CodeSniffer.
jrfnl commented 2 months ago

@func0der PHPCS can not look beyond the current file, but I do understand your problem.

It is the reason why the sniff has modular error codes since squizlabs/PHP_CodeSniffer#1318, which allows you to choose which errors to ignore by default, like FoundInExtendedClassBeforeLastUsed.

For a full list of the error codes which are available, have a look at this comment.

You can use these in your custom ruleset like so (example only, customize to your own liking):

<rule ref="Generic.CodeAnalysis.UnusedFunctionParameter">
    <!-- Allow for callback functions which may not use all parameters passed. -->
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundBeforeLastUsed"/>
    <!-- Allow for functions in extended classes/implemented interfaces. -->
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClass"/>
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClassBeforeLastUsed"/>
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClassAfterLastUsed"/>
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInImplementedInterface"/>
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInImplementedInterfaceBeforeLastUsed"/>
    <exclude name="Generic.CodeAnalysis.UnusedFunctionParameter.FoundInImplementedInterfaceAfterLastUsed"/>
</rule>
func0der commented 2 months ago

Thanks for the quick replay.

It does not feel right though to disable all those checks, because of some 'false positives'. Wouldn't that defeat the purpose of the check itself? Also from code I understand that it only checks if the class is extending or implementing something, anything basically. So the existence of the method in question is not checked at all (probably due to missing context analysis, right).

Clearly the alternative is to ignore each and every one of those "false positives" by hand, I am just wondering if we could improve the reporting itself.

Should they even be reported if there is an interface or an extending class, which would require some context awareness, I guess.

jrfnl commented 2 months ago

@func0der The sniff has no access to information about classes/interfaces declared in other files, so this is the most optimal solution within the current framework. Changing the framework on which PHPCS is build, is a completely different discussion and not on the table.

func0der commented 2 months ago

Understood. Just wanted to confirm that this is the current state.

Thanks for the insights :)