Problem/Motivation

The ArraySniff LongLineDeclaration error counts the number of elements in the array, to avoid giving the message on single-element arrays. However, the "counting" of array elemnts is achieved by searching for the presence of a T_COMMA between the array opener and closer. This works OK most of the time, when the array contents are simple. However, the array can contain a single item which is a function call, and this produces a false postive.

Steps to reproduce

A real-life example from the Rules module is:

    if ($entity->hasTags()) {
      $details = $details . '<br />' . $this->t('Tags: @tags', ['@tags' => implode(', ', $entity->getTags())]);
    }

The array ['@tags' => implode(', ', $entity->getTags())] has only one element, so should be allowed to extend beyond the limit. But this line gets incorrectly reported by Drupal.Arrays.Array.LongLineDeclaration

Proposed resolution

When searching for the T_COMMA examine the nested_parenthesis information, to determine if the comma is really part of the array being tested.

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented

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

I have added a test data row to demonstrate the problem. I have also worked on the fix.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review

I have pushed the fixed sniff and PR 129 is ready for review. I also have one comment on a coding standard that I'm not sure the best way to fix.

klausi’s picture

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

To be honest I like that the example line fails Coder. You should not write that long lines with arrays at the very end. Much better formatting:

    if ($entity->hasTags()) {
      $details = $details . '<br />' . $this->t('Tags: @tags', [
        '@tags' => implode(', ', $entity->getTags()),
      ]);
    }

I have mixed feelings, your change makes sense from a correctness perspective :)

jonathan1055’s picture

jonathan1055 commented

This standard is going to raise some opposing views - see #3185082: Drupal.Arrays.Array.LongLineDeclaration make me write less readable code

But yes, as the current sniff is not counting array elements correctly, and giving false warnings, even we don't like the example as given above, the sniff bug should be fixed, whcih is what I have done. Hopefully it can be reviewed and committed soon, as quite a few folks will be wanting to use the accurate version.

  • jonathan1055 authored e46990e on 8.3.x 
    fix(Array): Incorrect counting of array elements by comma (#3183715)
klausi’s picture

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

Right. Thanks, merged!

jonathan1055’s picture

jonathan1055 commented
Assigned: jonathan1055 » Unassigned

Thank you.
Unassigning.

Status: Fixed » Closed (fixed)

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