[meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard

We also need to remove this line from phpcs.xml.dist to ensure that this standard is always followed in the future:

    <exclude name="Drupal.Arrays.Array.LongLineDeclaration"/>

Incomplete review. This is going to be a bit disruptive to commit as it affects so much, but overall I think it does make a whole lot of code more readable.

1. +++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php
   @@ -31,7 +31,12 @@ class DisplayBlockTest extends ViewTestBase {
   +  public static $modules = [
   +    'node', 'block_test_views',
   +    'test_page_test',
   

   I think 'node' and 'block_test_views' should be on their own line.

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentCacheTagsTest.php
   @@ -91,7 +91,10 @@ public function testBlock() {
   +    $expected_block_cache_contexts = [
   +      'languages:' . LanguageInterface::TYPE_INTERFACE,
   +      'theme', 'user.permissions',
   +    ];
   

   I think each of these should be on their own line.

3. +++ b/core/modules/block_content/tests/src/Functional/Views/BlockContentFieldFilterTest.php
   @@ -60,7 +60,14 @@ public function setUp($import_test_views = TRUE) {
   +      'body' => [
   +        ['value' => $this->blockContentInfos['en']],
   +      ],
   

   Not sure how best to format this, but this looks a bit odd.

4. +++ b/core/modules/comment/src/CommentManager.php
   @@ -169,10 +169,17 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +        $destination = [
   +          'destination' => Url::fromRoute('comment.reply',
   +          $comment_reply_parameters,
   +          ['fragment' => 'comment-form'])->toString(),
   +        ];
   

   The formatting here is almost certainly off.

5. +++ b/core/modules/comment/src/CommentManager.php
   @@ -169,10 +169,17 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +        $destination = [
   +          'destination' => $entity->toUrl('canonical',
   +          ['fragment' => 'comment-form'])->toString(),
   +        ];
   

   Same here.

6. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -367,7 +367,11 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
   +      in_array($form_object->getOperation(), [
   +        'edit',
   +        'default',
   +        'layout_builder',
   +      ], TRUE) &&
          $this->moderationInfo->isModeratedEntity($form_object->getEntity());
   

   This also looks a bit odd now with the && but not sure what the solution is.

7. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -102,12 +111,18 @@ public function testEntityDisplayCRUD() {
   +    EntityViewMode::create([
   +      'id' => $display->getTargetEntityTypeId() . '.other_view_mode',
   +      'targetEntityType' => $display->getTargetEntityTypeId(),
   +      ])->save();
   

   Last line here is indented incorrectly.

1. This no longer applies. We'll also want to backport it to avoid branch divergence, so setting the version selector to 8.8.x.

2. This cleanup is large enough that I think we should split it into a few logical chunks (by concept, not by module). I'd suggest:

   • One patch for fixing the $modules test property.
   • One patch for fixing calls to drupalCreateUser().
   • One patch for fixing calls to drupalCreateNode().
   • Evaluating whatever's left after that and seeing if it should be split up further.

3. Also noticed this:

   +++ b/core/modules/ckeditor/tests/src/Kernel/CKEditorPluginManagerTest.php
   @@ -75,7 +75,19 @@ public function testEnabledPlugins() {
   +    $this->assertIdentical([
   ...
   +    ], $plugin_ids, 'Additional CKEditor plugins found.');
   

   Having array items after the closing square bracket like this his doesn't follow our coding standards -- if a child array is formatted as multi-line, the parent array needs to be as well.

4. The interdiffs attached so far are not correct. If the patch is being rerolled, make sure the interdiff is between the rerolled patch (before other changes) and the final patch (with additional changes).

Thanks!

I filed #3132745: Fix Drupal.Array.Array.LongLineDeclaration coding standard for instances of the $modules test property to start.

Created the next one: #3133033: Fix Drupal.Array.Array.LongLineDeclaration coding standard for instances of the drupalCreateUser() test method.

Updating IS with details from #15 now this is a meta.

I think we need to consider configuring this rule a bit differently. Take the code

getOperation(), ['add', 'edit', 'default'], TRUE)) {
      return;
    }

It's indented 4 characters because it's from a class method and another 2 because it is inside an if. This code will cause the sniff to fire.

But I think other ways of writing this code make it less legible and worse for both the computer and the programmer. At the very least we should consider setting \Drupal\Sniffs\Arrays\ArraySniff::$lineLimit to something higher than 80 - this can be configured in our phpcs.xml.dist. I guess we could also ask for the default to be changed.

After using the full Drupal standard on client projects this is easily the rule that makes me add and ignore the most.

Opened #3185082: Drupal.Arrays.Array.LongLineDeclaration make me write less readable code

I have commented on #3185082: Drupal.Arrays.Array.LongLineDeclaration make me write less readable code which could be a useful discussion.

Maybe we should try setting line limit to a few other options like 120 and 160 and see how many failures we get in core for each of those?

#23 is a good idea but will need to update coder to fix bugs in the rule.

Patch to show the state of things with the current 80 limit using existing Coder 8.3.10.
I know this is a meta issue, therefore leaving the status as 'active' and I will trigger tests manually.

[edit: ignore this failed patch at 8.9. The issue should be at 9.2]

Patch #25 shows a total of 5,805 long lines for inline arrays, using 80 limit and Coder 8.3.10 as is.

Patch #26 is an attempt to patch Coder for two issues, one already in 8.3.11 and one not committed yet. I have used this method before, with patch files saved on drupal.org, and have used github pr patches on Travis builds, but not tried them on d.o. before (so this could fail)

So the two Coder improvements reduce the count by 1,032 from 5,805 down to 4,773.

Patch #27 has the limit set to 100

A TOTAL OF 2928 ERRORS AND 0 WARNINGS WERE FOUND IN 983 FILES

So setting the limit to 100 removes 1,845 warnings (4,773 - 2,928) which is 39% of the problem lines.

Patch #28 sets the limit to 120.

A TOTAL OF 1907 ERRORS AND 0 WARNINGS WERE FOUND IN 721 FILES

A limit of 120 removes another 1,021 warnings (21%) taking the problem lines down to 1,907 (40% of the original 4,773)

Patch #29 sets the limit to 140, which I think is probably too long for a default. Only large monitors will have viewing ports able to display files this wide and still be readable. I would suggest that developers on regular laptops will find 120 is about the limit for viewing sensibly in an editor. But those working in terminal emulators will still want the 80 limit.

With 140 as the limit we still get

A TOTAL OF 1253 ERRORS AND 0 WARNINGS WERE FOUND IN 518 FILES

This has reduced the total by a further 654 (13% of the original) but that still leaves 47% of the problem lines being over 140.

I did a quick count and of the remaining 1,253, around 800 lines have 140-200 length, 240 lines are in the 200-300 range, 100 lines are in the 300-600 range, and an astonishing 50 rows exceed 700, with 4 lines over 1,100 the longest being 1133 chars. I think we should have some limit!

@jonathan1055 it'd be good to see a sample of what 1253 errors encompass. I'm guessing that some of the super long lines are not in files meant for humans. I.e. they are in generated files and therefore this rule is not that pertinent.

I'm guessing that some of the super long lines are not in files meant for humans.

On the contrary (and I was surprised) the very long lines are specifically writen by hand for human consumption in the UI. The five lines > 1000 in length are all implementations of hook_help(), here are three examples:

Length 1133 in modules/content_translation/content_translation.module line 31
Length 1100 in modules/field_ui/field_ui.module line 39
Length 1234 in modules/search/search.module line 75 (and 77)

Adding #3173782: Increase line length limit to 120 because we need to discuss on that issue the bigger question of why we have the error sniff for long array lines but to not have any warning at all for long code lines. It seems we should have both or neither. I guess if the line has an array then it is easier to break into separate lines, so maybe that is why we have ended up with this difference.