Faulty core fix: Array elements aren't aligned at the =>

[pento]

Given this example:

<?php

array(
	'aaa' => 'b',
	'c' => 'd',
	'eeeeeee' => 'f',
);

The array elements aren't being aligned at the =>. This behaviour isn't explicitly noted in the handbook, just recommended. I can make it more explicit in the Hanbook, thought.

[pento]

Discussion over on the Core ticket: https://core.trac.wordpress.org/ticket/41057

[jrfnl]

Additional notes:

• As mentioned on Slack: I checked the history of the ArrayDeclaration sniff which used to be included in Core and found that up to May 2015 the array alignment was sniffed for by that sniff, but this was (accidentally ?) taken out when auto-fixing of errors from that sniff was introduced in Add support for error fixing to ArrayDeclaration #378. /cc @JDGrimes
  I could not find any discussion about this.
• If checking of array assignment alignment would be introduced, should we also revisit the discussion about non-array multi-statement alignment ?

  $a = 123;
  $abc = 'abc';

  This is currently also not checked for.
  There is an upstream sniff for this Generic.Formatting.MultipleStatementAlignment which has some configuration options.
  This sniff was part of the original standard when the project began. Got taken out again in Eliminate multiple statement alignment #139, was proposed to be added again in Add Core and Extra sniffs #382 and rejected for not being refined enough in this discussion.

[JDGrimes]

As mentioned on Slack: I checked the history of the ArrayDeclaration sniff which used to be included in Core and found that up to May 2015 the array alignment was sniffed for by that sniff, but this was (accidentally ?) taken out when auto-fixing of errors from that sniff was introduced in #378. /cc @JDGrimes

It looks to me like it was already commented out prior to that: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/378/files#diff-8eb86ff485de5031b1b1644112295065L386

And this goes all the way back to the initial commit:

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/04fd547c691ca2baae3fa8e195a46b0c9dd671c5/Sniffs/Arrays/ArrayDeclarationSniff.php#L406-L426

Or is there another alignment check that I missed?

[jrfnl]

@JDGrimes I see what you mean. Seems I was looking in the wrong place (assignment operators in single line arrays which was not previously commented out): https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/04fd547c691ca2baae3fa8e195a46b0c9dd671c5/Sniffs/Arrays/ArrayDeclarationSniff.php#L117-L145

[westonruter]

This behaviour isn't explicitly noted in the handbook, just recommended. I can make it more explicit in the Hanbook, thought.

I personally do not think alignment is a good practice to recommend. Adding or removing an item from the array then means all of the items in the array will need to get re-aligned, resulting in messy diffs and conflicting patches. It's also a direct contradiction of the subsequent point in the coding standards, about use of a comma after the last item in an array:

Note the comma after the last array item: this is recommended because it makes it easier to change the order of the array, and makes for cleaner diffs when new items are added.

[jrfnl]

Adding or removing an item from the array then means all of the items in the array will need to get re-aligned, resulting in messy diffs

That could be partially accounted for by an $exact = false property, similar to how it's done in the ScopeIndent sniff.

[pento]

Adding or removing an item from the array then means all of the items in the array will need to get re-aligned, resulting in messy diffs and conflicting patches. It's also a direct contradiction of the subsequent point in the coding standards, about use of a comma after the last item in an array

The array might re-aligned, depending on whether you're adding or removing the longest key.

I also find myself less and less concerned about the cleanliness of a diff, vs. the cleanliness of the actual code - I look at diffs and revision history that would be affected by this occasionally, I work with the actual code all day, every day. It seems like a reasonable trade off to make the common case nicer.

[GaryJones]

To paraphrase - making the resulting code easier to read for everyone >>> making the diff cleaner for the few people who need to read the commit / chase down blames.

[jrfnl]

I've been working on this sniff and have a couple of questions:

