Fix PHPCS configuration

[azaozz]

Some current rules used in PHPCS don't fully match the WordPress PHP Coding Standards or recent developments/changes. Many of them can be fixed by changing core's phpcs.xml.dist.

In addition, optional rules can be set to lower priority so they are only displayed when needed.

[pento]

@azaozz: Which rules in particular are you referring to?

[azaozz]

Replying to pento:

Was thinking WordPress.PHP.YodaConditions.NotYoda needs to be disabled. Then WordPress.Arrays.ArrayKeySpacingRestrictions.SpacesAroundArrayKeys should allow for spaces around all array keys as in the JS standards (the decision there was to match PHP and JS, if I remember right).

Another is to perhaps replace WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned with Generic.Formatting.MultipleStatementAlignment as the latter allows setting of maximum spaces to insert. That will help to keep aligned associative arrays easier to read and match ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation better (we'll need to add a setting for it though).

Alternatively we can set the priority to lower than 5. That would make the rule(s) "optional", i.e. they will still be available from the cli but won't prevent building or trigger errors in Travis.

I'll make a patch so we can test and/or have a better look :)

[netweb]

Sounds good to me 👍

[azaozz]

In 46869.diff: Add few config options to phpcs.xml.dist to account for recent changes and format some associative arrays to be more easily readable.

This lowers the severity of few rules to 4 or less, which in practice makes them optional. They can be used by setting warning-severity=4 or error-severity=4 (or both) from the cli.

[azaozz]

In 46869.1.diff:

• Set maxPadding for Generic.Formatting.MultipleStatementAlignment. That works well for preventing too many spaces when aligning =.
• Do not align => across different dimensions of multidimensional associative arrays. This fixes the edge case of having the parent array's keys aligned across different blocks of code.

The last bit, setting maxColumn for WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned doesn't seem to do anything though, or perhaps I've run into another edge case.

The idea there is to limit how much white space can be added when aligning the =>, similarly to Generic.Formatting.MultipleStatementAlignment. Also, it doesn't stop at the end of the code block, i.e. at an empty row or empty row followed by a comment.

[azaozz]

@jrf thanks so much for offering to help on Slack :) Frankly I'm not sure if I'm doing something "not-right", or it can't be done that way.

Was trying to configure WordPress.Arrays.MultipleStatementAlignment to work similarly to Generic.Formatting.MultipleStatementAlignment, i.e. stop on empty lines and limit the white space that is inserted. Testing this code with the settings in the above patch:

$arr = array(
	'one' => array(
		'test'           => 1,
		'test-test'      => 1,
		'test-test-test' => 1,
		'test-test-test-test-test-test-test' => 1,
		'test-test-test-test-test-test-test-test-test-test-test' => 1,
	),
	'one-longer' => array(
		'test' => 1,
	),
);

Results in these errors:

----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 13 | WARNING | [x] Array double arrow not aligned correctly;
    |         |     expected 31 space(s) between "'test'" and double
    |         |     arrow, but found 11.
    |         |     (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
 14 | WARNING | [x] Array double arrow not aligned correctly;
    |         |     expected 26 space(s) between "'test-test'" and
    |         |     double arrow, but found 6.
    |         |     (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
 15 | WARNING | [x] Array double arrow not aligned correctly;
    |         |     expected 21 space(s) between "'test-test-test'"
    |         |     and double arrow, but found 1.
    |         |     (WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

despite the <property name="maxColumn" value="20"/> setting. Also I couldn't find a way to make it not "bleed" across different code blocks as in this commit: https://core.trac.wordpress.org/changeset/45453/trunk/src/wp-admin/includes/list-table.php (the red is proper, the green looks... out of alignment) :)

[johnbillion]

Overriding the values of core's own coding standards in core seems like something is going very wrong.

It's fine that core excludes certain checks in certain files (there's a bunch of those in phpcs.xml.dist currently), but it's not fine that core would globally override the configuration of a rule.

This sounds like it should be moved to ​the WPCS repo to be fixed there.

[azaozz]

Replying to johnbillion:

Overriding the values of core's own coding standards in core...

Frankly I'm a bit unsure here. Looking at 46869.1.diff, it properly implements he (optional) exception to the rule for aligning equal signs here:

Exception: if you have a block of code that would be more readable if things are aligned, use spaces:

[tab]$foo   = 'somevalue';
[tab]$foo2  = 'somevalue2';
[tab]$foo34 = 'somevalue3';
[tab]$foo5  = 'somevalue4';

This is done by setting the Generic.Formatting.MultipleStatementAlignment PHPCS rule to match the above. But running into some difficulties trying to match that when aligning associative arrays (aligning of arrays doesn't seem to exist in any other coding standard rules/settings).

It's fine that core excludes certain checks in certain files (there's a bunch of those in phpcs.xml.dist currently), but it's not fine that core would globally override the configuration of a rule.

A bit lost here too. Why shouldn't the WPCS rules be configurable? As far as I am aware, WPCS caters to a broader audience than just core. I think this is great :)

This sounds like it should be moved to ​the WPCS repo to be fixed there.

Yes, one of the WPCS rules seems to need a fix, as mentioned above (it currently doesn't "stop" at the end of the code block). I hope I'll have the time figure out how it works and make a pr.