New Feature: only load sniffs containing fixers when PHPCBF is run

[jrfnl]

Is your feature request related to a problem?

Some standards, like, for instance, PHPCompatibility contain a lot of sniff (150+), but barely any fixers. Another example is the VariableAnalysis standard, which only contains one non-fixable sniff, but has to do a lot of work for that one sniff, which makes it a slow sniff.

As things are, when running phpcbf all sniffs are loaded, whether they contain fixers or not. This also means that all sniffs are run in all fixer loops as PHPCS will only know about fixable errors/warnings once the sniff triggers an error/warning.

All in all, this can make PHPCBF much slower than is needed when a ruleset contains a lot of non-fixable sniffs.

Describe the solution you'd like

A preliminary idea is to have a FixableSniff and a NonFixableSniff interface.

When running phpcbf PHPCS could then decide when reading the ruleset and loading the sniffs whether sniffs should actually be run, which could lead to a significant performance boost in cases when the ruleset contains quite some sniffs not containing fixers.

Opinions ? Other ideas to tackle this ?

/cc @kukulich @wimg @GaryJones @dingo-d @fredden @michalbundyra @stronk7

Additional context (optional)

If there is interest in this idea, I'd be looking to introduce this feature in the 4.0 release.

The Sniff interface would then be (soft) deprecated in 4.0, to be removed in 5.0, which should give standard maintainers time to switch over to the new interfaces.

If when running on 4.x, the Sniff interface is still used, it would be treated as if it were the FixableSniff interface and the sniff would still run in phpcbf mode.

• I intend to create a pull request to implement this feature.

[kukulich]

I like the idea 👍

I would like more version that Sniff interface will be preserved and it would be possible to add FixableSniff interface to mark the sniff as fixable.

However I understand that you want to be backward compatible...

[jrfnl]

I would like more version that Sniff interface will be preserved and it would be possible to add FixableSniff interface to mark the sniff as fixable.

However I understand that you want to be backward compatible...

I don't mind preserving the Sniff interface, but yes, to allow for standards to upgrade in their own time, that would mean "Sniff = FixableSniff" as without a different interface name, there is no way for PHPCS to know whether to use the sniff when fixing or not.

So then we'd end up with Sniff and NonFixableSniff which feels a bit off.

[stronk7]

Hi,

just thinking how the interfaces stuff will work when Sniffs aren't implementing Sniff but, instead, they are extending other (abstract) Sniffs.

Take for example AbstractScopeSniff, there are dozens of Sniffs extending it... I imagine that some are "fixable" and some are not... and it doesn't seem that we can use the interfaces to decide anything there?

A possible alternative, just came to my brain because I've been working today with something similar, used to declare some stuff as "deprecated" within an app, could be to have some simple Trait, say NonFixableTrait (maybe also the opposite one, not sure), adding something for easy detection?

For some reason, I find the trait alternative natural enough... and that won't require us to go splitting / changing all the extensions tree.

Just shared, without too much reflexion, heh, so take it with a pinch of salt.

Ciao :-)

[stronk7]

OT, hah, curious... I've just seen #164... about Sniff deprecations. Another example where Traits could came to rescue.

[jrfnl]

@stronk7 Thanks for your input.

Take for example AbstractScopeSniff, there are dozens of Sniffs extending it... I imagine that some are "fixable" and some are not... and it doesn't seem that we can use the interfaces to decide anything there?

A class can extend only one other class, but can implement multiple interfaces.

So to take your example, to mark a sniff as containing a fixer, the class signature would become:

class CamelCapsFunctionNameSniff extends AbstractScopeSniff implements FixableSniff {}

// We could even combine it with the interface proposed in #164:
class CamelCapsFunctionNameSniff extends AbstractScopeSniff implements NonFixableSniff, DeprecatedSniff {}

The interfaces can be detected easily via an instanceof check (or if needs be via Reflection).

So yes, interfaces would work perfectly fine for this.

A possible alternative, just came to my brain because I've been working today with something similar, used to declare some stuff as "deprecated" within an app, could be to have some simple Trait, say NonFixableTrait (maybe also the opposite one, not sure), adding something for easy detection?

Traits would need some content for PHPCS to be able to detect their use, while there is no content PHPCS could/should provide by default for this functionality.

While I have nothing against traits, in my opinion, they are not the right tool for the job in this case.

[stronk7]

Last question… and which interface will abstract sniffs implement? I mean, after all, we will need the 3 (Sniff, fixable and non-fixable) to stay forever…

[jrfnl]

Last question… and which interface will abstract sniffs implement?

In most cases, in my thinking, that would be FixableSniff, which would effectively mean that the child classes are allowed to be fixable.
Though in practice, I see where you are coming from, it would probably be best for those to implement Sniff (in practice that would mean the same thing).

Having said that, I can imagine there may be a rare situation where the abstract would implement NonFixableSniff to effectively forbid child classes to contain fixers.

I mean, after all, we will need the 3 (Sniff, fixable and non-fixable) to stay forever…

You are probably right about that.