PHPCS: Add use statement sorting sniff
#17361
Conversation
In starting to add another sniff, I noticed several inconsistencies in the configuration of the current MasterUserConstant sniff. * We seem to want our phpcs standard to be named "Jetpack", but phpcs was calling it "PHPCSSniffs". * The MasterUserConstant sniff produces warnings with ID "..Jetpack_Sniffs_MasterUserConstant.ShouldNotBeUsed", rather than "Jetpack.Something.MasterUserConstant.ShouldNotBeUsed" like all other sniffs. * MasterUserConstantSniff.php is using old phpcs 2.x aliases for the phpcs classes, which don't get loaded if you use phpcs --standard to run only Jetpack's sniffs. This patch fixes the above with the following changes: * As phpcs seems to want the standard to be in an eponymous directory, move bin/PHPCSSniffs to bin/PHPCSStandards/Jetpack. * Rename Jetpack_Sniffs_MasterUserConstantSniff to Jetpack\Sniffs\Constants\MasterUserConstantSniff so the ID is determined correctly. * Update MasterUserConstantSniff.php to use the phpcs 3.x class names. * Move MasterUserConstantSniff.php to a PSR-4 style directory structure, since even the WordPress phpcs standards do that.
After a merge of two patches adding `use` statements for the same class in different places in the existing list broke things, discussion on Slack was in favor of adding a sniff to require that `use` statements are in alphabetical order. As https://github.com/wikimedia/mediawiki-tools-codesniffer already has just such a sniff, we'll copy theirs and modify it to suit our purposes. This commit copies it with minimal changes for compatibility with PHP 5.6 (as detected by `composer php:compatibility`), with the changes to make it pass all our coding conventions to be done as a followup because I want to see just how many changes that requires.
…entions 1 file changed, 103 insertions(+), 85 deletions(-)
anomiex
added
[Status] Needs Review
|
An alternative might be to just pull in mediawiki/mediawiki-codesniffer via composer and enable just the one sniff, instead of copying the code. But the MediaWiki version uses features that were introduced in PHP 7.1, does that prevent it? Also if WordPress wants to include something like this in wp-coding-standards/wpcs, which IMO they probably should, my guess is they'd want a copy anyway instead of pulling in all MediaWiki's rules to get just the one test. |
jeherve
left a comment
jeherve
left a comment
There was a problem hiding this comment.
This tests well for me. I am thinking we could merge this into master, and then in a follow-up PR, once we've updated all files, elevate the warning into an error?
jeherve
added
[Status] Needs Author Reply
There's no point in scanning the copies of WordPress or WordPress-develop checked out for Docker, or the built files in _inc that are already listed in .gitignore. Also one file being .gitignored is no longer used, so let's remove it from there.
anomiex
force-pushed
the
add/phpcs-use-statement-sorting-sniff
branch
from
63398cb to
b4c13a9
Compare
anomiex
added
[Status] Blocked / Hold
and removed
[Status] Needs Author Reply
|
I'm going to put this on hold for the moment, while we take a more holistic look at MediaWiki sniffs we might want to import in p9dueE-1Y9-p2. |
E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17361 This is an automated check which relies on |
|
Doing #17406 instead. |
Changes proposed in this Pull Request:
After a merge of two patches adding
usestatements for the same class in different places in the existing list broke things, discussion on Slack was in favor of adding a sniff to require thatusestatements are in alphabetical order.As https://github.com/wikimedia/mediawiki-tools-codesniffer already has just such a sniff, we'll copy theirs and modify it to suit our purposes.
This also adjusts .phpcs.xml.dist to ignore more files that aren't actually part of the repo, in preparation for the followup (#17362) that mass-fixes all the new sniff failures this patch introduces.
Jetpack product discussion
p9dueE-1Y9-p2 (and some earlier Slack discussion)
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
uselist.composer php:lint file.phpshould detect it.composer php:autofix file.phpshould fix it.Proposed changelog entry for your changes: