Fix 'Squiz.Arrays.ArrayDeclaration' coding standard

When I'm trying to check code with Squiz.Arrays.ArrayDeclaration sniff I get not proper failures like

-          '#title' => 'Error',
-          '#markup' => $message,
-        ];
+                   '#title'  => 'Error',
+                   '#markup' => $message,
+                  ];

Squiz.Arrays.ArrayDeclaration sniff has configurable options
But there are some issues with that
https://github.com/squizlabs/PHP_CodeSniffer/issues/582

Any idea what we can do?

Thank you @mfernea

It works with your ruleset.

After phpcbf I have to fix a lot of cases manually.
And finally phpcs reports only failures for arrays like

$array = array(
    'value',
    'key' => 'value',
);

Fixed review feedback.
Not all suggestions are passed CS.
For example following is failure

$array = [
      ['key1' => [   // <--- here
          'key11' => 'value',
        ],
      ],
      'value2',
    ];

I've started to corrected the CS issues, but can you confirm that I have the correct approach, e.g. ConfigEntityTypeTest.php:

     $data[] = [
       [
         'config_export' => [
-          'id',
+          0 => 'id',
           'custom_property' => 'customProperty',
         ],
       ],

@mfernea agree with your notice.
Fixed in the current patch.

@Jo I don't think that your approach is correct.
- it adds obvious code changes.
- we can't apply this approach to files like http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Component/Transli...

FILE: ...ib/drupal/core/lib/Drupal/Component/Transliteration/data/x21.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | ERROR | No key specified for array entry; first entry specifies
   |       | key
----------------------------------------------------------------------


FILE: ...trib/drupal/core/tests/Drupal/Tests/Core/Render/RendererTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 287 | ERROR | No key specified for array entry; first entry
     |       | specifies key
----------------------------------------------------------------------

I suppose that we have to exclude this sniff from list is that is available

Sounds sensible to me.

Good idea @mfernea
Thank you.

Updated patch contain:

• exclude sniff rule to phpcs.xml.dist
• removed not related to this sniff changes
• fix indentation issues

Thank you for a review.

Fixed from review #19

1. is not related to issue sniff and should be fixed in proper issue
2. added Squiz.Arrays.ArrayDeclaration.KeySpecified to phpcs.xml.dist file

Patch reroll

As a large coding standards cleanup that could disrupt other patches, this is one we want to schedule for during the RC phase (which is, conveniently, now). Don't have time to review just yet, but tagging. This will likely break other patches.

Alright, I want to get this one in soon because it will need rerolls a lot.

Reviewed with git diff --color-words --staged to ensure there were no other changes other than the array formatting. I confirmed that the patch's only non-whitespace changes were adding trailing commas (and in a couple of cases rearranging closing brackets and trailing commas across lines).

The word diff got confused on the hunks in the following files which I will need to read carefully by hand:

• core/modules/content_translation/tests/src/Unit/Menu/ContentTranslationLocalTasksTest.php
  This one is actually easier to read in HEAD. That's a shame. I checked it carefully and it looks correct.
• core/modules/user/src/Form/UserPermissionsForm.php
  word diff really made this one look like it was rearranging the array but it's not. Looks okay.
• core/tests/Drupal/Tests/Component/FileCache/FileCacheFactoryTest.php
  Eesh this one is hard to read in HEAD.
• core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
  I read it carefully in HEAD and with the patch; it looks correct.
• core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
  This is the only one I still haven't finished reading. Damn it's hard to follow in HEAD; I'm having to count brackets on my fingers. The word diff also totally fails because there are so many similar lines.

I need to go AFK so documenting this to pick up where I left off.

Let's add a followup to add inline comments documenting the provider in ConfigMapperManagerTest::providerTestHasTranslatable(). The problems are totally out of scope here but we really should have comments that explain each case in a provider. This one is definitely not self-documenting.

Posted: #2909079: Document the provider in ConfigMapperManagerTest::providerTestHasTranslatable()

Okay, finally finished spot-checking that last provider.

+++ b/core/phpcs.xml.dist
@@ -147,6 +147,44 @@
   <!-- Squiz sniffs -->
+  <rule ref="Squiz.Arrays.ArrayDeclaration">
+    <exclude name="Squiz.Arrays.ArrayDeclaration.NoKeySpecified"/>
+    <exclude name="Squiz.Arrays.ArrayDeclaration.KeySpecified"/>
+  </rule>
+  <!-- Disable some error messages that we do not want. -->
+  <rule ref="Squiz.Arrays.ArrayDeclaration.CloseBraceNotAligned">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.FirstValueNoNewline">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.KeyNotAligned">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.MultiLineNotAllowed">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.NoComma">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.NoCommaAfterLast">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.NotLowerCase">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.ValueNotAligned">
+    <severity>0</severity>
+  </rule>
+  <rule ref="Squiz.Arrays.ArrayDeclaration.ValueNoNewline">
+    <severity>0</severity>
+  </rule>

This is adding a lot of this pattern to phpcs.xml.dist. We do have it elswhere in the file, in the include section. Other places, we do exclude name = "". What's the reason for the different format? I see this discussed a little in #5 but it's not totally clear to me from that.

This format with the severoty 0 rather than including things was added to the few other lines in #2902396: Add sniffs already passing and not mentioned there.

@mfernea, are you saying that these exclusions using severity 0 are from coder itself?

Looks like it's so:
http://cgit.drupalcode.org/coder/tree/coder_sniffer/Drupal/ruleset.xml

So I can see the case for doing that then, good point.

Any idea what the difference is between that and the "exclude" format? Is there any?

Thanks @mfernea. I posted #2909267: Prefer "exclude" format over "severity 0" format in ruleset for readability and #2909268: Prefer "exclude" format over "severity 0" format in ruleset for readability once coder does. Now running all the array rules against both 8.5.x and 8.4.x locally just to check that no new mis-formatted arrays have been committed in the meanwhile.

Alrighty, committed and pushed to 8.5.x and backported to 8.4.x as a coding standards cleanup during RC. Thanks everyone!

Looks like this change made phpcs super slow on PHP 5.5 environments (some tests are aborting after 110 min now), compare:
8.5.x-dev test with PHP 5.5 & PostgreSQL 9.1
before: "Finished phpcs in 230.14850497246"
after: "Finished phpcs in 2306.7364938259"
8.5.x-dev test with PHP 5.5 & SQLite 3.8
before: "Finished phpcs in 233.63911414146"
after: "Finished phpcs in 2305.4536380768"
so it has added 34 minutes to the runtime.

Regarding requirement Drupal 8 need at least PHP 5.5.9+

What version of PHP you have on your environvent?

@andriyun: I'm fine :), what I meant is Drupal CI Testbots, they're on PHP 5.5.38 (see https://www.drupal.org/pift-ci-job/767155), PHP 5.6+ isn't showing the symptoms.

Let's fix that in follow-up

PS: probably the slowdown is #2911280: RectangleTest.php takes a very long time to scan for coding standards

@andypost: Confirming #2911280: RectangleTest.php takes a very long time to scan for coding standards fixes it.