Squiz.Arrays.ArrayDeclaration sniff not very configurable

[JDGrimes]

The Squiz.Arrays.ArrayDeclaration sniff is somewhat generic, but attempting to configure it to disable some errors by placing this in a ruleset will inadvertently disable other errors too:

    <rule ref="Squiz.Arrays.ArrayDeclaration">
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.KeyNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.CloseBraceNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNoNewline" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.MultiLineNotAllowed" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed" />
    </rule>

Basically, I'm not concerned about the alignment rules, but I'd like to still get the other errors, including the NoComma error when there is a missing comma, as in the following code:

array(
    'foo' => 1,
    'bar' => 2 // <-- missing comma, should give error.
);

However, due to logic of the processMultiLineArray() method, this error won't be given if alignment errors are disabled. Actually, it wouldn't be given even if the alignment errors were enabled, because only one error is given per-index within the loop. After a violation is found, continue is called. Maybe this is intended behavior, but it makes the sniff less configurable. Could this maybe be made more generic? Maybe custom indentation schemes could be defined?

[gsherwood]

It's probably best not to use that sniff unless you want everything it provides. It doesn't split all those checks into various sniffs because the lead-up work to parse the array would take too long over a large code base when run multiple times for each array.

It also stop processing at various places to ensure it doesn't generate incorrect errors.

So yes, it is a complex and non-configurable sniff because it exactly implements a single method of defining arrays for the Squiz standard. I'll leave this open as a feature request, but I don't have any plans on making it configurable. I think it would be better to write a new Generic sniffs (or sniffs) to implement checks that are more commonly used.

[JDGrimes]

OK. I started thinking that maybe a flag could be introduced that would just turn off the indentation part. But I'd have to dig into the logic a little more to see if it would be possible to that cleanly without still having the sniff do lot's of extra work.

[frvge]

+1 for more configuration possibilities (I ended up removing the sniff because of the alignment). Maybe it's possible to cache things somewhere based on the start of the token?

[gsherwood]

Reposting a comment from #815:

There is no point making modifications to this sniff. The only reason people ask for it is because it is the only sniff that actually checks array formats, but the formatting is bespoke and annoying.

Setting the indent to 4 spaces wont help you because it would want it indented to exactly 4 spaces after the open array token. What you (and the majority of developers, including me) probably want is to have content indented to the next tab stop (based on your indent rules) no matter how many spaces that actually is. This lets you easily indent arrays by just tabbing as normal.

What I want to do is write a set of new array sniffs in the Generic standard to enforce a few key concepts. You can then use as many or as few as you want in a custom standard. The ones I am planning are:

• comma after all array values
• indent values to next tab stop
• align => in key/value pairs

Then I'll probably change the existing Squiz sniff to remove a lot of checks, or remove the sniff entirely if the job can be done with other sniffs.

[beporter]

@gsherwood That's great to hear. A sniff that can enforce trailing commas (on multi-line arrays only) has been on my todo list for a long time. Is there anything we can do to help or make the job easier for you?

[aik099]

A sniff that can enforce trailing commas (on multi-line arrays only) has been on my todo list for a long time.

I've created such sniff myself for my standard. If you want you can use it: https://github.com/aik099/CodingStandard/blob/master/CodingStandard/Sniffs/Array/ArraySniff.php

[sebastienbarre]

Thanks @gsherwood; after spending some time looking at that Sniff to find why some rules where not triggered (early return were to blame), I understand there is probably no point sending a PR to add a configuration option. I was wondering if these new array sniffs were somewhere in a branch maybe? Thank you.

What I want to do is write a set of new array sniffs in the Generic standard to enforce a few key concepts. You can then use as many or as few as you want in a custom standard. The ones I am planning are: comma after all array values indent values to next tab stop align => in key/value pairs

[gsherwood]

I was wondering if these new array sniffs were somewhere in a branch maybe?

I haven't started them and I'm not sure when I'll get the time. But it's on my todo list.

[beryllium]

It would be really nice if double arrow alignment could be implemented as a separate sniff for 3.0, but I suppose it might be too much work/too late for that.

[jrfnl]

FYI: with the WordPress Coding Standards we've been running into more and more edge cases related to this sniff, so I've been splitting bits and pieces out from this sniff each time we run into another issue.

I expect nearly everything to be split off into independent sniffs before the end of the year. Once the split off sniffs have been tried & tested & found stable, I intend to send in pull requests for them here which would effectively solve this issue.

[beryllium]

Thanks for the update :) That's great news.

[gsherwood]

This is something I am considering for version 4 and not for version 3. I want to remove this sniff and replace it with a number of others - not necessarily one sniff per error message now that version 3 can include single messages in rulesets.

