Based on a discussion between @fredden and me.
Why have PHPCS specific sniffs ?
- There are a number of best practices when writing sniffs for which compliance currently needs to be checked manually in PRs by a human reviewer, while this could be automated.
- There are a few "upgrades" which could be made to sniffs once support for PHPCS 3.x has been dropped, for which auto-fixers could be written (and more things like this are expected for the future).
- Etc...
It would be helpful for maintainers of PHPCS standards, including the maintainers of PHPCS itself, to have these kind of things automatically flagged via the CI on PRs.
Why wasn't this done before ?
Well, I did actually think about this before, but as the public for these type of sniffs is very limited, I didn't deem it worth the time of writing and maintaining the sniffs. Aside from that, there always were other things to do in the repos which had higher priority.
This argument still stands, but who knows, someone else may have the time ?
Why should this be part of this repo ?
When thinking through the most logical place for these type of sniffs, there are a number of options:
- PHPCSDevTools
- New, separate repo
- This repo
PHPCSDevTools is not a good option because the PHPCSDebug standard/sniff should be compatible with a wide range of PHPCS versions, while I see no reason for sniffs in a PhpcsQA standard to be compatible with a wide range of PHPCS versions, but as PHPCSDebug is part of DevTools, the minimum supported PHPCS version cannot (and should not) be raised without good reason.
A new, separate repo has the positive side-effect that it might make the sniffs/standard more discoverable as the package/repo will be published as a new, separate package.
On the "con" side, it would mean yet another repo to maintain.
As for this repo....
This repo already provides a coding standard for devs writing PHPCS sniffs, so a significant part of the expected public for these sniffs already uses PHPCSDev anyhow, so it seems like a good fit.
Why set this up as a separate standard instead of adding sniffs to the existing PHPCSDev standard ?
To make it more straight-forward for packages which want to use the PHPCS specific sniffs, but not the PHPCSDev standard, to include just these sniffs.
If the sniffs would be added to the PHPCSDev standard, devs for an external standard would need to keep track of activity in this repo and would need to include each PHPCS-related sniff individually in their own project ruleset.
<rule ref="PHPCSDev.CatA.SniffNameA"/>
<rule ref="PHPCSDev.CatA.SniffNameB"/>
<rule ref="PHPCSDev.CatB.SniffNameA"/>
... while, if the sniffs would be added to a new, separate, standard which only contains these sniffs, devs for packages which want to use these sniffs could just do:
... and will then automatically benefit from every new PhpcsQA sniff being added, while they could also still use selective inclusions as per the previous example if so desired.
Initial sniff idea list
Another idea, though that probably doesn't belong in this standard and might be better to write as a generic code analysis sniff for PHPCSExtra: a sniff to check that most PHP native array_* functions aren't used directly on deeply nested arrays.
I.e. don't do: array_pop($array['key1']['key2']), but assign $array['key1']['key2']to an interim variable ahead of thearray_pop()` function call.
Additional actions needed
Based on a discussion between @fredden and me.
Why have PHPCS specific sniffs ?
It would be helpful for maintainers of PHPCS standards, including the maintainers of PHPCS itself, to have these kind of things automatically flagged via the CI on PRs.
Why wasn't this done before ?
Well, I did actually think about this before, but as the public for these type of sniffs is very limited, I didn't deem it worth the time of writing and maintaining the sniffs. Aside from that, there always were other things to do in the repos which had higher priority.
This argument still stands, but who knows, someone else may have the time ?
Why should this be part of this repo ?
When thinking through the most logical place for these type of sniffs, there are a number of options:
PHPCSDevToolsis not a good option because thePHPCSDebugstandard/sniff should be compatible with a wide range of PHPCS versions, while I see no reason for sniffs in aPhpcsQAstandard to be compatible with a wide range of PHPCS versions, but asPHPCSDebugis part of DevTools, the minimum supported PHPCS version cannot (and should not) be raised without good reason.A new, separate repo has the positive side-effect that it might make the sniffs/standard more discoverable as the package/repo will be published as a new, separate package.
On the "con" side, it would mean yet another repo to maintain.
As for this repo....
This repo already provides a coding standard for devs writing PHPCS sniffs, so a significant part of the expected public for these sniffs already uses
PHPCSDevanyhow, so it seems like a good fit.Why set this up as a separate standard instead of adding sniffs to the existing
PHPCSDevstandard ?To make it more straight-forward for packages which want to use the PHPCS specific sniffs, but not the PHPCSDev standard, to include just these sniffs.
If the sniffs would be added to the
PHPCSDevstandard, devs for an external standard would need to keep track of activity in this repo and would need to include each PHPCS-related sniff individually in their own project ruleset.... while, if the sniffs would be added to a new, separate, standard which only contains these sniffs, devs for packages which want to use these sniffs could just do:
... and will then automatically benefit from every new
PhpcsQAsniff being added, while they could also still use selective inclusions as per the previous example if so desired.Initial sniff idea list
PhpcsQA.UpgradeTo4.UseTokensConstants: a sniff to detect use of the deprecated[PHP_CodeSniffer\Util\]Tokens::$....token group variables and auto-fix these to use the PHPCS 4.xTokens::CONST_NAMEtoken group constants.File::findPrevious()andFile::findNext()with$typesset toT_WHITESPACEorTokens::EMPTY_TOKENSand$excludeset totrue, passnullfor the$endparameter.File::findPrevious()andFile::findNext()with$excludenot passed or set tofalse, do not passnullfor the$endparameter to prevent the "find" from walking to the start/end of the file.Another idea, though that probably doesn't belong in this standard and might be better to write as a generic code analysis sniff for PHPCSExtra: a sniff to check that most PHP native
array_*functions aren't used directly on deeply nested arrays.I.e. don't do:
array_pop($array['key1']['key2']), but assign$array['key1']['key2']to an interim variable ahead of thearray_pop()` function call.Additional actions needed
The checks and configs in PHPCSExtra can be used as inspiration/basis.