PHPCS fixes for 1.9.1-dev #486
Conversation
|
Two lines generate escaping warnings:
Please inspect them and choose an appropriate escaping function, or add a |
phpcs.xml
Outdated
| @@ -0,0 +1,14 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
This should be named .phpcs.xml.dist or phpcs.xml.dist.
There was a problem hiding this comment.
I based this on the work done at https://github.com/Automattic/amp-wp/ cc @westonruter
There was a problem hiding this comment.
@westonruter should know better :-)
I've created ampproject/amp-wp#1324 for them to update it at AMP.
There was a problem hiding this comment.
Renamed with 5767fb0.
phpcs.xml
Outdated
| @@ -0,0 +1,14 @@ | |||
| <?xml version="1.0"?> | |||
| <ruleset name="WordPress Coding Standards for Liveblog"> | |||
There was a problem hiding this comment.
Name should be "Custom ruleset for Liveblog". It's a ruleset, not a standard.
There was a problem hiding this comment.
Also copied from amp-wp.
There was a problem hiding this comment.
This is just about being accurate and educating developers.
For your reference (since I see you on many different repos doing PRs improving PHPCS setups like this - and thank you for that!), here's my plugin boilerplate .phpcs.xml.dist file, which applies many good practices.
There was a problem hiding this comment.
Fixed with 14a4a9e.
phpcs.xml
Outdated
| <ruleset name="WordPress Coding Standards for Liveblog"> | ||
|
|
||
| <rule ref="WordPress-VIP" /> | ||
| <rule ref="WordPress-Extra" /> |
There was a problem hiding this comment.
There should be no spaces before closing />. This is XML, not XHTML.
There was a problem hiding this comment.
Also copied from amp-wp.
There was a problem hiding this comment.
Fixed with 0ca4449.
phpcs.xml
Outdated
| <rule ref="WordPress-VIP" /> | ||
| <rule ref="WordPress-Extra" /> | ||
|
|
||
| <arg value="s"/> |
There was a problem hiding this comment.
Include the progress (p), colors, extensions, basepath and parallel args - this plugin is PHP 5.6 and above, so PHPCS 3.x will be installed (3.3.1 currently).
There was a problem hiding this comment.
I was previously told not to use --colors:
WordPress/gutenberg#1685 (comment)
There was a problem hiding this comment.
extensions is already present.
There was a problem hiding this comment.
What do you want for basepath and parallel? @westonruter any thoughts here?
There was a problem hiding this comment.
Fixed based on your boilerplate. 15df6b2.
There was a problem hiding this comment.
basepath="." is enough to remove everything before the current directory.
parallel="8" is a good default, though I put it to 50 on my plugins - I've not yet encountered a drawback, though that doesn't mean there isn't one.
phpcs.xml.dist
Outdated
| @@ -1,8 +1,9 @@ | |||
| <?xml version="1.0"?> | |||
| <ruleset name="Custom ruleset for Liveblog"> | |||
|
|
|||
| <rule ref="WordPress-VIP"/> | |||
| <rule ref="WordPress-Core"/> | |||
There was a problem hiding this comment.
No need to include WordPress-Core if including WordPress-Extra, as Extra already includes Core.
| @@ -1625,11 +1630,11 @@ private static function status_header_with_message( $status, $message ) { | |||
|
|
|||
| $status = absint( $status ); | |||
| $official_message = isset( $wp_header_to_desc[ $status ] ) ? $wp_header_to_desc[ $status ] : ''; | |||
| $wp_header_to_desc[ $status ] = self::sanitize_http_header( $message ); | |||
| $wp_header_to_desc[ $status ] = self::sanitize_http_header( $message ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.OverrideProhibited | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| <?xml version="1.0"?> | ||
| <ruleset name="Custom ruleset for Liveblog"> | ||
|
|
||
| <rule ref="WordPress-Extra"/> |
There was a problem hiding this comment.
Also add WordPress-Docs? It's not fully complete, but it's a good start for what it does cover. Could leave for a separate PR.
There was a problem hiding this comment.
Yep, let's do a separate issue outside of this PR 👍
| <arg name="parallel" value="50"/> | ||
| <arg name="extensions" value="php"/> | ||
| <file>.</file> | ||
|
|
There was a problem hiding this comment.
Would like to see text domain checks enabled, and prefixes, and some link references.
See https://github.com/GaryJones/plugin-boilerplate/blob/master/.phpcs.xml.dist, and consider copying this, and amending it with the liveblog bits (no need to include Neutron).
PHPCompatibilityWP standard would also then need to be added to composer.json and Travis config.
There was a problem hiding this comment.
Let's leave this for a separate change
There was a problem hiding this comment.
A few small bits to address, then I think this would be good to get merged, re-adjust the phpcs.xml.dist as necessary, and then re-scan and re-fix.
Great job @paulschreiber!
|
@philipjohn I've addressed the issues @GaryJones flagged. |
No description provided.