Whatever ends up happening, anyone using this sniff would need to change their ruleset to get the same functionality, so I'll provide that in an upgrade guide. I'd like to make version 4 the version where a lot of sniff codes change so it is a one-off process. This may mean moving more sniffs into the Generic standard, or breaking a few things out. But I'm not going to start on this for a while yet.

[jrfnl]

For what it's worth/FYI: for the WordPress Coding Standards I've written the following sniffs which could be pulled here without significant adjustment (other than code style):

• Array.ArrayIndentation
  • Checks that the array close brace is aligned with the first content on the line containing the array open brace.
  • Checks that each line within the array is indented one tab/4 spaces (configurable) from the start of the line containing the array open brace. This takes multi-line array items, heredoc/nowdoc structures etc into account and handles these correctly.
• Arrays.CommaAfterArrayItem
  • Check that every array item has a trailing comma in multi-line arrays
  • Check that there is no comma after the last array item in single-line arrays
  • Ensures that there is no space between the comma and the end of the array item (with the exception of heredoc/nowdoc)
  • Checks that there is exactly one space between the comma and the start of the next array item for single-line items.
• Arrays.MultipleStatementAlignmentSniff
  • Aligns the double arrow operator to the same column for each item in a multi-line array. This sniff is highly configurable and has public properties such as ignoreNewlines, exact, maxColumn to manage line length, alignMultilineItems to allow for finely grained configuration of how to handle arrays containing multi-line items.

Aside from the above WPCS has a ArrayDeclarationSpacing sniff which handles single vs multi-line arrays based on number of array items, array closing brace being a on a line by itself etc. That one could probably be split further if there is any interest in me pulling these here.

All the fixer conflicts and parse errors caused by fixers from the PHPCS native sniff we saw while working on the WP Core auto-fixer project have been solved by these changes.

[gsherwood]

Just FYI, 3.2.0 will add a Generic.Arrays.ArrayIndent sniff just to enforce a basic 4-space indent of array keys, and alignment of the closing brace with the main statement. We internally changed our Squiz coding standard to enforce this new indent, so I needed to bring this particular change forward to deal with that.

Commit is here: https://github.com/squizlabs/PHP_CodeSniffer/commit/99b2cb57c87c7b3bd555e9a7c2b4f65bfb24d956 Sniff is here: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/Arrays/ArrayIndentSniff.php

In anticipation of more generic array sniffs, I also added an AbstractArraySniff class to parse the array and make it easier to find keys and values. This new sniff is using that abstract class.

I have not touched the existing Squiz sniff in any way, so it will continue to function normally for anyone using it, or customising it.

[jrfnl]

@gsherwood Looking at it now. Want some feedback ?

[gsherwood]

Want some feedback ?

Sure. I rushed it this morning for work, but I may not get a chance to go back over it. So if you can, that would be great.

[jrfnl]

@gsherwood I've just ran some tests and am getting pretty weird results in a number of situations:

1. Arrays interspersed with comments (all comment formats)
2. Heredoc/nowdoc with concatenation after it
3. Tab-based files are not handled correctly at all, including tabs being replaced with spaces even when the indent is correct
4. Correct indentation sometimes gets corrected to incorrect indentation (3 tabs becoming 6 spaces)
5. Subsequent lines in a multi-line array value do not get handled (correctly)

To reproduce/see examples of all of the above, you could run the below command against this folder: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/tree/develop/WordPress/Tests/Arrays phpcbf . --standard=Generic --sniffs=Generic.Arrays.ArrayIndent --suffix=.fixedGeneric --tab-width=4 The most relevant results will be the diff between the output for the ArrayIndentationUnitTest.1/2.inc files as well as the diff for the ...UnitTest.php files (issue 4)

[gsherwood]

Arrays interspersed with comments (all comment formats)

It's not designed to change the indent of any lines that are not index start lines. That includes comment lines.

Heredoc/nowdoc with concatenation after it

I assume you mean that the subsequent lines are not indented. But this is not the intention of this sniff.

Tab-based files are not handled correctly at all, including tabs being replaced with spaces even when the indent is correct Correct indentation sometimes gets corrected to incorrect indentation (3 tabs becoming 6 spaces)

Sniffs shouldn't have to do any work with tabs if you use --tab-width and my testing has shown this sniff works the same way all others do, so I'm not sure why you'd be having problems. Which example did you see this problem with?

Subsequent lines in a multi-line array value do not get handled (correctly)

I intentionally didn't do this in the first version of the sniff. Other sniffs kick in and fix the values for me. This one is only to do with key alignment at the moment.

In general, I think you are expecting this sniff to do a lot more than I designed it to do. Over time, I'll either extend this sniff to do more formatting, or add new array sniffs to take over those other checks and fixes, but it's not going to replace the indent sniff you've written for the WordPress standard.

[jrfnl]

