Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
func0der opened this issue May 13, 2024 · 4 comments

Comments

@func0der
Copy link

func0der commented May 13, 2024

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
Copy link
Member

jrfnl commented May 14, 2024

@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
Copy link
Author

func0der commented May 15, 2024

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
Copy link
Member

jrfnl commented May 15, 2024

@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
Copy link
Author

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

Thanks for the insights :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants