Add PHPCS with custom bbPress ruleset

[netweb]

Add PHPCS with custom bbPress ruleset

To run PHPCS, run grunt phpcs

[netweb]

In 7006:

Build Tools: Add PHPCS with custom bbPress ruleset

• This changeset adds an initial bbPress PHPCS ruleset
• Future iteration of the rules in the ruleset can follow later
• To run the PHPCS check run `grunt phpcs

Fixes #3294.

[netweb]

In 7007:

Build Tools: Follow to PHPCS changes in [7006]

• Fixes an erronous change in Invision converter
• Tweaks a readability issue in bbp_update_reply()

See #3294.

[netweb]

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

[jrf]

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:

1. That file is in a subdirectory of the project root which would make running the tools fiddly.
2. 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:

1. The dev dependency setup.
2. 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.

[jrf]

Further iteration on the PHPCS setup

[jrf]

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-patterns. 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 the Core 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 the dist 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.

[jrf]

Replacement patch. Earlier patch was missing one change.

[jrf]

Fixes all violations against the WordPress.Classes.ClassInstantiation.MissingParenthesis error code

[jrf]

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%)

[jrf]

Fixes all remaining violations against the Generic.ControlStructures.InlineControlStructure.NotAllowed error code

[jrf]

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%)

[jrf]

Fixes all violations against the PSR2.Classes.PropertyDeclaration.ScopeMissing and the PSR2.Classes.PropertyDeclaration.VarUsed error codes

[jrf]

Fix accidental space indentation

[jrf]

Fix intentional space indentation.

[jrf]

Fixes most violations against the WordPress.WP.I18n.NonSingularStringLiteralText error code

[jrf]

Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged error code - part 1

[jrf]

Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged error code - part 2

[jrf]

Fixes most violations against the WordPress.PHP.NoSilencedErrors.Discouraged error code - part 3

[jrf]

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 to header() (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.

[jrf]

Just noticed I'd reversed the names of two patches:

• trac-3294-phpcs-fix-accidental-space-indents.patch​ contains the intentional space indents
• trac-3294-phpcs-fix-intentional-space-indents.patch contains the accidental space indents

[netweb]

In 7038:

Build Tools: Update svn:ignore and .gitignore

Props jrf.
See #3294.

[netweb]

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 the dist 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.

[netweb]

In 7039:

Build Tools: Add project root composer.json file.

This commit adds the following Composer package developer dependencies:

• phpcompatibility/phpcompatibility-wp
• wp-coding-standards/wpcs
• dealerdirect/phpcodesniffer-composer-installer

This also adds two composer scripts:

• composer lint to "lint" the PHP files using PHPCS
• composer format to "format" the PHP files using PHPCBF

Props jrf.
See #3294.

[netweb]

In 7045:

Build Tools: Update svn:ignore and .gitignore

• This commit is a follow up to [7038] to also ignore .phpcs.xml

Props jrf.
See #3294.

[netweb]

In 7046:

PHPCS: Use Parenthesis when instantiating a new object.

Props jrf.
See #3294.
For trunk.

[netweb]

In 7047:

PHPCS: Use Parenthesis when instantiating a new object.

Props jrf.
See #3294.
For branches/2.6.

[netweb]

In 7048:

PHPCS: Pinking shears.

Props jrf.
See #3294.
For trunk.

[netweb]

In 7049:

PHPCS: Pinking shears.

Props jrf.
See #3294.
For branches/2.6.

[netweb]

In 7050:

Build Tools: Improve PHPCS setup.

Props jrf.
See #3294.
For trunk.

[netweb]

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

[jrf]

FYI; the PHPCS Composer plugin has just released a new version, so pdating 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

[jrf]

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