Drupal.Arrays.Array.LongLineDeclaration make me write less readable code

The current implementation also triggers an error on lines like:

$this->assertEquals($settings['max'], $test_settings['max'], new FormattableMarkup('Correct maximum setting %value found for the %field_type field type', ['%value' => $settings['max'], '%field_type' => $field_type]));

Seems like it requires something like that:

$this->assertEquals(
  $settings['max'], $test_settings['max'],
  new FormattableMarkup(
    'Correct maximum setting %value found for the %field_type field type',
    [
      '%value' => $settings['max'],
      '%field_type' => $field_type,
    ],
  )
);

which is overkill IMO

As someone who has worked on the the array sniffs recently, I am happy to have this discussion.

@alexpott, @Taran2L, two questions:

1. What version of Coder are you using?
   
   • Core testing on drupal.org at 8.9, 9.0 and 9.1 all use Coder 8.3.9 (released 8 May)
   • Coder 8.3.10 (released 5 Sep) has a change to the inline array sniff which previously ignored two-element arrays, so this is a reason for geting more coding warnings than before - see #3153448: Why is a two-element array allowed to exceed the maximum line length?
   • Coder 8.3.11 contains a fix for a problem (introduced by me in the above issue) which used the full line length not the array ending position. This is fixed in #3169452: Use $parenthesisCloser column to correctly determine inline array length
   • I have also fixed another bug in the array length sniff (not introduced by me this time) which incorrrectly counted the number of array elements - #3183715: Incorrect counting of nested inline array items for linelength sniff. This has not been committed to dev yet
2. Please could you upload a short .txt file with your examples, so that we can be sure we are looking at exactly the same source, in context. Pasting code snippets to the issue, even within <code> tags does not guarantee the column positions or line returns when I want to use that example to work on the sniff.

Thanks.

Re #2 we are trying to remove assertion messages where they don't add value, see #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages

Probably 140 chars will need policy issue for coding standards...

Personally -1 on increase a limit because it makes code harder to read specifically in shell/terminal (when you trying to quickly hack/fix something remotely)

@alexpott

We don't have a rule that says a single line of code should be 80 chars or less so why do why enforce that here. It is a recommendation ...

That is a very good question, and one that I have often also wondered. For reference here is the Drupal standard on code line length and Inline commenting.

+1 for making this less strict.

For an example of how adhering to the current rule will IMHO get in less readable code, #3184864: Fix code style standards.

The issue with the 80 character limit is that it's an highly opinionated standard.

For example, I agree that the following code should be allowed to span 1 line:

if (!in_array($form_object->getOperation(), ['add', 'edit', 'default', 'somethingelse'], TRUE)) {
  return;
}

While I don't agree that the example given in #2 should be allowed to span 1 line, as it would increase the readability of the code to seperate it over multiple lines (in my opinion).

I'm giving a +1 to increase the line limit to 120/140, which might impact readability on smaller screens, but would greatly increase readability on default screens.

In regards to if this rule should be recommended / required (Drupal / DrupalPractice), we should propably clarify this via a coding standards (documentation) change request, as the current definition leaves allot to interpretation.

To see the effects of increasing the default limit on core files, I have done some analysis on #3116859-25: [meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard comments #25 - #30.

I would support an increase to 120, as that works OK with regular laptops. 140 is too long for the default IMO. 120 would fix 60% of the current core problem lines.

I have no strong opinion if we should use a limit of 80 or 120, I can see both working fine for me. As for the example in #13 there are multiple ways to split that line up to make it more readable, maybe:

if (!in_array(
  $form_object->getOperation(),
  ['add', 'edit', 'default', 'somethingelse'],
  TRUE)
) {
  return;
}

That way there is a strong emphasis on the last TRUE parameter which would fall out of sight because it is way over 80 characters otherwise.

Or:

$allowed_operations = ['add', 'edit', 'default', 'somethingelse'];
if (!in_array($form_object->getOperation(), $allowed_operations, TRUE)) {
  return;
}

Much better! I'm not sure why people want to cram all the things into if conditions. Shorter lines almost always mean more readable code. Don't do your array definitions within the if() clause. If it goes over 80 characters then that might mean your array definition is too long.

I tend to like the current 80 limit for line length more, but would yield to popular opinion if more people want to go for the 120.