In general, I think you are expecting this sniff to do a lot more than I designed it to do. Over time, I'll either extend this sniff to do more formatting, or add new array sniffs to take over those other checks and fixes, but it's not going to replace the indent sniff you've written for the WordPress standard.

Understood.

Sniffs shouldn't have to do any work with tabs if you use --tab-width and my testing has shown this sniff works the same way all others do, so I'm not sure why you'd be having problems. Which example did you see this problem with?

Any of the ...UnitTest.php files in the WP standard using the above mentioned command.

[gsherwood]

Any of the ...UnitTest.php files in the WP standard using the above mentioned command.

The main indent one is the one I tested and it's working for me. I don't know what specific issue you are having.

[jrfnl]

The main indent one is the one I tested and it's working for me. I don't know what specific issue you are having.

That's weird, not sure what's going on in that case.

[jrfnl]

@gsherwood FYI - I just ran into an error in the sniff while trying to auto-fix one of the open PRs: Undefined index: parenthesis_closer in /path/to/phpcs/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php on line 378

[gsherwood]

@gsherwood FYI - I just ran into an error in the sniff while trying to auto-fix one of the open PRs: Undefined index: parenthesis_closer in /path/to/phpcs/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php on line 378

I didn't modify that sniff, so it's probably an existing bug. It would be great if you happen to have code that can replicate it, and just submit a bug report for that.

[jrfnl]

I didn't modify that sniff, so it's probably an existing bug. It would be great if you happen to have code that can replicate it, and just submit a bug report for that.

@gsherwood It's probably to do with the exclusions and the sniff not handling the checks with enough isolation. As that sniff is moving towards deprecation in favour of more modular sniffs, I'm not sure it's worth spending much time on it.

[glen-84]

comma after all array values

Could the sniff be configurable to enforce the opposite (no comma after the last element in a multi-line array)?

align => in key/value pairs

It would be great if this could make alignment optional, for example:

$x = [
    1 => 1
    999 => 999
]

... is okay because there is a single space before each =>.

$x = [
    1   => 1
    999 => 999
]

... is okay because the => are all aligned.

It should prevent mixing those styles as well. I guess starting from the top, as soon as an element uses more than one space before the =>, it goes into "alignment mode", and all => must then be aligned.

Also, it would be nice if it could enforce that the longest key had only a single space after it. i.e., this would be invalid:

$x = [
    1       => 1
    999     => 999
]

... because key 999 (the longest) should be followed by a single space.

[gsherwood]

@glen-84

Could the sniff be configurable to enforce the opposite (no comma after the last element in a multi-line array)?

The existing sniff wont, but I still need to write a new sniff that enforces a comma after the last item and it's a good idea to also allow it to enforce no comma as well.

It would be great if this could make alignment optional, for example:

I see what you're getting at, but I wasn't intending to do anything like this in a new double array alignment sniff. Standards are better when the developer doesn't have to make a decision and the sniff doesn't have to infer anything from how the code is written.

One suggestion would be to figure out why you wouldn't want the arrows aligned and then code something like that into a sniff. For example, the sniff that aligns equals signs under each other when you are assigning multiple variables in a block knows that sometimes variable names are too long and the number of spaces you need to align an equals sign gets ridiculous. So it has a max alignment setting that says "if I need to use more than N spaces to align the equals sign, use a single space instead". It's not a decision the developer makes - they can't decide to align it anyway.

Something to think about maybe.

Also, it would be nice if it could enforce that the longest key had only a single space after it. i.e., this would be invalid:

The existing sniff already does this. The code sample you posted generates these errors:

  3 | ERROR | [x] Array double arrow not aligned correctly; expected 3 space(s) but found 7 (Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned)
  4 | ERROR | [x] Array double arrow not aligned correctly; expected 1 space(s) but found 5 (Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned)

[glen-84]

One suggestion would be to figure out why you wouldn't want the arrows aligned ...

I just don't think that it always make sense, especially in larger array declarations. It can become a bit of a pain to format everything correctly (assuming no cbf/prettier/etc.).

I suppose max spaces/array length settings could help, but optional alignment seems useful as well.

[RobQuistNL]

How is this still open :sweat_smile:

[ncou]

hi, nice piece of code. After a few tweak, it seems to me the check to enforce a space after the comma is not working. I should have an error with the "NoSpaceAfterComma" check but phpcs doesn't detect the missing space :(

[gsherwood]

From #2687

Help, I need check a whitespace after comma in single line array/list.

Code:

[$a,$b,$c] = ['a','b','c'];

// convert to

[$a, $b, $c] = ['a', 'b', 'c']; Version: 3.5.1

I saw the Squiz.Arrays.ArrayDeclaration source code, and config Squiz.Arrays.ArrayDeclaration.SpaceAfterComma rule, but it's does not work.