4.0 | Formally stop support for sniffs not complying with the PHPCS naming conventions

[jrfnl]

While PHPCS currently has partial/limited support for sniff file includes of sniffs which do not comply with the PHPCS naming conventions (as outlined in the Coding Standard Tutorial), AFAICS this is more by accident than by design and I propose to formally drop support for this in PHPCS 4.0.

What does "formally drop support" mean ?

• A mention of this in the release notes/upgrade guide(s).
• Revert PR Common::getSniffCode(): be more lenient about sniffs not following naming conventions #676 (remove leniency about sniffs not following naming conventions from the Common::getSniffCode() method)
• If/when code is discovered which explicitly supports unconventional setups, remove that code (and any related tests; or possibly: change the tests to confirm this is not supported).
  Note: this task does not have to be completed for the 4.0 release and code covered by this task can be removed at any point in time after the 4.0 release, as long as the 4.0 task 1 (explicit mention in release notes and upgrade guide) has been executed.
• No longer accept any bug reports/feature requests related to sniffs which do not comply with the PHPCS naming conventions and close those immediately and without hesistation.
• Other than that, I think it would be good to review the text of the Coding Standard Tutorial and to clarify the expected naming conventions where needed.

Note: including sniffs by file name will still be supported, but the sniffs included MUST be in a directory structure and with a namespace and class name which comply with the PHPCS naming conventions.

Related #675, #680, #683

Opinions ?

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

[greg0ire]

Does PHPCS have a deprecation mechanism that could be leveraged to make people aware of this in 3.x, and to migrate ahead of time? Maybe that could reduce the number of reports filed.

[dingo-d]

A deprecation notice would be great and would definitely urge the external standards maintainers to fix this ahead of time.

[jrfnl]

Does PHPCS have a deprecation mechanism that could be leveraged to make people aware of this in 3.x, and to migrate ahead of time? Maybe that could reduce the number of reports filed.

Good point. PHPCS doesn't have a deprecation mechanism for this, but I think this can probably be detected when loading the ruleset, which would mean we could throw a deprecation notice without impacting CS runs.

I'm going to look into this.

[fredden]

Yes to the plan, but I'd also like to see a code change which detects sniffs which do not confirm and refuses to load these into a ruleset, with a user-visible notice ideally.

[jrfnl]

Just FYI: I am working on adding an "error handling" mechanism to the Ruleset to allow for throwing such deprecation notices (and the warning @fredden mentions once we get to 4.0).

Principles I'm working from:

• Classify "errors" as "blocking" (like no sniffs being registered) or "non-blocking" (like an incorrect <type> being passed).
• Don't hide one error behind another (if it can be avoided).

In practice this will come down to all errors being collected while the ruleset(s) are being processed and then at the end of the processing:

• First display non-blocking issues (deprecations, notices, warnings).
• Next display blocking errors.
• If there are no blocking errors: continue the scan run.

Non-blocking errors will not affect the exit code and not be displayed in -q (quiet) mode.
Blocking errors will result in exit code 3 (as before).

The above will also be applied to the pre-existing errors being thrown from the Ruleset class.

The "problem" I'm running into is that the Ruleset class has relatively low code coverage and a lot of the pre-existing errors are not covered by tests.

In other words, I'm currently writing a sh*tload of tests to improve the code coverage of the Ruleset class. I expect to pull a lot of these over the next week or two.

After that I can introduce the new error handling in v 3.12.0 and possibly combine it straight away with a deprecation notice for the type of setups being discussed in this issue.

How does that sound ?

[greg0ire]

Sounds like a great improvement!

[mk-mxp]

We have been struggling to adjust our custom sniff to the naming conventions, as we use no full featured coding standard with an XML, but directly include the class in the main configuration. Our struggle was due to these points:

• The Coding Standard Tutorial contains all the steps required to setup a full featured coding standard, but has no instructions on our use case
• The Annotated Ruleset only shows how to refer to the class file, but has no further details

I suggest the following changes to these places, now or when the naming conventions are enforced:

Annotated Ruleset

<!--
   If you are including sniffs that are not installed, you can
   reference the sniff class using an absolute or relative path
   instead of using the sniff code.
-->

should become

<!--
   If you are including sniffs that are not installed, you can
   reference the sniff class using an absolute or relative path
   instead of using the sniff code.
   The path and class name must follow the naming conventions
   outlined in the Coding Standard Tutorial. Do not add sniff
   classes to your autoloading. See
   https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Coding-Standard-Tutorial
-->

Coding Standard Tutorial

I suggest adding a heading Naming conventions that can be linked to directly. As far as I remember, the conventions are:

• Path structure {any prefix}/{Coding standard name}/Sniffs/{Category}/{SniffName}Sniff.php
• Namespace structure [{any prefix}\]{Coding standard name}\Sniffs\{Category}\{SniffName}
• Resulting sniff name {Coding standard name}.{Category}.{SniffName} to use in rulesets
• No ., / or \ allowed in {Coding standard name}, {Category} and {SniffName}
• Choose any {Coding standard name}, {Category} and {SniffName} you like
• Do not include Sniff classes in your (e.g. composer-provided) autoloading, PHP_CodeSniffer handles class loading on its own

Such a summarized specification of the naming convention would be enough, I think.

Edit: Added hints on autoloading.

[jrfnl]

@mk-mxp Oh, thank you, that's a nice actionlist for me to apply!

I'm going to wait with updating the Wiki until I've finished the branch for this change locally (to see if I find anything else which should be included or is subtly different), but this will definitely come in very handy!

[jrfnl]

FYI: I'm nearly finished with all the work on the Ruleset class. Turned out that writing those tests was quite a lot of work 😉.
Once PR #801 has been merged, I can pull the changes as outlined in my comment above #689 (comment)

So... I've now also spend some time on the wiki and have added a new page "About Standards for PHP_CodeSniffer" which includes detailed information and examples about the naming conventions (and more).
Links to the new page have been added in the Annotated Ruleset and the Coding Standard Tutorial pages.

I'd love for you all to have a read through the new page. Feedback and improvements to the page are very welcome.

[greg0ire]

Very clear wiki page!

I think there are possibly some small fixes:

build-in => built-in
end on => end with

I'm not familiar with wiki pages, but if it's the same as for regular markdown file, I'd enable syntactic coloration with the syntax that consists in following the triple backticks with php

Also, I don't think invalid examples are useful given how clear the rest of the pages already is, they could even be confusing if somebody reads too fast and decides to use copy/paste from the wrong section.

[mk-mxp]

Very impressive piece of documentation! And the Annotated Ruleset got a link to the naming conventions, too. Thanks a lot for your work!

I also like the invalid examples and don't think they do any harm. There is lots of valid copy&paste sources directly above those. Often it's easier to see your own faulty decisions in a negative example compared to mentally diffing against valid code.

[jrfnl]

@greg0ire @mk-mxp Thank you both for reviewing and for your feedback.

I've made the following updates to the page now:

• Syntax highlighting for the PHP code samples (not sure how I missed that the first time around, sorry about that)
• Spelling/grammar fixes as suggested by @greg0ire
• I've kept the invalid examples for now, but have added a ❌ in front of the description for each, which should hopefully lessen the chance that people copy an invalid code sample.

[jrfnl]

Made one more update to the page:

The name "Sniffs" MUST NOT be used as a category name.