Open mmtr opened 4 years ago
cc @ntwb , @westonruter
Not really sure the history here as to whether it was an explicit choice. Based on some of the early pull requests (#617, #2914), I think there's been a pattern of gradual adoption of stricter rulesets.
I personally don't see any reason why we shouldn't consider adopting these additional rules.
Yeah, the additional sniffs in WordPress-Extra
help catch potential security issues. If the sniff marks code as a false positive, it can be suppressed with the phpcs:ignore
comment, naturally.
What Weston said šš¼
After a discussion with @anton-vlasenko regarding issue #44151, I suggested the exact same thing: start using the WordPress-Extra
ruleset, and in particular, make sure the PrefixAllGlobals
sniff is activated.
I've done an initial pass at updating the ruleset and fixing up issues which would start to get flagged - either straight away or in the near future via WPCS 3.0.0 -. The PRs I've pulled today are the result of that effort.
Even after those 11 PRs would be merged, there are still over 500 issues left to be fixed.
Some of the fixes may be simply a case of adding select excludes to the updated ruleset, either for a complete sniff or for a sniff in combination with certain files/directories.
In other cases, actual code improvements will need to be made and people more familiar with the Gutenberg code base than me, are better suited to make those fixes.
Please regard this as a call to action for people more familiar with the code base to start fixing these issues and/or to let me know about excludes which should be made.
If it helps, I can pull a draft PR with the proposed ruleset update already.
The below is a list of issues by error code which still need to be fixed/reviewed before the ruleset update is viable.
To see the underlying issues for any one of the sniffs, change <rule ref="WordPress-Core"/>
in the phpcs.xml.dist
file to <rule ref="WordPress-Extra"/>
, and then run PHPCS like so:
vendor/bin/phpcs --sniffs=Standard.Category.SniffName
... where Standard.Category.SniffName
is replaced with one of the sniffs mentioned in the below list.
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
--------------------------------------------------------------------------------------
SOURCE COUNT
--------------------------------------------------------------------------------------
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound 260
WordPress.Security.EscapeOutput.OutputNotEscaped 53
WordPress.Security.EscapeOutput.UnsafePrintingFunction 38
WordPress.Security.NonceVerification.Recommended 30
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound 26
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound 26
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound 20
WordPress.WP.GlobalVariablesOverride.Prohibited 11
WordPress.PHP.DevelopmentFunctions.error_log_trigger_error 9
WordPress.WP.EnqueuedResourceParameters.NotInFooter 9
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound 8
WordPress.Security.NonceVerification.Missing 6
WordPress.WP.AlternativeFunctions.json_encode_json_encode 6
Squiz.PHP.CommentedOutCode.Found 5
Generic.Files.OneObjectStructurePerFile.MultipleFound 4
PHPCompatibility.Syntax.RemovedPartiallySupportedCallables.DeprecatedStatic 4
WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize 4
WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents 4
WordPress.WP.AlternativeFunctions.strip_tags_strip_tags 4
WordPress.WP.EnqueuedResourceParameters.MissingVersion 3
Generic.CodeAnalysis.UnconditionalIfStatement.Found 2
Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed 1
Squiz.Operators.ValidLogicalOperators.NotAllowed 1
Squiz.PHP.DisallowSizeFunctionsInLoops.Found 1
Universal.Operators.DisallowLogicalAndOr.LogicalOr 1
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound 1
WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode 1
WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize 1
WordPress.PHP.DiscouragedPHPFunctions.system_calls_shell_exec 1
WordPress.PHP.DiscouragedPHPFunctions.urlencode_urlencode 1
WordPress.WP.AlternativeFunctions.file_system_read_fclose 1
WordPress.WP.AlternativeFunctions.file_system_read_fopen 1
WordPress.WP.AlternativeFunctions.file_system_read_readfile 1
WordPress.WP.AlternativeFunctions.parse_url_parse_url 1
WordPress.WP.AlternativeFunctions.rand_mt_rand 1
--------------------------------------------------------------------------------------
A TOTAL OF 546 SNIFF VIOLATIONS WERE FOUND IN 35 SOURCES
--------------------------------------------------------------------------------------
I've worked on fixing WordPress.NamingConventions.PrefixAllGlobals
linter errors.
I submitted a PR that fixes lots of the issues related to that.
There should be 0 WordPress.NamingConventions.PrefixAllGlobals.*
linter errors after adding the changes I'm listing below and merging my PR.
I propose to make the following changes to phpcs.xml.dist
:
_gutenberg
prefix to the list of allowed prefixes: https://github.com/WordPress/gutenberg/blob/4f350dcb4210cd84c9a758d2acdeebaada0b0d7c/phpcs.xml.dist#L83-L90Let's add these rules to phpcs.xml.dist:
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.ForbiddenPrefixPassed">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound">
<exclude-pattern>lib/compat/*</exclude-pattern>
<exclude-pattern>lib/experimental/*</exclude-pattern>
<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
<exclude-pattern>packages/*</exclude-pattern>
</rule>
Let me explain the changes a bit.
lib/compat
and lib/experimental
folders should be excluded as they contain Core-ready code. It should be allowed to have any prefixes/hook names there. The code in these folders is going to be merged to Core anyway.
However, functions/classes in these folders should be guarded. There must be another sniff in place to check that since WordPress.NamingConventions.PrefixAllGlobals
is not designed to do it. Please see https://github.com/WordPress/gutenberg/issues/44151 for more details.
Let's exclude the packages/
folder for now. This code is being used under specific circumstances. I feel like a broader discussion is needed to decide the correct approach here.
bin/generate-gutenberg-php.php
- this one should be refactored into a PHP class. Please see https://github.com/WordPress/gutenberg/issues/46016.
@jrfnl Would it be possible to add these phpcs.xml.dist
changes to your branch?
start using the WordPress-Extra ruleset
Not sure this is a good idea. Why should core code (Gutenberg is core) be using non-core rules/sniffs? This may even increase the complexity when merging the PHP code from GitHub to Trac.
make sure the PrefixAllGlobals sniff is activated.
Enabling only this rule seems sufficient to fix the problem. Still there seem to be false positives (that need exceptions that later need to be removed before merging to Trac).
Frankly I'm 50/50 on how useful PrefixAllGlobals
is at this point, but perhaps would be nice to have yet another safeguard that is applied to patches/PRs early.
Is there any reason why Gutenberg is not following the recommended best practices provided by the
WordPress-Extra
ruleset for phpcs?I enabled it on my local dev environment to check how many linting issues would it raise, and it seems it has a few, including some security issues like missing escaping and nonces.
See full log