Opened 5 years ago
Last modified 4 years ago
#3294 reopened task (blessed)
Add PHPCS with custom bbPress ruleset
Reported by: | netweb | Owned by: | netweb |
---|---|---|---|
Milestone: | 2.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Tools - Build | Keywords: | has-patch |
Cc: |
Description
Add PHPCS with custom bbPress ruleset
To run PHPCS, run grunt phpcs
Attachments (11)
Change History (32)
#3
@
5 years ago
Reference, I've currently got 3 groups of sniffs in the phpcs.xml.dist
file, here are each of the groups testing results displayed for reference if those rules were enabled:
- <!-- Rules that require no further review -->
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY ------------------------------------------------------------------------------- SOURCE COUNT ------------------------------------------------------------------------------- [x] Generic.WhiteSpace.ScopeIndent.Incorrect 4765 [x] Generic.WhiteSpace.ScopeIndent.IncorrectExact 3757 [x] WordPress.Arrays.CommaAfterArrayItem.NoComma 2064 [x] Generic.Functions.FunctionCallArgumentSpacing.TooMuchSpaceAfterComma 1228 [x] Generic.Formatting.MultipleStatementAlignment.NotSameWarning 209 [x] Generic.Formatting.MultipleStatementAlignment.IncorrectWarning 87 [x] Squiz.PHP.EmbeddedPhp.ContentBeforeEnd 80 [x] Squiz.PHP.EmbeddedPhp.ContentBeforeOpen 38 [ ] PEAR.NamingConventions.ValidClassName.StartWithCapital 6 ------------------------------------------------------------------------------- A TOTAL OF 12234 SNIFF VIOLATIONS WERE FOUND IN 9 SOURCES ------------------------------------------------------------------------------- PHPCBF CAN FIX THE 8 MARKED SOURCES AUTOMATICALLY (12228 VIOLATIONS IN TOTAL) -------------------------------------------------------------------------------
- <!-- Rules that require further review, partially reviewed -->
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY ----------------------------------------------------------------------------------- SOURCE COUNT ----------------------------------------------------------------------------------- [x] PEAR.Functions.FunctionCallSignature.Indent 20 [x] Generic.Formatting.DisallowMultipleStatements.SameLine 19 [x] Generic.WhiteSpace.ArbitraryParenthesesSpacing.SpaceBeforeClose 19 [x] Generic.ControlStructures.InlineControlStructure.NotAllowed 7 [x] Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace 7 [x] Generic.Formatting.SpaceAfterCast.TooMuchSpace 5 [x] PEAR.Functions.FunctionCallSignature.EmptyLine 3 [x] PSR2.ControlStructures.SwitchDeclaration.BreakIndent 3 ----------------------------------------------------------------------------------- A TOTAL OF 83 SNIFF VIOLATIONS WERE FOUND IN 8 SOURCES ----------------------------------------------------------------------------------- PHPCBF CAN FIX THE 8 MARKED SOURCES AUTOMATICALLY (83 VIOLATIONS IN TOTAL) -----------------------------------------------------------------------------------
- <!-- Rules that require further review, not yet reviewed -->
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY -------------------------------------------------------------------------------------- SOURCE COUNT -------------------------------------------------------------------------------------- [x] WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned 1074 [x] PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket 448 [x] PEAR.Functions.FunctionCallSignature.CloseBracketLine 436 [x] PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket 428 [x] PSR2.ControlStructures.SwitchDeclaration.SpaceBeforeColonCASE 383 [x] PEAR.Functions.FunctionCallSignature.MultipleArguments 353 [ ] WordPress.WP.I18n.MissingTranslatorsComment 275 [x] Squiz.PHP.EmbeddedPhp.ContentAfterOpen 108 [x] Squiz.Strings.ConcatenationSpacing.PaddingFound 106 [x] PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket 103 [x] WordPress.Arrays.ArrayDeclarationSpacing.SpaceBeforeArrayCloser 102 [x] WordPress.Arrays.ArrayDeclarationSpacing.AssociativeArrayFound 94 [x] PSR2.ControlStructures.SwitchDeclaration.SpaceBeforeColonDEFAULT 89 [ ] WordPress.PHP.YodaConditions.NotYoda 85 [ ] WordPress.DB.PreparedSQL.InterpolatedNotPrepared 82 [ ] Squiz.PHP.DisallowMultipleAssignments.Found 76 [ ] WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase 58 [x] PSR2.ControlStructures.SwitchDeclaration.BodyOnNextLineCASE 56 [x] WordPress.WhiteSpace.DisallowInlineTabs.NonIndentTabsUsed 51 [x] WordPress.Arrays.ArrayIndentation.ItemNotAligned 48 [ ] WordPress.Files.FileName.InvalidClassFileName 42 [x] PEAR.Functions.FunctionCallSignature.SpaceBeforeOpenBracket 40 [ ] WordPress.PHP.StrictComparisons.LooseComparison 40 [x] WordPress.WhiteSpace.OperatorSpacing.SpacingBefore 36 [x] Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace 33 [x] WordPress.Arrays.CommaAfterArrayItem.SpaceAfterComma 28 [x] WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore 28 [ ] Squiz.PHP.DisallowMultipleAssignments.FoundInControlStructure 27 [ ] WordPress.CodeAnalysis.AssignmentInCondition.Found 27 [x] WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter 27 [x] Squiz.PHP.EmbeddedPhp.ContentAfterEnd 26 [ ] WordPress.Files.FileName.NotHyphenatedLowercase 23 [x] WordPress.Arrays.ArrayKeySpacingRestrictions.SpacesAroundArrayKeys 21 [ ] WordPress.Arrays.ArrayIndentation.CloseBraceNotAligned 19 [ ] WordPress.WhiteSpace.PrecisionAlignment.Found 19 [ ] Squiz.PHP.EmbeddedPhp.MultipleStatements 18 [ ] WordPress.DB.PreparedSQL.NotPrepared 11 [x] WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine 10 [x] WordPress.Arrays.ArrayDeclarationSpacing.CloseBraceNewLine 10 [x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeCloseParenthesis 10 [ ] WordPress.PHP.StrictInArray.MissingTrueStrict 8 [ ] WordPress.PHP.NoSilencedErrors.Discouraged 7 [x] WordPress.Arrays.ArrayIndentation.MultiLineArrayItemNotAligned 6 [ ] WordPress.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition 6 [x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterOpenParenthesis 6 [x] PSR2.ControlStructures.SwitchDeclaration.BodyOnNextLineDEFAULT 4 [x] Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterOpen 4 [x] WordPress.Arrays.ArrayKeySpacingRestrictions.NoSpacesAroundArrayKeys 4 [ ] WordPress.WP.I18n.NonSingularStringLiteralText 4 [x] WordPress.WhiteSpace.OperatorSpacing.SpacingAfter 4 [x] Squiz.ControlStructures.ControlSignature.SpaceAfterCloseBrace 2 [x] WordPress.WhiteSpace.ControlStructureSpacing.ExtraSpaceAfterOpenParenthesis 2 [x] Squiz.PHP.EmbeddedPhp.SpacingAfterOpen 1 [x] WordPress.Arrays.MultipleStatementAlignment.SpaceBeforeDoubleArrow 1 [ ] WordPress.WP.I18n.LowLevelTranslationFunction 1 -------------------------------------------------------------------------------------- A TOTAL OF 5010 SNIFF VIOLATIONS WERE FOUND IN 55 SOURCES -------------------------------------------------------------------------------------- PHPCBF CAN FIX THE 36 MARKED SOURCES AUTOMATICALLY (4191 VIOLATIONS IN TOTAL) --------------------------------------------------------------------------------------
These can reviewed further and iterated on at a later time, some of the rules with few violations could be ignored using phpcs:ignore
directives
#4
@
5 years ago
- Keywords has-patch added
- Resolution fixed deleted
- Status changed from closed to reopened
As promised, I've had a look at the setup.
While I have questions about the current exclusions and such, that can be dealt with later and bit by bit.
For now, I've initially just focused on the dev-usability, ruleset and documentation of the setup.
Choice of WPCS ruleset:
At this moment, the ruleset is set to WordPress-Core
.
For plugins, I'd strongly recommend using either WordPress-Extra
or WordPress
.
The WordPress-Core
ruleset is heavy on sniffs which focus on code-style and light on best practices.
In contrast, the WordPress-Extra
contains lots of additional checks to safeguard against problems with interoperability with other plugins/themes and WP itself, more security sniffs, sniffs checking against common errors, as well as other best practices.
If I run these rulesets now over the codebase (without the detailed rule based exclusions, but with the proposed configuration), the difference in the amount of issues found isn't that huge anyway, so I suggest starting with Extra
:
Core:
Errors: 15776 in 159 files in 70 flavours Warnings: 1029 in 17 flavours
Extra:
Extra: Errors: 16801 in 162 files in 82 flavours Warnings: 1100 in 37 flavours
Running against WordPress
does give a significant amount of extra issues. This is largely due to the Docs
sniffs being included. As that ruleset is currently not that great and incomplete anyway, let's leave that for later when the ruleset has been improved.
WordPress:
Errors: 27824 in 190 files in 114 flavours Warnings: 1136 in 41 flavours
PHPCompatibility
I'd strongly recommend using the PHPCompatibilityWP ruleset as well to safeguard against introduction of PHP cross-version compatibility issues.
An initial run with that throws up 11 issues.
- 3 are confirmed issues for compatibility with PHP 7.4 which should be fixed ASAP
- 8 will need review of the code (which I will do in a bit).
Configuration
There are a number of WPCS sniffs which greatly benefit from having some configuration set. I'm talking about, for instance, the I18n sniff knowing which text-domain
should be used or the sniffs checking for use of deprecated WP features knowing what the minimum supported WP version is.
Also see: https://github.com/WordPress/WordPress-Coding-Standards/wiki/Customizable-sniff-properties
Grunt vs Composer
The current setup to run via grunt
presumes every dev already has PHPCS installed locally and in their OS path etc.
In my personal opinion, it is illogical to run a tool which is not set up via grunt
via grunt
as this makes too many assumptions and doesn't ensure in any way that devs will be able to get the same results as there is no management of the package versions used.
I'd recommend switching over to using Composer for this.
Now I noticed there is a composer.json
file in the src
directory, but it doesn't seem right to use that, as:
- That file is in a subdirectory of the project root which would make running the tools fiddly.
- That file doesn't contain any
dev
requirements anyhow and doesn't seem set up for dev use, just build use.
So, I'm proposing to add a cursory composer.json
config to the project root. This file would contain:
- The
dev
dependency setup. - Scripts for devs to use.
Running PHPCS will then be as easy as running:
composer install composer check-cs
Note: the svn-ignore
config needs to be updated when this setup is committed to ignore the vendor
directory as well as the composer.lock
file in the project root.
#5
follow-up:
↓ 11
@
5 years ago
Some notes regarding the patch:
composer.json
As described above, this is a new Composer setup in the project root just containing the dev
requirements and convenience scripts.
phpcs.xml.dist
- Added the PHPCS schema annotation.
- Added some additional inline documentation to the ruleset, including dividing it up in sections for easier understanding of what is what.
- Removed the
v
from<arg value="psv"/>
. This causes a list of all scanned files to be shown and is normally not necessary except when debugging the ruleset. - Changed the basic ruleset to
WordPress-Extra
. - Added the
PHPCompatibilityWP
ruleset to scan against. - Added configuration for select sniffs. Additional configuration may still be needed and can be added later.
- Stabilized
exclude-pattern
s. These are a special flavour of PCRE regexes and characters which have special meaning in regex should be escaped to prevent accidentally matching other files. - Added exclude patterns for the additional errors thrown when using the
Extra
ruleset instead of theCore
ruleset. These should all be reviewed.
Gruntfile.js
- PHPCS 3.x allows for a shorter form of setting the requested reports.
- Not hard-coding the
--standard
allows for individual devs to overload thedist
file with their own[.]phpcs.xml
file with additional/personalized settings.
.gitignore
Aside from the typical Composer related ignores, now also ignoring the typical files which devs can use to overload the phpcs.xml.dist
file.
@
5 years ago
Fixes all violations against the WordPress.Classes.ClassInstantiation.MissingParenthesis
error code
#6
@
5 years ago
The majority of object instantiations in bbPress use parenthesis, so fixing the violations against the WordPress.Classes.ClassInstantiation.MissingParenthesis
error code makes the code base more consistent.
Object instantiation with parenthesis: yes => 68 ( 89.47%) no => 8 ( 10.53%) ---------------------- total => 76 (100.00%)
@
5 years ago
Fixes all remaining violations against the Generic.ControlStructures.InlineControlStructure.NotAllowed
error code
#7
@
5 years ago
The vast majority of control structures are declared with curly braces in bbPress, so fixing the remaining violations against the Generic.ControlStructures.InlineControlStructure.NotAllowed
error code makes the code base more consistent.
Control structure defined inline: no => 3,861 ( 99.82%) yes => 7 ( 0.18%) ------------------------- total => 3,868 (100.00%)
@
5 years ago
Fixes all violations against the PSR2.Classes.PropertyDeclaration.ScopeMissing
and the PSR2.Classes.PropertyDeclaration.VarUsed
error codes
@
5 years ago
Fixes most violations against the WordPress.WP.I18n.NonSingularStringLiteralText
error code
@
5 years ago
Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged
error code - part 1
@
5 years ago
Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged
error code - part 2
@
5 years ago
Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged
error code - part 3
#8
@
5 years ago
Regarding the violations against the WordPress.PHP.NoSilencedErrors.Discouraged
error code.
- Patch 1 removes the silence operator from some functions which don't throw PHP warnings/notices. I.e. these were redundant.
- Patch 2 adds a
if ( ! headers_sent() ) {}
wrapper around calls toheader()
(and removes the silencing from those calls as it won't be needed anymore). - Patch 3 allows one use of the silence operator in combination with
include_once
. The functions needed to check whether the file exists before doing the include would also throw warnings which would need to be silenced if the file doesn't exist, so leaving this as is.
Additionally, I'd recommend adding the following snippet to the PHPCS config file:
<rule ref="WordPress.PHP.NoSilencedErrors"> <properties> <property name="use_default_whitelist" value="true"/> </properties> </rule>
The combination of the uploaded patches and the additional config gets rid of all violations against the WordPress.PHP.NoSilencedErrors.Discouraged
error code.
#9
@
5 years ago
Just noticed I'd reversed the names of two patches:
trac-3294-phpcs-fix-accidental-space-indents.patch
contains the intentional space indentstrac-3294-phpcs-fix-intentional-space-indents.patch
contains the accidental space indents
#11
in reply to:
↑ 5
@
5 years ago
Replying to jrf:
Gruntfile.js
- PHPCS 3.x allows for a shorter form of setting the requested reports.
- Not hard-coding the
--standard
allows for individual devs to overload thedist
file with their own[.]phpcs.xml
file with additional/personalized settings.
The shorter form throws an error for Grunt, most likely the way Grunt passes those args to phpcs
, for now I'll use the longer syntax:
Short Syntax: args: [ '--report-summary,source', '--cache=.phpcscache' ]
Running "phpcs" task [D] Task source: /Users/netweb/Code/bbPress/bbpress-svn/trunk/Gruntfile.js Running "phpcs:default" (phpcs) task [D] Task source: /Users/netweb/Code/bbPress/bbpress-svn/trunk/Gruntfile.js Verifying property phpcs.default exists in config...OK File: [no files] ERROR: Class file for report "summary,source" not found Warning: Use --force to continue. Aborted due to warnings.
Long Syntax: args: [ '--report-summary', '--report-source', '--cache=.phpcscache' ]
Running "phpcs:default" (phpcs) task [D] Task source: /Users/netweb/Code/bbPress/bbpress-svn/trunk/Gruntfile.js Verifying property phpcs.default exists in config...OK File: [no files] EEEEEEEEW.E.......E 19 / 19 (100%)
Going to drop Grunt altogether in the near future and switch to npm
scripts following Core's lead on this, so not a priority to investigate and spend time on further.
#19
@
5 years ago
My weekend has come to an end, alas, for now there are no warnings or errors when running composer lint
, as any that were there I've added to the exclusions in phpcs.xml.dist
Will continue to review, revisit, and prioritize these in the comings days, weeks, etc
#20
@
5 years ago
FYI; the PHPCS Composer plugin has just released a new version, so updating it may be a good idea As Composer treats minors < 1.0 as majors, this requires an update in the composer.json
file to ^0.6
Release log: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/releases/tag/v0.6.0
#21
@
4 years ago
FYI; the PHPCS Composer plugin has just released a new version, which includes compatibility with Composer 2.0 (expected soon), so updating it may be a good idea.
As Composer treats minors < 1.0 as majors, this requires an update in the composer.json file to 0.7
Release log: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/releases/tag/v0.7.0
In 7006: