Allow array unpacking in ArrayDeclaration multiline Sniff

[edorian]

See #3557

Description

Squiz.Arrays.ArrayDeclaration.NoKeySpecified doesn't accept the following snippet:

$x = [
  'foo' => 'bar',
  ...$baz,
];

It triggers an error as phpcs doesn't detect that the second entry is array unpacking.

Key specified for array entry; first entry has no key (Squiz.Arrays.ArrayDeclaration.KeySpecified)

The following (single line) array validates:

$x = ['foo' => 'bar',  ...$baz];

Suggested changelog entry

Squiz.Arrays.ArrayDeclaration.KeySpecified: Allow array unpacking (...$array) in multi-line key-value array definitions.

Related issues/external references

Fixes #3557

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
  • I did not. I'd be happy to, if you give me a pointer on how I can do this without breaking earlier PHP versions, given it's new syntax.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• [Required for new files] I have added any new files to the package.xml file.

Notes:

The one test failure for PHP 8.2 seems unrelated as it was already present in 2e59060

[jrfnl]

I did not. I'd be happy to, if you give me a pointer on how I can do this without breaking earlier PHP versions, given it's new syntax.

@edorian Please do add some new tests. The fact that it is new syntax is irrelevant as the PHPCS tokenizer has backported the T_ELLIPSIS token to PHP 5.4, so the test code should tokenize the same in all PHP versions.

(if that is not the case, we have another issue, but better to know about it than not)

[edorian]

• Test

Thanks @jrfnl

Without my fix:

PHP_CodeSniffer\Standards\Squiz\Tests\Arrays\ArrayDeclarationUnitTest::testSniff
[LINE 475] Expected 0 error(s) in ArrayDeclarationUnitTest.2.inc but found 1 error(s). The error(s) found were:
 -> No key specified for array entry; first entry specifies key (Squiz.Arrays.ArrayDeclaration.NoKeySpecified)

With my fix: ✔️

[jrfnl]

@edorian Hi Volker, thanks for this PR.

I can confirm the current fix is correct and working.

I've left some minor comments inline, but nothing blocking.

One thing I would like to ask - could you add the test to both the test case files ?

The ArrayDeclarationUnitTest.1.inc test case file is supposed to have long array syntax, while the ArrayDeclarationUnitTest.2.2inc test case file is supposed to have those same tests using short array syntax.

It does look like the files have diverged a little over time, but I do think it's good to have the test(s) in both files.

And looking at the test, I also wonder if there should also be a test with the array unpacking at the start of the array declaration and one where the array unpacking happens in the "middle" of an array ?

As things are, this PR addresses just one part of issue #3557 (NoKeySpecified), while the other part (KeySpecified) is not yet fixed.

[edorian]

@jrfnl I've implemented your suggestions, fixed the KeySpecified error and added test cases.

I've done so by basically saying "if it's array unpacking line don't decide if it's a 'list' or a 'dict' shaped array".

Doesn't look very clean looking at the diff but it passes all tests I could come up with :)

[jrfnl]

@edorian Thanks for this update. The fixes look good.

I'm just thinking that there should probably be some additional tests still to safeguard that when an array entry is ignored due to unpacking, the key/no key for literal array entries is still correct (which it is with your fixes, but safeguarding that is the idea).

What about adding these additional tests ?

$x = [
      ...$a,
      'foo' => 'bar', // OK.
      'bar', // Error about no key (based on second entry).
     ];

$x = [
      ...$a,
      'bar', // OK.
      'foo' => 'bar', // Error about key (based on second entry).
     ];

$x = [
      'foo' => 'bar',
      ...$a,
      'baz' => 'bar',
      'bar', // Error about no key (based on first entry).
     ];

$x = [
      'bar',
      ...$a,
      'bar',
      'baz' => 'bar', // Error about key (based on first entry).
     ];

P.S.: thanks for taking on this issue, this is one of the sniffs which is unwieldy to adjust in most cases. Also see #582.

[edorian]

Added your suggested tests and tried to align the two test files, but the diff of that grew to big for this PR.

Happy to do that in a follow up when this is merged :)

@jrfnl Thank you for the fast response an the clear directions!

[edorian]

This is aligning 1.inc and 2.inc

[jrfnl]

@edorian Uh oh.. the tests are failing now. Could it be that the test case comment was only applied to the one test case file, not to all four ?

[edorian]

@jrfnl Sorry my bad. Didn't commit/push all the changes. Tests are passing again (failing test on master excluded) and I've applied your copy suggestion everywhere

[jrfnl]

Didn't commit/push all the changes.

@edorian Happens to the best of us.

All good now! Verified the test fail without the fix and pass with the correct expected notices with the fixes. ✅

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).