slevomat / coding-standard

Slevomat Coding Standard for PHP_CodeSniffer provides many useful sniffs
MIT License
1.39k stars 176 forks source link

Allow disabling BC breaking behavior in TypeHintDeclarationSniff #242

Open alcaeus opened 6 years ago

alcaeus commented 6 years ago

This was previously reported in #202 and #221.

An example from Doctrine MongoDB ODM:

    /**
     * Factory for returning new PersistenceBuilder instances used for preparing data into
     * queries for insert persistence.
     *
     * @return PersistenceBuilder $pb
     */
    public function getPersistenceBuilder()
    // ...

Enabling SlevomatCodingStandard.TypeHints.TypeHintDeclaration throws a fixable MissingReturnTypeHint error because the return type hint could be moved to the method signature.

Of course, fixing that error produces a BC break, so the only option for now is to either ignore the MissingReturnTypeHint on every method or disable the entire sniff because it can produce BC break.

It would be nice to have an option to disable the BC breaking behavior, which would suppress that specific error for all non-private members. An alternative would be splitting up the sniff into smaller parts and adding dedicated error codes (e.g. typehint missing completely, typehint could be written in signature, etc.).

kukulich commented 6 years ago

Yes, TypeHintDeclarationSniff is one of the sniffs that really need refactoring (splitting to more sniffs). I plan to do it in next major release but I don't have any schedule currently.

The sniff is already very complicated so I don't want to add another option to it now.

So I'm sorry, using @phpcsSuppress annotation is therefore the only option how to solve your problem and not exclude the whole sniff :(

stephanvierkant commented 6 years ago

An alternative would be splitting up the sniff into smaller parts and adding dedicated error codes (e.g. typehint missing completely, typehint could be written in signature, etc.).

See #353

Majkl578 commented 6 years ago

FYI for now you can use phpcs-only="true" to disable fixers for specific rules (should imho work with specific codes too) or disable some codes compeletely with <exclude>.

stephanvierkant commented 5 years ago

It would be great if we can exclude method names from fixing.

For example: \Symfony\Component\Form\AbstractType::buildForm \Symfony\Component\Form\AbstractType::configureOptions \Symfony\Component\Security\Core\Authorization\Voter\Voter::supports \Symfony\Component\Security\Core\Authorization\Voter\Voter::voteOnAttribute

vv12131415 commented 5 years ago

It would be great if we can exclude method names from fixing.

For example: \Symfony\Component\Form\AbstractType::buildForm \Symfony\Component\Form\AbstractType::configureOptions \Symfony\Component\Security\Core\Authorization\Voter\Voter::supports \Symfony\Component\Security\Core\Authorization\Voter\Voter::voteOnAttribute

yes, something like this https://github.com/phpstan/phpstan#ignore-error-messages-with-regular-expressions

kukulich commented 5 years ago

@stephanvierkant @vladyslavstartsev it’s not possible because PHPCS works only in scope of the current file. You can only use @phpcsSuppress

stephanvierkant commented 5 years ago

@kukulich I understand that doesn't know about C in class A extends B; class B extends C, but let's take this example:

(pseudo)code:

namespace App;
class A extends App\B {
    public function someMethod() {}
}

class App\B implements Vendor\C {
    public function someMethod() {}
}

namespace Vendor;
interface C {
    /** @return bool */
    public function someMethod();
}

I'd like to add Vendor\C::someMethod to a ignore list so MissingReturnTypeHint doesn't add : bool to it.

In this case phpcs doesn't know about interface C when checking class A, but it knows (at least the existence of) about it in class B. This requires adding implements App\C to class A, but I'd prefer that over adding // phpcs:ignore everywhere.

kukulich commented 5 years ago

@stephanvierkant No, PHPCS doesn't know anything about class B. It's possible to check only methods in the current class.

stephanvierkant commented 5 years ago

But it knows about the existence of B, isn't it? I understand it doesn't know that A implements C, but it does for B.

I want to add Vendor\C::someMethod to a whitelist, which means 'ignore rule on method 'someMethod' if current class directly extends/implements C`. It's not relevant what's in class C.

See https://github.com/squizlabs/PHP_CodeSniffer/issues/2508