• I presume this sniff should be added to the Core ruleset (even though it's not explicit in the handbook) ?

• Generally speaking, everything that can be auto-fixed is an error. However, even though this can be auto-fixed, this is currently a recommendation, not a requirement, so should this be an error or a warning ?

• If checking of array assignment alignment would be introduced, should we also revisit the discussion about non-array multi-statement alignment ?

  On that note: anyone remember why the sniff for this was previously deemed not refined enough ? Particularly code samples would be very welcome so I can see if the upstream sniff can be improved.

[JDGrimes]

On that note: anyone remember why the sniff for this was previously deemed not refined enough ?

I think the main issue I had with the sniff was that it always aligned assignments that were on consecutive lines, regardless of the difference in length between the variable names. I usually only align the assignments if they are within like 5-10 spaces. Sometimes when the difference is greater than that aligning just doesn't make sense (depending on the values being assigned), and can actually decrease readability. I don't think this was configurable, but I might be remembering wrong.

Of course, for array alignment, I tend to not worry about how many spaces need to be used, since it is all part of a single statement. Aligning in an array is almost always more readable. Though I will admit that I'm sometimes pragmatic about alignment even then.

[jrfnl]

Sometimes when the difference is greater than that aligning just doesn't make sense (depending on the values being assigned), and can actually decrease readability. I don't think this was configurable, but I might be remembering wrong.

It actually is configurable (and looks like it always has been) using the $maxPadding property: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#genericformattingmultiplestatementalignment

Of course, for array alignment, I tend to not worry about how many spaces need to be used, since it is all part of a single statement. Aligning in an array is almost always more readable. Though I will admit that I'm sometimes pragmatic about alignment even then.

I intend to implement a similar $maxPadding property for the WP Array assignment alignment sniff, so for those of us who want a little more pragmatism, it will be configurable.

[jrfnl]

On a different note: Based on what I have so far, the sniff would have some overlap with the WordPress.WhiteSpace.OperatorSpacing sniff which checks that:

• there is a minimum of 1 space between array keys and the arrow (doesn't check exact spacing before to allow for alignment)
• there is exactly 1 space between the arrow and the array value

That would currently result in slightly conflicting error messages addressing the same issue:

  86 | ERROR | [x] Array double arrow not aligned correctly; expected 3 space(s) between
     |       |     "'a'" and double arrow, but found 0
     |       |     (WordPress.Arrays.AssignmentOperatorSpacing.ML_DoubleArrowNotAligned)
  86 | ERROR | [x] Expected 1 space before "=>"; 0 found
     |       |     (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)

Options:

• I could remove the double arrow from the WhiteSpace.OperatorSpacing check completely.
• Or I could remove the duplicate bits from the Arrays.AssignmentOperatorSpacing sniff, that would lower the maintenance burden as the WhiteSpace.OperatorSpacing sniff comes from upstream, but would make that Arrays.AssignmentOperatorSpacing sniff feature incomplete for when it would be upstreamed at some point (and, yes, this is a candidate for that as it would partially solve upstream Squiz.Arrays.ArrayDeclaration sniff not very configurable squizlabs/PHP_CodeSniffer#582 ).
• We could decide that the duplication is is something we can live with and leave it as is.

Opinions ?

[JDGrimes]

I think these are really independent checks that someone might decide to use one without the other. So I don't think that either of them should be removed. Perhaps the ruleset should simply silence WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore. And/or maybe it could be updated to say "Expected at least 1 space".

[jrfnl]

Perhaps the ruleset should simply silence WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore. And/or maybe it could be updated to say "Expected at least 1 space".

The problem with that is that the upstream sniff handles the whitespace checking for a large number of operators and there is no difference in the error codes, so we can't silence it only for the double arrow assignment operator.
Similarly, "Expected at least 1 space" is only the behaviour for (all type of) assignment operators. For all the other operators, the behaviour is to demand exactly 1 space. Again, as the error code is the same for all operators, we cannot change the message just and only for the assignment operators.

[JDGrimes]

Ah, right. Maybe just remove the double arrow from the WhiteSpace.OperatorSpacing check then. I don't really understand the alternative of removing part of the Arrays.AssignmentOperatorSpacing sniff—how would it still work, what would it still do?

[jrfnl]

I don't really understand the alternative of removing part of the Arrays.AssignmentOperatorSpacing sniff—how would it still work, what would it still do?

@JDGrimes Having thought about this some more based on the questions you posed, I think removing part of the Arrays.AssignmentOperatorSpacing sniff will in actual fact be the best option. I will also rename the sniff to something like Arrays.MultipleStatementAlignment (better name suggestions welcome).

In the WIP branch, I was checking both spacing before as well as after the => array assignment operator, similar to what the upstream Squiz.Arrays.ArrayDeclaration sniff does and the - now deprecated - WP version of that once did.
For single line arrays, this meant: enforcing exactly one space before and after the double arrow.
For multi-line arrays, enforcing alignment before the double arrow and one space after.
(with some configuration options, but that's besides the point)

This is basically already covered by the WhiteSpace.OperatorSpacing except for the alignment before the arrow.
So, I think it's best to narrow the focus of the sniff and only target the multi statement alignment for multi-line arrays.
That would still yield an overlapping message when no space at all is found between an array index key and the double arrow, but would eliminate the other duplications.

I will adjust the WIP branch and expect to be able to pull this tomorrow.

[jrfnl]

Ok, so I've pulled two branches now:

• Add new Arrays.MultipleStatementAlignment sniff to Core #1135 to sort out the double arrow alignment in arrays
• Add sniff to check for alignment of assignment operators to Core #1136 to sort out the equal sign alignment for multiple assignment statements after each other.

One important thing to note about these PRs:

The maxPadding property in the Generic.Formatting.MultipleStatementAlignment sniff works differently than the maxColumn property in the new WordPress.Arrays.MultipleStatementAlignment sniff.

• maxPadding determines the maximum whitespace padding between the variable token and the equal sign independently of the position of either on a line.
• maxColumn only looks at the position on a line.

I've made the choice to implement a maxColumn property rather than maxPadding for the new sniff as the whitespace padding required is reset for each "group" of assignments by a blank line, a comment, a non-assignment statement, or, if that would be the case, the needed padding getting out of hand.

An array however is never "broken" up like that, so I've let consistency prevail.

Regarding the fact that only blank lines, comments and non-assigment statements break a group of assignments in the Generic.Formatting.MultipleStatementAlignment sniff:
this also means that this code from the tools/i18n/t/NotGettextedTest.php file would get reformatted to align the = for $code with the one for $tokens:

		$code = '
<?php
	$s = 8;
echo /* WP_I18N_GUGU*/ 	"yes" /* /WP_I18N_UGU		*/;
	if ($x == "18181") { wp_die(sprintf(/*WP_I18N_DIE*/\'We died %d times!\'/*WP_I18N_DIE*/)); }
?>';
		$tokens = token_get_all( $code );

This has to do with it $code being assigned a multi-line string, but it still being the same statement.

There is currently no option in the sniff to break the grouping if there are "more than x lines" within in an assignment statement, which when an assignment is an array or a closure could be desirable.

Opinions welcome.

[GaryJones]

There is currently no option in the sniff to break the grouping if there are "more than x lines"

I don't think it would, or necessarily should, get much use - that feels like it's introducing an exception.

If someone doesn't want an alignment (as I wouldn't if my own code contained this), then they can add a blank line.

[JDGrimes]

There is currently no option in the sniff to break the grouping if there are "more than x lines"

This must be why I stopped using this sniff before. IMO this makes code far less readable, rather than more readable.

That said, as @GaryJones pointed out, you can just add a blank line to overcome this, so an option isn't really needed. I tend to add blank lines to my code liberally anyway, so I probably could live with it.

In fact, I'd prefer if blank lines were added for most of the fixes in WPCS for multiline assignments rather than padding being added.