I would agree that 120 makes sense. Code I had triggering the error was at 93:

 159 | ERROR | The array declaration extends to column 93 (the limit

     |       | is 80). The array content should be split up over

     |       | multiple lines

// Set values for the primary Content Type Facet.
if ($content_type_facet_field && in_array($entity_type, ['entity:node', 'entity:media'])) {
  $this->setContentTypeFacetValues($item, $entity_type, $content_type_facet_field);
  }

I changed it to

// Set values for the primary Content Type Facet.
if ($content_type_facet_field && in_array($entity_type,
  ['entity:node', 'entity:media'])) {
  $this->setContentTypeFacetValues($item, $entity_type, $content_type_facet_field);
  }

Not much difference in the readability between the two for me in VS Code, but 120 would give a bit more flexibility particularly to those who inherited their custom code from a vendor and are making these changes during their D7/D8 upgrade to D9. As an example, I have 30+ of these to fix but none exceeded 120. Most were between 90 and 110.

From the previously mentioned #3039007: Relax the rule that arrays may not span more than 80 characters, accessing a nested value from form state which takes 100+ characters:

class {
  function () {
    foreach (...) {
//       |10       |20       |30       |40       |50       |60       |70       |80       |90       |100
      $reportHandlerOptions = $form_state->getValue([$policyTypeKey, 'reporting', $reportHandlerPluginId]);
    }
  }
}

I don't feel there's a readability improvement from breaking it up to one item per line according to the standard to stay within 80 characters:

class {
  function () {
    foreach (...) {
      $reportHandlerOptions = $form_state->getValue([
        $policyTypeKey, 
        'reporting', 
        $reportHandlerPluginId
      ]);
    }
  }
}

I agree with the general sentiment here: this rule does not achieve the goal of making people write more readable code, but it can contribute to the creation of less readable code.

I've personally always adapted the strategy outlined by @klausi in #3185082-16: Drupal.Arrays.Array.LongLineDeclaration make me write less readable code but in the cases I am forced to go this way it would almost always be more readable to just keep it inline. It's frustrating especially if it happens to very innocent uses such as calls to $this->t() with a longish string and an arguments array.

