Update PHPCS default rule set #161

NicktheGeek · 2018-07-20T12:58:15Z

The .phpcs.xml.dist rule file is detected and implemented automatically, reducing complexity for running phpcs.

The updated rules account for new wpcs sniff properties including the ability to check global prefix and i18n implementation with the correct text domain when specified.

The values have been left blank so the sniffs are generic, but the rules are in place to make it easier to get started by scaffold.

This is tangentially related to #109

This is also loosely related to #63 as updating composer scaffolds could work with this change.

@GaryJones may be able to provide additional feedback on this file change.

The .phpcs.xml.dist rule file is detected and implemented automatically, reducing complexity for running phpcs. The updated rules account for new wpcs sniff properties including the ability to check global prefix and i18n implementation with the correct text domain when specified. The values have been left blank so the sniffs are generic, but the rules are in place to make it easier to get started by scaffold.

schlessera · 2018-07-20T13:01:33Z

Yes, I would indeed love feedback from @GaryJones and @jrfnl to know whether this would be a good default starting point for new plugins.

schlessera · 2018-07-20T13:02:43Z

@NicktheGeek For the blanks, it would be helpful to add comments so that developers immediately know where to turn the dials.

NicktheGeek · 2018-07-20T13:21:13Z

@schlessera good idea.

NicktheGeek · 2018-07-20T13:25:16Z

@schlessera added the comments.

GaryJones · 2018-07-20T13:43:11Z

templates/.phpcs.xml.dist

+    <arg name="basepath" value="."/><!-- Strip the file paths down to the relevant bit -->
+    <arg name="colors" />
+    <arg name="extensions" value="php"/>
+    <arg name="report" value="full"/>


Remove the reports args - the default is a full report anyway, and any different reports should be decided upon at runtime.

FYI: I totally agree with @GaryJones' advice about the report.

Heh - the advice is what I took from Juliette!

GaryJones · 2018-07-20T13:44:07Z

templates/.phpcs.xml.dist

@@ -0,0 +1,44 @@
+<?xml version="1.0"?>
+<ruleset name="WordPress Coding Standards for Plugins">


