-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coding standards: Use WordPress-Extra ruleset to prevent potential security issues #18502
Comments
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 |
What Weston said 👆🏼 |
After a discussion with @anton-vlasenko regarding issue #44151, I suggested the exact same thing: start using the 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 -. 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 vendor/bin/phpcs --sniffs=Standard.Category.SniffName ... where
|
I've worked on fixing
<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. However, functions/classes in these folders should be guarded. There must be another sniff in place to check that since Let's exclude the
@jrfnl Would it be possible to add these |
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.
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 |
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
The text was updated successfully, but these errors were encountered: