Closed (works as designed)
Project:
Coder
Version:
8.x-3.x-dev
Component:
Coder Review
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
6 Oct 2012 at 23:54 UTC
Updated:
26 Nov 2020 at 20:02 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
traviscarden commentedActually, the coding standards do state "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". This may not be desirable, but if not, the standards need to change first. For now, Coder is, in fact, correct, so I'm marking this "works as designed" until and unless the standards are changed. Thanks!
Comment #2
lars toomre commentedThanks for the clarification @TravisCarden.
Comment #3
liam morlandThe coding standard is referring to lines that contain only an array definition, not any line which contains an array definition as part of it. Lines like this can trigger the error if long enough:
It doesn't make sense to break this array declaration into multiple lines.
Comment #4
liam morlandComment #5
gappleI'm encountering this warning in two instances:
1. In a complex form, I'm altering
#parentsto remove wrapping elements from the structure of submitted values in form state. The array goes over 80 characters on the line, but breaking it into individual lines is awkward for this type of simple definition.2. Later when retrieving the form values during validation or submission, they are accessed by specifying the keys in an array which will be longer than 80 characters.
---
I think both of these instances could be addressed by only providing a warning for long arrays if keys are specified for the array, but that doesn't also address @Liam Morland’s example of an array passed to a function.
Comment #6
gappleThis patch adds a requirement to the warning that the array assigns at least one key as well.
e.g.
This would not produce a warning:
$variable = ['a', 'really', 'long', 'array', 'that', 'is', 'longer', 'than', 80, 'characters];
However this would have a warning:
$variable = [1 => 'a', 'really', 'long', 'array', 'that', 'is', 'longer', 'than', 80, 'characters];
---
For the case where an array is passed into a function, the warning only appears if the array contains more than three items (and with this patch, also has meaningful keys). In that case, I think either breaking the function call to multiple lines or assigning the array to a variable which is passed in to the function is reasonable.
Comment #7
joelpittet@gapple, This seems reasonable to me, worth a few comments in the code to explain it a bit better, that may be just me, first time reviewing a patch file without drupal coding standards applied to it, so strange!
Comment #8
liam morlandWhy does it matter if a key is assigned?
Comment #9
volkswagenchickTagging for upcoming contribution days.
Comment #10
volkswagenchickComment #11
pfrenssenThe current coding standards mention that all array declarations longer than 80 characters should be split over multiple lines: https://www.drupal.org/docs/develop/standards/coding-standards#array
So this is not a false positive but working as designed. This standard applies to all array declarations, there is no exception made for array declarations that happen to be inside other array declarations. Also there are no exceptions made for indexed arrays or associative arrays.
I can understand from the given examples that this is less than desirable in some cases where a simple array is declared inside an already heavily indented code block, but we cannot make this change inside Coder unless the formal coding standards are changed.
There are some ways this can be worked around: for example you can put the array declaration on a separate line, or add a comment above with the annotation
// @codingStandardsIgnoreLine.Comment #12
gappleI've created an issue against the coding standard: #3039007: Relax the rule that arrays may not span more than 80 characters
Comment #13
liam morlandI don't agree that the current coding standards require the current behavior. The section on function calls has no provision for breaking them into multiple lines. Coder's current behavior is inconsistent with this.
Reading this together with the array examples, it is reasonable to say that "the line declaring an array" refers only to lines that only declare an array and does not refer to array syntax which appears within a function call parameters. This point should be made explicit in the standard, but I don't think fixing Coder needs to wait on a change in the standard.
Comment #14
gappleI don't see a distinction between assigned arrays and directly passed arrays and think they should be treated equally, breaking up long and nested arrays where necessary.
There's no recommendation for splitting function calls to multiple lines in the Drupal coding standard, but it is not prohibited.
The PSR-2 standard is similar to the note for arrays - MAY be broken to multiple lines, in which case MUST only have one argument per line.
Comment #15
liam morlandFor function calls, it says "spaces between commas and each parameter". There is no allowance for a newline.
Comment #16
gapplePatch reroll
Comment #17
gappleThe exception for 2-element arrays was removed by #3153448: Why is a two-element array allowed to exceed the maximum line length?
Here's a re-roll of the patch.