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

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 →

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!

Automatically closed - issue fixed for 2 weeks with no activity.