Problem/Motivation
Splitting #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines as suggested #1539712-92: [policy, no patch] Coding standards for breaking function calls and language constructs across lines and agreed at a #3282227: Coding Standards Meeting 2022-06-07 2100 UTC
There is lengthy discussion in that issue that is not duplicated here, it would be a good idea to look at that. The result of that was the following options.
Steps to reproduce
Proposed resolution
Option A from PSR-2 += as at #1539712-68: [policy, no patch] Coding standards for breaking function calls and language constructs across lines and in the Issue summary of that issue.
-
When calling functions, argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.
$foo = foo_function( $term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Translate some message here; good times.'), t('Some other message here.'), [ 'foo' => 'thing', 'bar' => 'thing2', ], $bar ); -
Values in an options-style associative array with more than one value SHOULD be wrapped onto multiple lines; e.g.:
$result = format_string('@foo at some point before had the value of @bar', [ '@foo' => $foo, '@bar' => t('The Bar in @foo', [ '@foo' => $foo, ]), ]); -
A nested call to a callable returning an object with chainable methods SHOULD be placed on the same line, using the regular coding style for chained method calls on the new indentation level; e.g.:
$value = db_select('users') ->fields('users', ['uid']) ->condition('uid', $account->uid, '<>') ->condition(db_or() ->condition('mail', db_like($form_state['values']['mail']), 'LIKE') ->condition('name', db_like($form_state['values']['mail']), 'LIKE') ) ->range(0, 1) ->execute() ->fetchField(); -
Opening and closing parentheses MUST use the same level of indentation across multiple lines.
Option B from from #1539712-68: [policy, no patch] Coding standards for breaking function calls and language constructs across lines and in the Issue summary of that issue.
- Function call parameters MAY wrap multiple lines, with each parameter on its own line. This MAY increase code readability when the length of individual paramters is very long, especially when the function call includes other function calls or arrays. For example:
$foo = foo_function( $term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Translate some message here; good times.'), t('Some other message here.'), [ 'foo' => 'thing', 'bar' => 'thing2', ], $bar );Ie. Parameters that contain natural language strings, references to values deeply nested within arrays, inline/"options-style" definitions of arrays or other parameters over 30 characters long are often good candidates to be considered "very long" parameters.
Developers SHOULD focus on the readability of the logic of the entire code block though, and take into account how common the parameters of the called function are. As an example for the latter, which is passing forward the passed-in arguments only:
file_field_load($entity_type, $entities, $field, $instances, $langcode, $items, $age);Function calls MUST either wrap all parameters or none. Do NOT do this:
$foo = foo_function($term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Message with %arg1 and @arg2', [ '%arg1' => $bar, '@arg2' => $baz, ])); - Values in an options-style associative array that contains more than one value SHOULD be wrapped onto multiple lines; e.g.:
$result = format_string('@foo at some point before had the value of @bar', [ '@foo' => $foo, '@bar' => t('The Bar in @foo', [ '@foo' => $foo, ]), ]); - Parameters of nested function calls, MAY be wrapped onto multiple lines, and MAY keep the first parameter on the same line; e.g.:
$this->assertEqual( $article_built[$field_name]['#items'][0]['fid'], $default_images['field_new']->fid, format_plural($count, 'An existing article node without an image has the expected default image file ID of @fid.', '@count existing article nodes without an image have the expected default image file ID of @fid.', [ '@fid' => $default_images['field_new']->fid ] ) ); - A nested call to a callable returning an object with chainable methods SHOULD be placed on the same line, using the regular coding style for chained method calls on the new indentation level; e.g.:
$value = db_select('users') ->fields('users', ['uid']) ->condition('uid', $account->uid, '<>') ->condition(db_or() ->condition('mail', db_like($form_state['values']['mail']), 'LIKE') ->condition('name', db_like($form_state['values']['mail']), 'LIKE') ) ->range(0, 1) ->execute() ->fetchField(); - Opening and closing parentheses MUST use the same level of indentation across multiple lines.
Option C from from #1539712-69: [policy, no patch] Coding standards for breaking function calls and language constructs across lines and in the Issue summary of that issue.
As defined above but all multi-line structures, whether nested or not, must have each parameter/element each starting a new, indented line.
It is worth noting that this is essentially equivalent to what PSR and PEAR say about multi-line function calls and what PEAR
says about multi-line if statements.
Original examples:
$foo = foo_function(
$term_1[$lancode][$delta]['value']['tid'],
$term_2[$lancode][$delta]['value']['tid'],
t('Translate some message here; good times.'),
t('Some other message here.'), [
'foo' => 'thing',
'bar' => 'thing2',
],
$bar
);
$this->assertEqual(
$article_built[$field_name]['#items'][0]['fid'],
$default_images['field_new']->fid,
format_plural($count,
'An existing article node without an image has the expected default image file ID of @fid.',
'@count existing article nodes without an image have the expected default image file ID of @fid.', [
'@fid' => $default_images['field_new']->fid
]
)
);
$result = format_string('@foo at some point before had the value of @bar', [
'@foo' => $foo,
'@bar' => t('The Bar in @foo', [
'@foo' => $foo,
]),
]);
if (!isset($file)
// If no core compatibility version is defined at all, then it is
// impossible to check for updates.
|| !isset($file->info['core'])
// If the extension is not compatible with the current core, it cannot
// be enabled in the first place.
|| $file->info['core'] != DRUPAL_CORE_COMPATIBILITY
// If the new version requires a higher PHP version than the available,
// then it is not possible to update to it. The PHP version property
// defaults to DRUPAL_MINIMUM_PHP for all extensions.
|| version_compare(phpversion(), $file->info['php']) < 0) {
return TRUE;
}
Alternative examples:
$foo = foo_function(
$term_1[$lancode][$delta]['value']['tid'],
$term_2[$lancode][$delta]['value']['tid'],
t('Translate some message here; good times.'),
t('Some other message here.'),
[
'foo' => 'thing',
'bar' => 'thing2',
),
$bar
];
$this->assertEqual(
$article_built[$field_name]['#items'][0]['fid'],
$default_images['field_new']->fid,
format_plural(
$count,
'An existing article node without an image has the expected default image file ID of @fid.',
'@count existing article nodes without an image have the expected default image file ID of @fid.',
[
'@fid' => $default_images['field_new']->fid
]
)
);
$result = format_string(
'@foo at some point before had the value of @bar',
[
'@foo' => $foo,
'@bar' => t(
'The Bar in @foo',
[
'@foo' => $foo,
]
),
]
);
<?php
if (
!isset($file)
// If no core compatibility version is defined at all, then it is
// impossible to check for updates.
|| !isset($file->info['core'])
// If the extension is not compatible with the current core, it cannot
// be enabled in the first place.
|| $file->info['core'] != DRUPAL_CORE_COMPATIBILITY
// If the new version requires a higher PHP version than the available,
// then it is not possible to update to it. The PHP version property
// defaults to DRUPAL_MINIMUM_PHP for all extensions.
|| version_compare(phpversion(), $file->info['php']) < 0
) {
return TRUE;
}
?>
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork coding_standards-3301214
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
quietone commentedComment #3
quietone commentedComment #4
quietone commentedChange array syntax.
Comment #5
arkener commentedShould we also include a requirement for trailing commas after the last parameter in a multiline function call? The same arguments that were made for the requirement of trailing commas on arrays can be made here, namely the fact that trailing commas will
prevent parsing errors if another parameter is added later and the reduction of diff complexity.
For example, the following will require a change on line 3 and 4 when a new service is added.
Core currently has ~100 implementations of trailing commas on function calls and ~1.500 implementations without.
Comment #6
jwilson3#5++ for simpler patchfiles and diffs alone.
Comment #7
drunken monkeyOption A sound good to me, though I don’t quite see the difference between the three. Is it just in the phrasing? In that case, definitely option A, option B has a number of problems in my opinion. (And I don’t quite understand what the difference is for option C.)
#5++
I don’t think the potential parsing errors are a good argument, but the diffs definitely are.
We’d just have to include the condition that this only applies if the code in question depends on PHP 7.3+ – which Drupal 7 code does not always do.
Comment #8
donquixote commentedI think we can forget about Drupal 7 now :)
Although I don't mind having a note "trailing commas in function calls are available since php 7.3".
I was looking for an issue about this trailing comma, I fully support it.
As for the options proposed in the issue summary: I prefer option A, but maybe we can reduce the scope to only function arguments, not chained calls. The nested array argument could also be in a follow-up.
Comment #10
donquixote commentedFirst attempt at an MR, just to start somewhere.
Comment #11
donquixote commentedWe could add something about nested calls and expressions where some levels are multi-line and others are not.
This would be more generic than the proposed section about options-style arrays.
I am not sure we allow this.
EDIT: But this maybe?
or
EDIT: If we don't want to allow this, we can say that an argument in a single-line function call cannot be broken to multiple lines, unless it is an associative array. Or that only one argument in a single-line function call can be broken to multiple lines.
But we can also just leave this as follow-up.
Comment #12
drunken monkeyI would say that
should be allowed, but definitely not your first example. Another way to allow this would be to say that only the last argument can be split into multiple lines (if it’s an array or function/method/constructor call). (I don’t think continuing with the arguments after the array is a good idea, though I have admittedly done this in test files.)
In any case, we should then probably also specify that the argument’s opening and closing brackets/parentheses should not be preceded resp. followed by a newline. That is, we don’t want this (I’d say):
If necessary, however, I’d rather forbid both of your former examples than allow your first example, in case we cannot manage to define this without it becoming unreadable.
The last one I’d definitely want to allow, though I admit it also is tricky to properly define. I guess what we are saying with that is that a nested call/array can be multi-lined even if the enclosing structure isn’t if it is the only argument/element? For the reverse, it should of course (I’d say?) also be fine to have single-lined calls inside a multi-lined call:
Maybe easier to guide by examples than to explicitly define in words what is or isn’t allowed?