As others also have done, I reviewed the coding standards regarding line length (https://www.drupal.org/docs/develop/standards/php/php-coding-standards#l...) and there is nothing there that requires arrays to be split up over multiple lines. It even explicitly allows lines to exceed 80 characters in these cases:

• Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 characters.
• Control structure conditions may exceed 80 characters, if they are simple to read and understand.

I think this covers many cases where we see inline arrays being used. They are often in variable declarations or when starting a control structure like a foreach.

Regarding demoting this from an error to a warning, or moving it to DrupalPractice: a motivation for this would be because it historically is part of Coder and people got used to writing their code in a way that satisfies this rule. There is also the sunk cost aspect: we spent a lot of effort to create and maintain this sniff, so let's keep it around in some form. But if I take a step back and try to look at it objectively: it's hard to come up with a justification that splitting up a line in the middle just because it contains an inline array at that point is some best practice. Yes, keeping lines short is absolutely a good practice, but this rule is not a good way to try to enforce that.

I think changing the line length overall from 80 to 120 or another length is out of scope for this issue since this has much wider implications, let's keep the scope here on Drupal.Arrays.Array.LongLineDeclaration. Changing the overall line length is something to discuss in the coding standards issue queue.

Final thoughts: I think the main factor is that this sniff is not based on an actual requirement in the coding standard. It was added a long time ago with the best intentions when Drupal code was much more compact, but nowadays it causes more harm than good. It causes frustration rather than being helpful.

I think the simplest and best solution would be to just remove this rule.

@pfrenssen: I absolutely agree to that.

As there is no actual coding standard that insists on breaking lines at 80 then I also agree we should just drop this rule. I suppose it would be good to discuss it with the Coding Standards team, because there is now a new agreed process for getting standards changed. Although actually, there is no standard for this. So maybe we can just drop it? But it will need a Core issue, as currently the rule is mentioned (excluded) in phpcs.xml

  <!-- Drupal sniffs -->
  <rule ref="Drupal.Arrays.Array">
    <!-- Sniff for these errors: ArrayClosingIndentation, ArrayIndentation, CommaLastItem -->
    <exclude name="Drupal.Arrays.Array.LongLineDeclaration"/>
  </rule>

A new issue template for approving changes to standards is being developed on #3387167: Add an issue template for the Coding Standards project. So I think it would be useful to trial that here, hence I have modifed the issue summary accordingly. Anyone else is welcome to make further amendments.

If this is correct route to go, then this issue should be moved into the Coding Standards project issue queue so it can be reviewed and approved, before any action is taken on Coder sniffs.

Thanks @jonathan1055 but this process is intended for making changes to the coding standards documentation, that is not applicable in this situation. The rule were planning to change is not covered by the coding standards.

I think getting a third sponsor in is a great idea though. I would love for @klausi to sign off on it.

On a side note I have posted about this potential change on our company Slack to get an idea of how our devs feel about it and the response was unanimously in favor. Looks like this change is not going to be too controversial.

I have overlooked this, but there is in fact a coding standard about this, it is in PHP coding standards: Arrays:

Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level

Thanks a lot @pfrenssen for digging out the relevant coding standards. I updated the issue summary to explicitly say that we have this currently in the coding standards.

I was almost ready to sign off on dropping this rule, but now that I see it in our written coding standards I'm neutral again and reluctant.

We can be bold in Coder and make changes ahead of any coding standard changes (especially getting more lenient), so if there is still enough support I can be convinced. I raised the required supports in the issue summary to 5 now, any more supporters?

But I have another idea: what if we drop the rule only in if() statements and function() calls? That seems to be the biggest pain point for people in this issue. We keep the rule for arrays defined in normal statements (like form arrays) - you really should split up your lines there. I think that would be in the spirit and intention of the coding standard we have.

My comments from #19 and #23 are invalid since I was under the impression that this was not included by the coding standard but it is in fact.

I think we should abolish that section from the coding standards. There is already an issue for it: #3039007: Relax the rule that arrays may not span more than 80 characters.

Core never adopted this sniff, presumably because it results in such ugly and hard to read code. I think we should allow and encourage people to write their arrays in the way they want, and just give a recommendation to not put complex arrays in a single line of code, without enforcement.

I personally would not keep the sniff around in any form because it forces to use a hard to read format. It's just counterproductive. I posted an example in #3039007: Relax the rule that arrays may not span more than 80 characters of an array definition I found in \Drupal\locale\Form\TranslateEditForm::buildForm() which is a nice combination of being compact and easy to read. It hits exactly 81 characters so our sniff would need to be applied:

$form['strings'] = [
  '#type' => 'table',
  '#tree' => TRUE,
  '#language' => $langname,
  '#header' => [
    $this->t('Source string'),
    $this->t('Translation for @language', ['@language' => $langname]),
    $this->t('Delete'),
  ],
  '#empty' => $this->t('No strings available.'),
  '#attributes' => ['class' => ['locale-translate-edit-table']],
];

Applying the sniff results in this array definition which is so much worse:

$form['strings'] = [
  '#type' => 'table',
  '#tree' => TRUE,
  '#language' => $langname,
  '#header' => [
    $this->t('Source string'),
    $this->t('Translation for @language', [
      '@language' => $langname,
    ]),
    $this->t('Delete'),
  ],
  '#empty' => $this->t('No strings available.'),
  '#attributes' => [
    'class' => [
      'locale-translate-edit-table',
    ],
  ],
];

Yes, but we want to throw errors when beginners write the array like this without line breaks:

form['strings'] = ['#type' => 'table', '#tree' => TRUE, '#language' => $langname,'#header' => [$this->t('Source string'),$this->t('Translation for @language', ['@language' => $langname]), $this->t('Delete')], '#empty' => $this->t('No strings available.'), '#attributes' => ['class' => ['locale-translate-edit-table']]];

That's is bad and violates the coding standards we use in the Drupal community, the practically used ones and the rule we have written down.

That makes me circle back to the idea that a simple increase to 120 characters should cover all the example cases that have been brought up in this issue.

I will try to add them all to our tests and increase to 120, let's see how that goes.

The 120 limit seems to work fine and gives plenty of space, while still catching beginners that write way too long array definitions on one line.

PR: https://github.com/pfrenssen/coder/pull/210

Let me know if you have any comments on that change, I plan to merge it in the next days.

Merged that one, thanks everybody for the input!

Thanks a lot, this will already alleviate most frustration!