In coder_sniffer/Drupal/Sniffs/Arrays/ArraySniff.php line 112 we have

if ($isInlineArray === true) {
  // Check if this array contains at least 3 elements and exceeds the 80 
  // character line length.

So what is the reason for an array with two elements being allowed to extend beyond the specified limit? I get it if the 2nd element starts before the limit and extends beyond, but that is not what's being checked for here. I must be missing something.

Secondly, a minor point, following https://git.drupalcode.org/project/coder/commit/0b2dc22 when next changing that file the comment should refer to $this->lineLimit instead of 80.

Set the array line limit for a project

The default (and Drupal standard) array line length is 80 characters. This is independent of the general maximum line length, and can be set for all files by using the project's phpcs.xml.dist file. The example below sets the general line length to 100 but allows arrays to extend to 120

<rule ref="Drupal.Files.LineLength">
    <properties>
      <property name="lineLimit" value="100"/>
    </properties>
  </rule>
  <rule ref="Drupal.Arrays.Array">
    <properties>
      <property name="lineLimit" value="120"/>
    </properties>
  </rule>

Override the array line limit for a file

To override the setting in a specific file, add this to the file document header block. This example increases the array limt to 140.

* phpcs:set Drupal.Arrays.Array lineLimit 140

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented

The coder message given is

If the line declaring an array spans longer than 80 characters, each element should be broken into its own line
(Drupal.Arrays.Array.LongLineDeclaration)

So if the rule is correct to only apply for 3-element arrays and above, the message could be altered to say that. Otherwise you get confusion.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

No particular reason that I remember, so we can also apply this to 2 element arrays.

jonathan1055’s picture

jonathan1055 commented

Created PR 114 to deal with this. First the test data change, then the sniff change.
https://github.com/pfrenssen/coder/pull/114

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review

The PR is ready for review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Category: Support request » Bug report
Status: Needs review » Needs work

Thanks, left some comments on the pull request.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Hi @klausi,
I have updated PR 114 with the changes you asked for, and answred your questions.

  • jonathan1055 authored 7375694 on 8.x-3.x 
    fix(Array): Detect line length issues with 2 element arrays (#3153448 by...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Merged, thanks a lot!

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

I think somewhere we should document explicitly document how to set the overall array line length per project in phpcs.xml.dist and how to override this setting per file. I have updated this issue summary with the details. Does this need to copied elsewhere too?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I don't think so - people shouldn't change this setting as it then would violate the Drupal coding standard. We should not encourage that.

So I think leaving this here in an issue is enough for advanced users to find.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

OK, yes that's fine. Core 8.9 and 9.0 currently exclude this sniff anyway. There has been some general talk (not just within Drupal) that the 80-char limit is quite dated now, and was set when we all had much smaller monitor screens. So I would not be surprised if some project maintainers would happily chose to increase the limit. I have modified the issue summary slightly.

Status: Fixed » Closed (fixed)

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

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented

Hello, I think that this particular change creates a lot of false-positive warnings.

A few examples

The following line:

$actual = $plugin->transform([[], [], $field_settings], $this->createMock(MigrateExecutableInterface::class), $this->createMock(Row::class), NULL);

generates

FILE: range/tests/src/Unit/Plugin/migrate/process/d6/RangeFieldInstanceSettingsTest.php
--------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------------
 25 | ERROR | The array declaration line has 151 characters (the limit is 80). The array content should be split up over multiple lines
--------------------------------------------------------------------------------------------------------------------------------------------------

Similar issue with lines like the following:

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

Is it expected behavior?

jonathan1055’s picture

jonathan1055 commented

Hi Taran2L,
I think the problems you describe have been fixed in #3169452: Use $parenthesisCloser column to correctly determine inline array length but this is currently only in the -dev release.
Jonathan

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Coder 8.3.11 has been released with the fix there!