This naming is a bit misleading - perhaps `"WordPress Coding Standards for this plugin"? is better, and less implies that there's a WPCS specifically for Plugins?

I'm pulling this name from the current version, but I am good to do this naming change.

May I suggest "WPCS based custom ruleset for plugins" or "WordPress Coding Standards based custom ruleset for your plugin/for plugin xyz" ?

Makes it very clear that this is a custom ruleset and not a standard. There is a significant difference between those two and naming this right will help people find the right information if they would search for more info.

To explain the difference:

A standard can add new sniffs, can be installed and referenced by name.

A custom ruleset uses existing standards and sniffs and may add configuration for these.

GaryJones · 2018-07-20T13:45:45Z

templates/.phpcs.xml.dist

+    <arg value="sp"/> <!-- Show sniff and progress -->
+    <arg name="basepath" value="."/><!-- Strip the file paths down to the relevant bit -->
+    <arg name="colors" />
+    <arg name="extensions" value="php"/>


Another potentially useful arg is <arg name="parallel" value="8"/> - the number can vary, but 8 is a nice starting point.

👍 Comment to add along-side it: 

GaryJones · 2018-07-20T13:46:03Z

templates/.phpcs.xml.dist

+    <!-- How to scan -->
+    <arg value="sp"/> <!-- Show sniff and progress -->
+    <arg name="basepath" value="."/><!-- Strip the file paths down to the relevant bit -->
+    <arg name="colors" />


Remove the space before the slash.

GaryJones · 2018-07-20T13:46:53Z

templates/.phpcs.xml.dist

+
+    <!-- How to scan -->
+    <arg value="sp"/> <!-- Show sniff and progress -->
+    <arg name="basepath" value="."/><!-- Strip the file paths down to the relevant bit -->


I know @jrfnl favours ./ here rather than ..

Either is fine as long as the [.]phpcs.xml.dist file is located in the project root.

GaryJones · 2018-07-20T13:47:56Z

templates/.phpcs.xml.dist

+
+	<!-- Rules: Check PHP version compatibility -->
+	<config name="testVersion" value="5.3-"/>
+	<rule ref="PHPCompatibility"/>


Since this is a WordPress plugin/theme, then it can reference the PHPCompatibilityWP ruleset instead - would need the composer.json template updated as well to pull that package in.

Since that is outstanding, should this remain as is and implement a change for it with #63?

I would most definitely recommend changing this already to use the PHPCompatibilityWP standard instead as it will prevent false positives for things WP back-fills/poly-fills.

GaryJones · 2018-07-20T13:48:56Z

templates/.phpcs.xml.dist

+	<rule ref="PHPCompatibility"/>
+
+    <!-- Rules: WordPress Coding Standards -->
+    <config name="minimum_supported_wp_version" value="3.3"/>


This seems particularly low. I personally use current - 2 versions (so, 4.7 right now), and WPCS default is current - 3 versions (so, 4.6 for WPCS 1.0.0).

GaryJones · 2018-07-20T13:49:39Z

templates/.phpcs.xml.dist

+    <!-- Rules: WordPress Coding Standards -->
+    <config name="minimum_supported_wp_version" value="3.3"/>
+    <rule ref="WordPress">
+        <exclude name="WordPress.VIP"/>


Might be worth waiting until WPCS 1.0.0 drops, which already removes WordPress-VIP from the WordPress ruleset.

This will work either way, even if the WPCS VIP deprecation would not make it into 1.0.0, so I'd leave it for now.

GaryJones · 2018-07-20T13:51:22Z

templates/.phpcs.xml.dist

+    <rule ref="WordPress.NamingConventions.PrefixAllGlobals">
+        <properties>
+            <!-- Value: provide the function, class, and variable prefixes used. Separate multiple prefixes with a comma. -->
+            <property name="prefixes" type="array" value=""/>


@jrfnl may know better, but having a my-plugin placeholder here may better point folks towards changing it to something useful when it gives some warnings on the first run, rather than an empty value which may either blow things up, or not show any related violations (and therefore not drawing attention to changing it) at all.

having a my-plugin placeholder here may better point folks towards changing it to something useful when it gives some warnings on the first run

I concur with @GaryJones, please do add a placeholder to give people incentive to change it.

may either blow things up, or not show any related violations

An empty value won't blow things up, but will basically disable the sniff.

GaryJones · 2018-07-20T13:52:54Z

templates/.phpcs.xml.dist

+    <rule ref="WordPress.WP.I18n">
+        <properties>
+            <!-- Value: provide the text domain used. -->
+            <property name="text_domain" type="array" value=""/>


I think WPCS 1.0.0 now supports a better way of adding in array properties - individual entries, instead of a comma-separated value.

Not WPCS, but PHPCS 3.3.0+. So if you would want to use the new array format, you would need to make sure that people are aware that they will need a very recent PHPCS version.

@jrfnl will the current CSV method work with PHPCS 3.3.0+? If so it seams like it might be better to wait before implementing that even by reference.

GaryJones · 2018-07-20T13:54:28Z

Rushing the review, and @jrfnl can fill in some blanks (or I can another day), but overall +1 for the PR to make it a much nicer starting point to implementing WPCS.

Thanks @NicktheGeek!

NicktheGeek · 2018-07-20T14:02:30Z

thanks you @GaryJones after you helped me get this on GFWA I've used it on several projects and I can't tell you the number of times it has found a minor typo in the text domain and saved me so much headache down the road. That one feature is worth the price of admission.

change the the ruleset name and add a comment explaining parellel processing.

NicktheGeek · 2018-07-20T18:04:02Z

@jrfnl Thanks! I've added the changes you recommended.

@schlessera this may need some additional feedback from @jrfnl or @GaryJones but so far I've implemented almost everything suggested. There are a couple of comments regarding WPCS 1.0.0 but IMO we are better if this is implemented now and an issue created to update when 1.0.0 comes out with additional improvements. I'm good either way though pending release timeline for WP CLI 2.0 and WPCS 1.0.0.

jrfnl

@schlessera Thanks for the ping. @NicktheGeek I've left some comments inline.

@jrfnl will the current CSV method work with PHPCS 3.3.0+? If so it seams like it might be better to wait before implementing that even by reference.

Yes, the current comma-delimited string format will still work, but it will be removed in PHPCS 4.0.

For more information about this change and the new format - much more readable! -, see the PHPCS 3.3.0 release notes: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.3.0

jrfnl · 2018-07-20T18:07:31Z

templates/.phpcs.xml.dist

+
+    <!-- What to scan -->
+    <file>.</file>
+    <exclude-pattern>vendor/</exclude-pattern>


Be aware that exclude-pattern values are interpreted as regular expressions, so this snippet would exclude every directory which included the string vendor/, i.e. /some-other-vendor/ would also be excluded.

While this may be something which will rarely cause issues, you can stabilize this to just the Composer vendor directory by using something along the lines of ^/vendor/* or the slightly more generic ./vendor/

For more information about how the exclude-patterns are interpreted, see: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-files-and-folders

Darn... my bad. The more generic version should be /vendor/. The . which I'd inadvertently placed in front of it will not be interpreted as root, but as a regex wildcard, so, while it shouldn't give any problems, it also isn't necessary.

jrfnl · 2018-07-20T18:10:44Z

templates/.phpcs.xml.dist

+    <file>.</file>
+    <exclude-pattern>vendor/</exclude-pattern>
+
+    <!-- How to scan -->


I would recommend adding a link here to the PHPCS wiki so people will know where to find more information about these options.

You can either use the generic wiki link: https://github.com/squizlabs/PHP_CodeSniffer/wiki

Or links to both these pages as good starting points for people:

Usage instructions: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Usage

Annotated ruleset: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml

jrfnl · 2018-07-20T18:11:08Z

templates/.phpcs.xml.dist

+    <arg name="extensions" value="php"/>
+    <arg name="parallel" value="8"/><!-- Enables parallel processing when available for faster results. -->
+
+	<!-- Rules: Check PHP version compatibility -->


Indentation is off here. Tabs vs spaces.

jrfnl · 2018-07-20T18:14:50Z

templates/.phpcs.xml.dist

+    <arg name="extensions" value="php"/>
+    <arg name="parallel" value="8"/><!-- Enables parallel processing when available for faster results. -->
+
+	<!-- Rules: Check PHP version compatibility -->


As per the earlier comments, using PHPCompatibilityWP will be the better choice.

I'd also recommend adding a link to the PHPCompatibilityWP repo as a comment, again to help people to more easily find out more info: https://github.com/PHPCompatibility/PHPCompatibilityWP

Maybe also add a link for more info about what the testVersion means and how it works: https://github.com/PHPCompatibility/PHPCompatibility#sniffing-your-code-for-compatibility-with-specific-php-versions

You may also want to add a comment saying that people can change the testVersion to suit their project.

jrfnl · 2018-07-20T18:21:52Z

templates/.phpcs.xml.dist

+	<config name="testVersion" value="5.3-"/>
+	<rule ref="PHPCompatibility"/>
+
+    <!-- Rules: WordPress Coding Standards -->


Along the same lines as some of my other comments, you may want to add a link here to the WPCS repo and/or to the WPCS wiki page about configuring sniffs:

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties

jrfnl · 2018-07-20T18:24:38Z

templates/.phpcs.xml.dist

+            <property name="text_domain" type="array" value="my-plugin"/>
+        </properties>
+    </rule>
+    <rule ref="WordPress.WhiteSpace.ControlStructureSpacing">


This is more a personal preference, than a recommended setting. I'm not sure if it belong in a scaffold.

jrfnl · 2018-07-20T18:26:22Z

templates/.phpcs.xml.dist

+        <properties>
+            <property name="blank_line_check" value="true"/>
+        </properties>
+    </rule>


I haven't looked at the scaffold in detail, but in case it also adds a PHPUnit test set-up, you may want to add an additional set of custom properties for that: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#custom-unit-test-classes

jrfnl · 2018-07-20T18:27:57Z

templates/.phpcs.xml.dist

+    <!-- How to scan -->
+    <arg value="sp"/> <!-- Show sniff and progress -->
+    <arg name="basepath" value="./"/><!-- Strip the file paths down to the relevant bit -->
+    <arg name="colors"/>


Just a side-note: colors will only work on certain platforms. Not sure by heart, but IIRC it works on Linux and Mac, but not on Windows.

jrfnl · 2018-07-20T18:33:30Z

Forgot to respond to this:

There are a couple of comments regarding WPCS 1.0.0 but IMO we are better if this is implemented now and an issue created to update when 1.0.0 comes out with additional improvements.

WPCS 1.0.0 was due to come out last week, but there's still one issue which needs to be fixed. Expect it very soon though.

All the same, for the ruleset as it is, AFAICS, no changes would be needed for WPCS 1.0.0, so you're good.

also converts spaces to tabs

GaryJones · 2018-07-20T19:16:07Z

The failure looks like some behat tests need updating to reference .phpcs.xml.dist (leading dot) instead of phpcs.xml.dist.

NicktheGeek · 2018-07-20T19:48:09Z

@GaryJones @jrfnl will it be ok for this to not have the leading .? I think it is required to be automatically used but I want to make sure I'm understanding the reason.

jrfnl · 2018-07-20T20:06:00Z

@GaryJones @jrfnl will it be ok for this to not have the leading .? I think it is required to be automatically used but I want to make sure I'm understanding the reason.

Do you mean for the actual file name phpcs.xml.dist ? (in contrast to my remark about the exclude pattern #161 (comment) )

Yes, no problem.

Allowing a leading . for the filename was introduced in PHPCS 3.1.0 with a file loading order change in 3.1.1.

So for the custom ruleset with a leading dot to work, the minimum PHPCS requirement would have to be set at 3.1.0.

The file loading order (as of PHPCS 3.1.1) is:

.phpcs.xml
phpcs.xml
.phpcs.xml.dist
phpcs.xml.dist

In other words, not having the leading . now allows three different files to overrule the file.

Refs:

GaryJones · 2018-07-20T20:08:36Z

will it be ok for this to not have the leading .?

And beyond what @jrfnl just said, whether it's right for PHPCS 3.1[.1] to be the minimum version for scaffolded plugins is a different matter, though for new plugins I don't see much reason why the default shouldn't be 3.3.

NicktheGeek · 2018-07-20T20:10:30Z

I went ahed and updated the tests to solve for the file ext change

jrfnl · 2018-07-20T20:14:52Z

templates/plugin-distignore.mustache

@@ -21,7 +21,7 @@ phpunit.xml.dist
 multisite.xml
 multisite.xml.dist
 phpcs.xml


You may also want to add .phpcs.xml here and possibly to the .gitignore file too ?

@jrfnl thanks! I added .phpcs.xml but .gitignore is already in the file higher up.

jrfnl · 2018-07-29T15:02:09Z

FYI: in the mean time, both PHPCS 3.3.1 as well as WPCS 1.0.0 have been released.

schlessera · 2018-07-29T15:02:17Z

Thanks to @NicktheGeek for the pull request, and thanks to @GaryJones & @jrfnl for the extensive review!

change to .phpcs.xml.dist and update rules

schlessera added the command:scaffold-plugin label Jul 20, 2018

add comments for blank values

e4a3d1b

GaryJones suggested changes Jul 20, 2018

View reviewed changes

Nick Croft added 2 commits July 20, 2018 10:16

updates per pr feedback

b64faae

update based on more PR feedback

d1047f2

change the the ruleset name and add a comment explaining parellel processing.

add placeholder for prefix and textdomain rules

c449067

jrfnl reviewed Jul 20, 2018

View reviewed changes

jrfnl mentioned this pull request Jul 20, 2018

Add template/composer.json #63

Closed

add comments and update rule ref

4160c12

also converts spaces to tabs

remove leading .

d0ac996

update tests and distignore for .phpcs.xml.dist

2f23c5c

jrfnl reviewed Jul 20, 2018

View reviewed changes

Nick Croft added 2 commits July 20, 2018 16:20

update to handle additional phpcs rule files

c22db9a

update test for verifying the ruleset contents

64920e5

schlessera approved these changes Jul 29, 2018

View reviewed changes

schlessera added this to the 1.1.4 milestone Jul 29, 2018

schlessera merged commit a897a54 into wp-cli:master Jul 29, 2018

schlessera changed the title ~~change to .phpcs.xml.dist and update rules~~ Update PHPCS default rule set Jul 29, 2018

jrfnl mentioned this pull request Aug 14, 2018

Add WordPress exclusions to PHPCS compatibility sniffer #155

Closed

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022

Merge pull request #161 from NicktheGeek/update-phpcs-rules

c740acb

change to .phpcs.xml.dist and update rules

		@@ -0,0 +1,44 @@
		<?xml version="1.0"?>
		<ruleset name="WordPress Coding Standards for Plugins">

Update PHPCS default rule set #161

Update PHPCS default rule set #161

Conversation

NicktheGeek commented Jul 20, 2018

schlessera commented Jul 20, 2018

schlessera commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaryJones commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 20, 2018

GaryJones commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

jrfnl commented Jul 20, 2018

GaryJones commented Jul 20, 2018

NicktheGeek commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 29, 2018

schlessera commented Jul 29, 2018

jrfnl Jul 20, 2018 •

edited

Loading

jrfnl Jul 20, 2018 •

edited

Loading