Squiz.Arrays.ArrayDeclaration throws NoComma error when array value is a shorthand IF statement

[Aliance]

I'm trying to check this file:

<?php
declare(strict_types=1);

namespace Test;

class SomeClass
{
    protected function transformRow(array $row)
    {
        return [
            'key1' => 'value1',
            'key2' => (int) $row['value2'],
            'status_verify'            => $row['status'] === 'rejected'
                ? self::REJECTED_CODE
                : self::VERIFIED_CODE,
            'user_verification_status' => in_array($row['status'], ['notverified', 'unverified'], true)
                ? self::STATUS_PENDING
                : self::STATUS_VERIFIED,
            'created_at' => strtotime($row['date']),
        ];
    }
}

With this command:

docker run --rm -v $(pwd):/scripts/ texthtml/phpcs:3.3.0 phpcs --standard=/scripts/phpcs.xml -n -p -s /scripts/test.php

And I'm getting these unexpected errors:

13 | ERROR | [x] Each line in an array declaration must end in a
| | comma (Squiz.Arrays.ArrayDeclaration.NoComma)
16 | ERROR | [x] Each line in an array declaration must end in a
| | comma (Squiz.Arrays.ArrayDeclaration.NoComma)

---

Interesting, that if I replace status_verify with key3 f.e., the error on the line 13 will disappear. And if I replace user_verification_status with key4 f.e., the all errors will disappear 🤔

[Aliance]

@gsherwood please pay attention to this bug!

[jrfnl]

@Aliance Already shouting for attention less than 24 hours after reporting something ? You may want to check your attitude.

[gsherwood]

@Aliance I've got the report. I'm probably in a different timezone to you and have been asleep half the time.

[gsherwood]

This can be replicated using this code:

<?php
$foo = [
    '3' => $foo
        ? 1
        : 0,
    '5' => '3',
];

It is caused by the sniff not understanding how to skip shorthand IF statements being used as array values. If you put parenthesis around it, it does skip it. But without, it will need to try and determine the end of the statement itself.

So this, for example, doesn't generate that error:

<?php
$foo = [
    '3' => ($foo
        ? 1
        : 0),
    '5' => '3',
];

[gsherwood]

Forgot to address the fact that the error is muted when changing the keys:

This is just due to the sniff bailing out early when it finds the double arrow alignment error. If you change the keys and align the double arrows, you'll get the errors back:

<?php
declare(strict_types=1);

namespace Test;

class SomeClass
{
    protected function transformRow(array $row)
    {
        return [
            'key1' => 'value1',
            'key2' => (int) $row['value2'],
            'key3' => $row['status'] === 'rejected'
                ? self::REJECTED_CODE
                : self::VERIFIED_CODE,
            'key4' => in_array($row['status'], ['notverified', 'unverified'], true)
                ? self::STATUS_PENDING
                : self::STATUS_VERIFIED,
            'key5' => strtotime($row['date']),
        ];
    }
}

[gsherwood]

The only way to find the end of a shorthand IF is to begin at the very start, so I had to use findEndOfStatement to see where the array values ended. I tried discovering the shorthand IF when I hit the T_INLINE_THEN but that's a bit too late to find the ending because I'm in a slightly different context.

So I ended up changing the way that part of the sniff works to always use findEndOfStatement instead of looping over the value tokens manually and making exceptions for things like closures and multi-line strings.

@Aliance All the tests are passing, but it would be great if you could test out the patch manually before I release it.

[Aliance]

@jrfnl I've just thought that maybe the repo owner does not have the indirect notifications, so I've decided to mention him explicitly. Is it illegal? Where does the 24 hours delay rule is located?

[Aliance]

@gsherwood thanks for the patch!

it would be great if you could test out the patch manually before I release it.

I've tested it, seems like it really works fine!

Are you planning to create a new release?

[gsherwood]

@Aliance Thanks for testing that. This was the last scheduled item for the 3.3.1 release, so a new version should come out this week if nothing major gets reported.

[Aliance]

When do you plan to create a new release?

[gsherwood]

When do you plan to create a new release?

I was planning to create one 3 weeks ago but I've had barely any time to work on PHPCS lately. I hope to have a release out ASAP though.

[EvgenyOrekhov]

Still getting this error on version 3.3.1 with the following code:

return [
    'foo' => $bar
        ? baz([
            'abc' => function () {
                return;
            },
        ])
        : $def,
];

[gsherwood]

@EvgenyOrekhov Your issue is actually by bug #2120 that has been fixed in master. The problem is that the inline if was not being tokenized properly in very specific cases (read the bug report for an example).

So I can confirm that your code is exposing a bug in 3.3.1 but it will be fixed in 3.3.2. Thanks for posting that code.

[Aliance]

Also, there is the same error but on the other hand: no error is shown, but expected to be:

<?php
declare(strict_types=1);

namespace Test;

class SomeClass
{
    public function getCorrections($id): array
    {
        return $this->storage->cacheResponse(
            function () {
                return [];
            },
            [
                $this->storage->handleCacheKey(Constant::CACHE_KEY_LIST, $id)
            ]
        );
    }
}

I've expected to see the error on missing the , for the array, but nothing found.

---

Tested in 3.3.1 and 3.3.2:

$ docker run --rm -v $(pwd):/scripts/ texthtml/phpcs:3.3.1 phpcs --standard=/scripts/phpcs.xml -n -s -p /scripts/test.php
. 1 / 1 (100%)


Time: 55ms; Memory: 6Mb

$ phpcs --standard=./phpcs.xml -n -s -p test.php
. 1 / 1 (100%)


Time: 65ms; Memory: 6Mb

$ phpcs --version
PHP_CodeSniffer version 3.3.2 (stable) by Squiz (http://www.squiz.net)

[gsherwood]

@Aliance Squiz.Arrays.ArrayDeclaration doesn't throw that error for arrays that contain a single entry. It throws an error that says "Multi-line array contains a single value; use single-line array instead". That's just the formatting that it wants.

[Aliance]

@gsherwood I thought there was a setting to force a comma requirement in every array, didn't it?

[gsherwood]

I thought there was a setting to force a comma requirement in every array, didn't it?

Squiz.Arrays.ArrayDeclaration does not have any settings so there is no way to change it's internal behaviour. It's never had any settings so I'm not sure which one you are referring to.