[policy, no patch] Coding standards for breaking function calls and language constructs across lines

Expanding scope to reflect the current discussions in the cleanup issues.

My personal view is that we shouldn't break function *declarations*.

But I'm not sure about the rest... What exactly do you mean by "language constructs"? I think we already have some policies on the coding standards page about breaking up if() statements, right?

Or is this just about arrays within these statements?

I'd say about multiline formats in general, including arrays within other calls in particular. What we have so far about when not to wrap is only:

Conditions should not be wrapped into multiple lines.
...
Instead, it is recommended practice to split out and prepare the conditions separately, which also permits documenting the underlying reasons for the conditions[.]

There is no comment about wrapping with regard to function declarations, function calls, flow control constructs like while or foreach, etc.

I'd propose:

• Function declarations should never be wrapped onto multiple lines.
• Amend "conditions should not be wrapped onto multiple lines" to "Language constructs like if, while, and foreach should never be wrapped onto multiple lines. E.g., do this:
  

  $numbers = array(1, 2, 3, 5, 7, 11, 13, 17, 19, 23);
  foreach ($numbers as $n) {
    drupal_set_message(t('%number is prime!', array('%number' => $n)));
  }
  

  Never this:

  foreach (array(
    1,
    2,
    3,
    5,
    7,
    11,
    13,
    17,
    19,
    23,
    ) as $n) {
  

• Function calls may be wrapped to multiple lines, with each parameter on its own line, similar to the multiline array format. This is recommended when the line length exceeds 80 characters, especially when the function call includes other function calls or arrays.

$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.'),
  $bar
);

$this->assertEqual(
  $article_built[$field_name]['#items'][0]['fid'],
  $default_images['field_new']->fid,
  format_string(
    'An existing article node without an image has the expected default image file ID of @fid.',
    array('@fid' => $default_images['field_new']->fid)
  )
);

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'],
  t(
    'Message with %arg1 and @arg2'
    array(
      '%arg1' => $bar,
      '@arg2' => $baz,
    )
  )
);

$value = db_select('users')
  ->fields('users', array('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();

field_create_field(array(
  'field_name' => 'alter_test_text',
  'type' => 'text',
));

I meant to add, outstanding question I have is if/when/whether closing parentheses should be on their own line. I'd lean toward yes since that's our policy with curlies, but I can also see a case for condensing it when there's no confusion (e.g. the last several parens when there are no subsequent lines before the semicolon).

I like #4. It is sensible and readable.

RE #5, I think all closing parens can be together on the last line, but they should be on their own line rather than put after the last item on the previous line.

Any decision on things like this:
[function file_copy in file.inc]

<?php
watchdog('file', 'File %file (%realpath) could not be copied because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', array('%file' => $source->uri, '%realpath' => $realpath, '%destination' => $destination));
?>

should it be:

<?php
watchdog('file', 'File %file (%realpath) could not be copied because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', array(
  '%file' => $source->uri,
  '%realpath' => $realpath,
  '%destination' => $destination,
));
?>

[edit]

Or should it be:

<?php
watchdog(
  'file',
  'File %file (%realpath) could not be copied because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.',
  array(
    '%file' => $source->uri,
    '%realpath' => $realpath,
    '%destination' => $destination,
  )
);
?>

RE #7, I would vote for the last alternative of those three, I think, although if the first couple of parameters before the array were small, I might tend to prefer alternative 2.

RE #9 - to reach a final decision, we probably need some more comments by core developers...

Actually, in order to get comments easier, the next thing to do would probably be to make an issue summary.

I updated the issue summary so it contains the actual proposal from xjm.

And regarding comment #7, I realized this case is covered by the proposed standards -- the last choice in comment #7 would be consistent with the standard, and in the summary, that would be item #4 "Nested arrays or function calls ...".

Anyway, I'm going to ask for feedback in IRC, and let's see if we can reach a consensus.

+1 for the proposal in the summary and comment #4. Looks clean, sane, readable, and consistent to me.

Add standards proposal to summary

p.s. I just edited the proposal slightly to read:

Nested arrays or function calls should be formatted with each parameter or key on its own line, and each subsequent level indented by two spaces.

It used to say "... by one space" but that didn't match the examples, nor the rest of our coding standards. I hope that's a "friendly amendment" to the proposal. ;)

Cheers,
-Derek

RE #15 - good catch, yes 2 spaces would definitely be the standard. :)

Yep, #4 looks good to me. I definitely prefer the closing )'s to be on separate lines indented to the same level as the line with the opening (

As opposed to the closing being eg )));

It seems that phpcs only cares if an array within the line exceeds 80 chars not if the overall line length exceeds 80 chars. Is that correct? What if one line contains 2 arrays of 41 chars each?

We need to get this resolved -- there are a number of patches that are being held up pending this decision.

I haven't seen any "no" votes to the policy in the issue summary, so I am going to mark this tentatively RTBC, and if no one objects in a few days, I'll add it to the coding standards and we'll call it fixed. Sorry, we should have done that a while back -- this fell off my radar...

3 and 4 contradict our common coding practice throughout Drupal core; in particular these:

$this->assertEqual(
  $article_built[$field_name]['#items'][0]['fid'],
  $default_images['field_new']->fid,
  format_string(
    'An existing article node without an image has the expected default image file ID of @fid.',
    array('@fid' => $default_images['field_new']->fid)
  )
);

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'],
  t(
    'Message with %arg1 and @arg2'
    array(
      '%arg1' => $bar,
      '@arg2' => $baz,
    )
  )
);

Our actual coding practice and semi-adopted standard is this instead:

  format_string('An existing article node without an image has the expected default image file ID of @fid.', array(
    '@fid' => $default_images['field_new']->fid,
  ))

  t('Message with %arg1 and @arg2', array(
    '%arg1' => $bar,
    '@arg2' => $baz,
  ))

Which, in turn, leads to an adjustment of those more complex/nested examples:

$this->assertEqual(
  $article_built[$field_name]['#items'][0]['fid'],
  $default_images['field_new']->fid,
  format_string('An existing article node without an image has the expected default image file ID of @fid.', array(
    '@fid' => $default_images['field_new']->fid,
  ))
);

$foo = foo_function($term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Message with %arg1 and @arg2', array(
  '%arg1' => $bar,
  '@arg2' => $baz,
)));

The latter does not have to be wrapped into multiple lines. But if you choose so, then use the style of the former to write it.

I guess what this bascially hints at is a standard for "options" function parameters, which has been non-obvious thus far.

It especially becomes relevant when taking additional parameters after the options parameter into account. Typical example:

$foo = t('Message with %arg1 and @arg2', array(
  '%arg1' => $bar,
  '@arg2' => $baz,
), array('langcode' => LANGUAGE_NONE));

This is the coding style that we're using almost exclusively everywhere throughout Drupal core. And so I think it only makes sense to formalize it exactly like that.

It probably becomes relevant to note this, because other PHP projects like Symfony use a completely different coding style for such things, which in turn means that developers can easily get confused when looking at the "wrong" files in the search for code examples.

Furthermore, on 2):

It does not make sense to me to disallow the multi-line syntax for control structures. There are perfectly legitimate use-cases for that; in particular in case of complex code conditions.

Whenever I see such in patch reviews, I usually demand for inline comments that explain what is being checked and why. In turn, that provides some reasoning for allowing the multi-line syntax.

For example:

  if (!isset($file)
      || !isset($file->info['core'])
      || $file->info['core'] != DRUPAL_CORE_COMPATIBILITY
      || version_compare(phpversion(), $file->info['php']) < 0) {
    return TRUE;
  }

Alas, this code actually slipped into core without any explanation at all. If I had reviewed or authored it, it would probably look like this today:

  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;
  }

Note that these comments are made up; they may not be fully correct; didn't spend the time to check what this code actually does -- which in turn is the exact reason for why I encourage such inline comments.

Of course, you could say that "this can be written differently" to avoid the multi-line condition in the first place:

  if (!isset($file)) {
    return TRUE;
  }
  // If no core compatibility version is defined at all, then it is
  // impossible to check for updates.
  if (!isset($file->info['core'])) {
    return TRUE;
  }
  // If the extension is not compatible with the current core, it cannot
  // be enabled in the first place.
  if ($file->info['core'] != DRUPAL_CORE_COMPATIBILITY) {
    return TRUE;
  }
  // 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.
  if (version_compare(phpversion(), $file->info['php']) < 0)) {
    return TRUE;
  }

But as you can see, that leads to code duplication -- which may not matter in this case of simple Boolean return values, but can totally matter in other situations.

Of course, you can say that "even for those other situations this can be written differently" to avoid the code duplication:

  $valid = isset($file);
  // If no core compatibility version is defined at all, then it is
  // impossible to check for updates.
  $valid = $valid && isset($file->info['core']);
  // If the extension is not compatible with the current core, it cannot
  // be enabled in the first place.
  $valid = $valid && $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.
  $valid = $valid && version_compare(phpversion(), $file->info['php']) >= 0);
  if (!$valid) {
    return TRUE;
  }

However:

It is my strong belief that coding standards only exist to standardize how code should be written, if it gets written in a certain way. Coding standards should have no business in telling me which code logic I should use. The only exception to that are platform-specific or technical/performance related rules (which are rare in the first place).

In other words: If it makes more sense for my code to use a multi-line syntax, then I should be free to do so. The coding standards should be there to tell me how to write the multi-line syntax in that case. (They are not here to tell me that I'm not allowed to write my code that way.)

Comment #21 -- I agree completely.

Comment #20 -- I am not sure I agree. In particular, this example:

$foo = foo_function($term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Message with %arg1 and @arg2', array(
  '%arg1' => $bar,
  '@arg2' => $baz,
)));

This makes that first line extremely long... it seems like if you're going to break it up, it would be better to do as the issue proposal currently stands, which would be:

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'], 
  t('Message with %arg1 and @arg2', 
    array(
      '%arg1' => $bar,
      '@arg2' => $baz,
)));

Or maybe I misunderstood -- were you advocating this as a good example of how to write code, or a bad example?

Maybe you can rewrite the proposed standards so it is clear what you are advocating to use as a standard?

FWIW, I also disagree with the defacto "standard" in #20. I think that's much less readable than the new proposal. I'd rather adopt a better standard and then have to clean up some places than adopt a more clumsy standard just because that's what we mostly sorta do now.

If you want to wrap, then you're free to do so, but without the unnecessary verbosity for the array( declaration; i.e.:

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'],
  t('Message with %arg1 and @arg2', array(
    '%arg1' => $bar,
    '@arg2' => $baz,
  )),
);

I.e., only affecting the "options-style array" parameter for function calls (of which we have many and will likely have many more in the future).

At least from my perspective and experience, I can say that we're using this style after trying others, because it is the right balance between clarity and readability and verbosity and indentation-level-nightmare.

I will never ever write any code that uses an inane level of verbosity and indentations like this [copied from the summary] (even if you'd put it into the standards):

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'],
  t(
    'Message with %arg1 and @arg2'
    array(
      '%arg1' => $bar,
      '@arg2' => $baz,
    )
  )
);

That being said, I have a not so ungrounded fear that people are going to run over all kind of core patches to set them to needs work, just because some functional code lines are exceeding 80 chars.

Our coding standards explicitly state that the 80 chars rule only applies to code comments, so this must not happen under any circumstances, and we better carefully clarify that these rules only apply if you write code that wraps.

And now that I check the actual proposed wording in the summary, it actually attempts to do exactly that :( -- 3. "recommends" wrapping function arguments when a line exceeds 80 chars.

So, next to aforementioned argument, this is also wrong, because the actual reason for recommending to wrap anything is to make the code more readable and understandable — which can be happily omitted, if the parameters passed to another function are "common sense" or otherwise clear already.

Furthermore, a limit of 80 chars for functional code is, in terms of real and actual Drupal code, way too narrow. You need to consider that function names like drupal_array_unset_nested_value() already consume 50% of 80 chars and that's only the bare and plain function name. And if that doesn't convince you, then just have a look at these 88 chars:

  file_field_load($entity_type, $entities, $field, $instances, $langcode, $items, $age);

So if we really need or want to place a number there, then at least based on my experience, 110 chars are much more realistic and compatible with Drupal.

However, even with 110, it does not always make sense to wrap a line of functional code, because when optimizing the readability of functional code, you should focus on the readability of its logic, so other programmers can understand it quicker. And yes, I fully acknowledge that this requires human sense and evaluation (which ultimately is one of the main points that make the difference between senior and junior programmers). But whether a machine or code parser warns about not wrapping lines or not is totally and absolutely irrelevant.

Lastly, this will also trigger confusions about wrapping strings in arguments (which we already see in the code base today), so it should be clarified that string arguments should not be wrapped. (Not sure whether there are exceptions to that rule though.)

This issue is holding up a major effort underway to bring all of core into Coder compliance for 8.x. Is it intractable or can we come to an agreement? sun, you seem to be the only one objecting to the last proposed standard, so can you propose a new one that you think we can all agree upon?

Agree with #25 - I don't mind how the code needs to be formatted, we just need a decision, this issue has really knocked the wind out of the Coder review code tidy up.

@sun: we love you, but you don't get veto power over stuff like this. We don't operate by consensus. One lone objection isn't enough to halt all progress. I say we move forward with the proposal in the summary. It's consistent, easy to teach text editors and IDEs to do for you, and already has a bunch of support in this issue.

Onward,
-Derek

BTW, I did just find a typo in the proposed standard [summary, item 4, second example]:

t(
    'Message with %arg1 and @arg2'
    array(

missing a comma at the end of that line. I'll fix the summary.

s/one space/two spaces/ to match the actual example code (and the rest of our coding standards)

Completely off topic from this discussion. Don't mean to bike-shed, but out of curiosity, I ran Drupal through the PHP Coding Standards Fixer. The result is actually surprisingly readable.

Main notes in the changes:

1. Class and class function declarations get a line break before the {
2. There's a line break before class function returns
3. else's are on one line since it's not a function declaration like #1

The most important thing about a coding standard is that it's consistent. It all simply comes down to just syntax, and if that syntax is consistent, nothing else matters.

Rob: yes, off-topic. I think if you want to suggest using a formatter, that should be a different issue entirely.

add missing comma (syntax error)

I thought I already provided sufficient and extensive reasons for why the original proposal won't work out. But well, forcing me to do a full rundown on a revised proposal now subtracted two hours from other core work. It's in the issue summary.

I've made sure to use W3C/RFC/spec terminology to separate rules from suggestions, recommendations, and options (which we generally should do some more in our coding standards).

Sorry, just realized that #33 could be interpreted wrongly - I meant that my time is currently much better spent with working on the CMI architecture, which blocks a large range of issues. It has been stated here that the lack of this standard here would "block" some issues, but I don't really understand why and how that is possible; we've been able to write code in a sensible way in the past, so I don't get why we're trying to rush this coding standards addition in so quick.

Also, since any new hard boundaries being defined here would affect everyone writing code for Drupal, some more people should weigh in.

@sun:

• I respect (and share) your desire to be perfect, and have tremendous gratitude for all that you contribute to core development.
• I think it's great you added MAY, SHOULD, etc terminology. +1 to that approach for defining our standards.
• You're free to spend your time however you want. If you think it's worth 2 hours to argue about this, that's your "fault", not ours. ;) Don't blame anyone else in here that CMI (and therefore, all of core) is being held up because of this. It's being held up b/c that's how you're choosing to spend your time.
• We're not "rushing" this in just for the fun of it. This issue has been open for discussion for 2 months (and counting). People are trying to clean up some code standards compliance, and they're hitting a place that we don't have a standard for. Lots of people work on all sorts of aspects of core for many different reasons. In addition to our obsession with perfect code and standards-compliance, code standards changes are generally good novice tasks. We want to encourage novices to start contributing so that they get used to how we get things done, and can then "graduate" to more meaningful contributions. There are meta reasons why unblocking those people is important, even if the actual reason (getting our code wrapped consistently) doesn't seem as important as the itches you're currently scratching. And just because in 2 months no one else has agreed with you doesn't mean we're rushing this in. ;)
• I think it's pretty lame that you inserted your proposal at the top of the summary as if it's the main proposal, when in fact, you're the only one arguing for that. But, I don't want to get in a pushing match over it, so I'm just going to tell you how I feel and let you (or someone else) choose to reorder them and provide a little more context.
• You're heavily contradicting yourself in comment #34. You're simultaneously saying this is a major change that will effect everyone writing Drupal code (clearly hyperbole), and also saying that this is a minor edge case so there's no rush for having a standard here since we've managed to survive without a standard for 10 years and it doesn't really matter. Frankly, I think you're just upset that no one else supports your proposal, and you're trying to argue both that there's no cost in waiting for you to build more support for it, and to predict fire and brimstone if we adopt a proposal you don't like. That's not a fair argument, and I'm hereby calling you on it. ;)

That said, I'm going to choose to not spend a ton of time arguing with you over the details of our differences of opinion on what makes the code look more pretty. That's all that's happening here -- there's nothing objectively quantifiable to help us pick a given standard for this. It's just subjective opinion, and extremely bright people can have differences of opinion over things like this. But, for the record, I'm still -1 to "#20 + #21" (if that's what we have to call it), and +1 to "#4".

Cheers,
-Derek

1. "Coding standards" are standards. They take effect as soon as they are in the docs.
2. Patches that do not comply with coding standards can be marked as needs work. An addition or change to coding standards immediately affects all patches in the queue.
3. There are only 11 people subscribed to this issue. It's ridiculous to talk about any general agreement or disagreement on behalf of "all" already, especially as it seems that people weren't able to distill the actual differences of later follow-ups.
4. I've driven and participated in many coding standards issues in the past and can assure that most of them took much much longer than eight weeks. Drupal coding standards are based on community consensus. They have to be, or they will not be adopted by developers, in which case they would miss their entire purpose.
5. It doesn't matter which of the proposals comes first. It would be great to get some more developers from core but also from contrib on this issue. Actually, since the "first" proposal mimics what current code looks like for the most part, and generally reduces the amount of hard rules vs. options/recommendations and defers to human sense instead, so others should be able to provide feedback on the proposals and details within more easily; i.e., a [undocumented-]before/after comparison to some extent.
6. The originally reported issue was only about:
   

   Function declarations MUST be written on a single line; they should never wrap multiple lines.

   If it helps to split that one out, then I'm fairly sure we can do so immediately. I don't think I've ever seen any Drupal code using multi-line function declarations in my life.

Bump... If a decision is made on this then we can get on with the coder review tomorrow at drupalcon :)

If not then could we just ignore this 1 formatting issue, complete all our patches and reopen any files for which this is blocking as separate issues?

matt

My main concern with the new proposal is that it's just way too much text and specific instructions. We want to define a best practice guideline with some simple rules (that happen to already be implemented in code sniffer), not need a flowchart for deciding when to wrap lines.

I also don't like the introduction of 110 characters. This doesn't appear anywhere else in our coding standards and seems arbitrary. "If over 80, split the array onto multiple lines for better scanability" is far more consistent with our current standards for arrays defined by themselves and for line length generally.

I actually quite like the idea of fixing everything except this 1 formatting issue.. then reopen issues for any files that are affected by it.

That way even if this issue is never resolved we've still fixed 90% of the errors

I wanted to find out who wrote this *really* nice issue summary, so I looked at the revisions tab, but the user and date are wrong because of #1217286: Posting a comment changes too many issue node revision properties which is postponed because it will be fixed when the d7 d.o migrate happens. The reason I wanted to know who/when wrote the summary was because I wanted to know what comments were made after the good summary, so which were pertaining to that. I'm guessing the last update to the summary was #1539712-33: [policy, no patch] Coding standards for breaking function calls and language constructs across lines (comment 33)
Posted by sun on June 19, 2012 at 8:15pm

The next step for this issue is to get more core developers to comment on the proposal in the summary. sphism and I will try and round up a few.

My 2c on #20 + #21, some small remarks
1/ Please don't add another boundary
5/ so a while with a complex condition has to be written on one line, code copied from 3. I agree on the rule in the case of for/foreach

  while (isset($file)
      || isset($file->info['core'])
      || $file->info['core'] == DRUPAL_CORE_COMPATIBILITY
      || version_compare(phpversion(), $file->info['php']) >= 0) {
    return TRUE;
  }

7/ I prefer ... MAY be wrapped ... this is mostly commen sense to not write lines of > 200 characters, but forcing people to break on a certain count is (for me) too much demanding.

My 2c on #4:
all fine by me

I have read through the summary, but really, this is the kind of things we should not spend this much time on right now. Code freeze is coming closer the last thing we want is to require all core developers to start re-roll patches because we changed the coding standards.

So please, let's just stick to what we have right now and not change anything. Our current standard has worked great for a long time.

Let's acknowledge that some patches might look a bit different than others, our standard might leave some room for that in this particular case.
We are all people with slightly different opinions. But let's see it as a beautiful thing - how our code grows organically ;)

Yeah, what really needs to be done is that the Drupal Code Sniffer needs to not come up with standards of its own and complain when people make perfectly reasonable choices... which is what triggered this issue in the first place.

There was a suggestion to tell the code sniffer that certain ... tests ... are ok to be failed, and then things can proceed.

This issue appears to be holding up using Coder to perform reviews of core modules such as #1512434: Make Aggregator module pass Coder Review. It looks like there was some consensus formed which was use human judgement while encountering cases like the examples in the issue summary.

Is this a correct summary so that we can move forward on the other postponed issues?

Hey, I agree with a lot of what Sun is saying in #24 about wrapping functions.

I really hate this, I think it's hard to both read and write:

$foo = foo_function(
  $term_1[$lancode][$delta]['value']['tid'],
  $term_2[$lancode][$delta]['value']['tid'],
  t(
    'Message with %arg1 and @arg2'
    array(
      '%arg1' => $bar,
      '@arg2' => $baz,
    )
  )
);

I don't think I've ever seen an example of functions being spaced like that t() is. Nobody I've ever worked with, or any code I've referenced from documentation/blogs Drupal or otherwise does that. Where was this idea adopted from, exactly?

Compulsory wrapping of function calls over 80 characters would be even worse in contrib than core - Remember that every contrib module currently prefixes the name of its functions with the name of the module and some contrib modules have quite long names.

I have a lot of perfectly readable single line function calls that are over 80 characters long in my contrib code simply because of indentation + long function names and don't really want them ending up in sparsetown.

I hate the idea of banning multi-line if statements where each new line begins with an || or && - I agree 100% with #21 and think that the example there isn't even a worst-case example of how complex if statements can get.

I love the idea of forcing inline comments for multi-line if statements.

Also, why is it ok to say we have to wrap function calls over 80 characters but we're no longer allowed to wrap if statements? what is it about if statements that makes them easier to read than function calls if they're on a single line?

For me I'm giving a bit +1 to "Proposed resolution 20 + 21" over "Proposed resolution 4".

The one thing I'd really like to remove from #20+21 is the arbitrary secondary character limit of 110, same as what was said in #43. Even Sun admitted that it's arbitrary and won't work 100% of the time.

I don't think I've ever seen an example of functions being spaced like that t() is. Nobody I've ever worked with, or any code I've referenced from documentation/blogs Drupal or otherwise does that. Where was this idea adopted from, exactly?

I'd replace "Drupal" with "any PHP", and couldn't agree more. I've seen a lot of PHP code, but the proposed formatting here is beyond me.

Various patches recently slipped into core that are using this new style and all affected code is hardly readable. The extreme verboseness makes it really hard to decipher what the actual stack of calls and arguments is. You need to mentally remove all whitespace first, before you can make sense of the actual PHP code.

OK.

Lets get a proposal here, that re-writes the standard/recommendation with "MAY" wrap, with an example of not wrapping, and an example of a reasonable wrap.

And, let's implement the check for *may* in the code sniffer, so we can get automation for most of the things there is consensus for.
I dont think that needs to be blocked on this.

@YesCT, isn't proposed resolution 20+21 /6 exactly those two examples? or are you looking for something more?

Those are a good basis. But those need re-written in a more proposal/conclusion manner than a discussion one, and take into account some good suggestion that came after that too.

Could someone
1) read this really carefully
2) check the issue summary and update it to make sure the proposed resolution matches it (mostly 20/21, with the *may* and other good followup points)
3) post a comment to say that is done
4) someone else mark it RTBC

I get a little fuzzy with the next steps after that, maybe:
5) ... a core committer to say OK, and then mark it ... needs work?
6) update the standards doc ... http://drupal.org/coding-standards ... wait, not everyone can edit that page. so maybe if we get to this point, it needs to get assigned to someone who can edit that page
7) mark this issue fixed?

So you want somebody to add a third proposed resolution that combines the two current ones + most recent criticisms?

It seems to me like that is needed. But if you dont think so. I'm happy to have someone skip to number 3 and 4: say that it's done and mark it RTBC.
I just want to unblock this and move it forward.

I'll have a shot now.

Updated issue summary.

Added proposed revision as at #55

I added a "proposal as at #55". It was largely based on 20+21 simply because I thought the wording was better with the MAY, SHOULD, RECOMMENDED, MUST style syntax.

Main differences between what I posted and 20+21 are:

1/ Removed specific reference to 110 characters in response to #40, #43, #48
2/ No changes
3/ Explicitly stated that inline comments need to be provided for each line of a multiline `if` statements in response to #21 and #48
4/ No changes
5/ No changes
6/ Removed reference to 110 characters in response to #40, #43, #48. Changed it to "when the length of individual paramters is very long".
Re-instated "especially when the function call includes other function calls or arrays." from proposal #4.
Added an array to the example.
Added further qualification to the example "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."
Added "Function calls should either wrap all parameters or none" with an example in response to #22.
7/ Removed reference to 110 characters in response to #40, #43, #48. Replaced with "array that contains more than one value" in response to #20.
8/ Removed reference to 110 characters in response to #40, #43, #48
9/ No changes

I also added "10/ Opening and closing parentheses MUST use the same level of indentation across multiple lines." in response to #17.

That should cover all the constructive criticism in this thread so far. Let me know if I missed anything.

ah, shouldn't be assigned to me any more.

Fix for previous revision

Updated issue summary removed old proposals.

super. I updated the issue summary to take out the older proposals so they dont confuse people. (see them by looking at the revision log if needed)

this is now really ready for someone to review and mark it RTBC if .... it's rtbc.

Thanks for the new summary!! I have some definite concerns about the proposal that is now in the issue summary.

#3:

a) The || operator is at the beginning of each line. Which could be fine, but I'd like to know whether that's a standard we're adopting (if so, it should be mentioned in the notes on #3), someone's personal preference, or what. I think it's a lot more common to keep the operators on the previous line in code I've seen over the years (not necessarily PHP/Drupal), although the benefit of having the operators at the beginning of the line is that they are more easily scannable.

b) #3 also kind of implies that you can't split a conditional up unless you plan to document each condition with an in-line comment. Is that the case? Seems a bit restrictive.

#5:

I don't see why the rules for a while() should be different for an if(), since both use logical expressions.

#6:

I don't like the proposed example of how to wrap. In one of the lines, it has:

t('Some other message here.'), array(

where t() is one of the function params, and array() is the next one. How come those are on the same line? The text says either to wrap all or none of the params in a function call, and this example has two params (or parts of two params) on the same line. I don't get how that can be a good style.

Overall:

Do we really need 10 belabored points added to our coding standards just for this? Truly, I'm inclined to just say "won't fix", remove the obviously bad standard that I think is still disgracing our standards page currently on this subject, and call it good. Or adopt a simpler standard, something like:

If you want to break long lines into multiple lines, be consistent in how you do it:
- If you are breaking an array declaration, put each element on its own line, rather than having a few elements on each line. Similarly with other structures.
- Never break up function declarations.
- Never break up for, while, etc. lines.

I don't want to say "won't fix" as this issue effects me pretty often while I'm refactoring things that I find in the wild for legibility. Let's just keep at it until we get something that works - likely that we can come up with an abridged version once we're clear on the specifics.

- I've never seen anyone put the || or && at the end of the line as it makes it too hard to scan, I wasn't referencing any standard but it never occurred to me that putting the logical operator at the end of the line was an option. I guess we just hang out in different circles >.< I could probably dig up some references if needed.

- Yes, I was thinking that you shouldn't split an if statement into multiple lines unless it has comments as an `if` statement that doesn't require comments probably doesn't need to be split onto multiple lines. The multiline split for conditions is more about keeping things clear despite complexity in the logic than just reducing line length. Happy to add the word SHOULD instead of the implied MUST if you feel that's too restrictive.

- Seems fair to me that `while` can follow the same rules as `if` but I'd like to hear if anyone else has objections to that.

- I think the idea with the array thing is that the "array(" part of it is just a keyword that's allowed to stay inline and the first actual key/value of that array is considered the first part of "the parameter". You can tell that it is part of the array and not a new argument for the original function by the indentation. This was raised in #24 by sun:

If you want to wrap, then you're free to do so, but without the unnecessary verbosity for the array( declaration

I personally don't mind either syntax Sun's is more compact, but jhodgdon's example in #22 (excluding the incorrect closing parentheses) seems more consistent with what we're doing elsewhere with arrays. Would like to see some more discussion on this point...

It always depresses me when we have these long, drawn out discussions sucking in tons of key people in the project about how to make up our own Drupal-specific coding standards.

We really should align instead with one of the PSR-X working groups/standards and work to improve their docs where necessary. See for example https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-codi...

@webchick, using a standard that isn't Drupal-specific makes sense to me, even if just because it reduces the amount of work we have to do ourselves.

At the moment though there's quite a few differences already between http://drupal.org/coding-standards and PSR-X. Even "simple" things like declaring a function or writing a switch statement are different in how they handle whitespace and braces/parentheses between the two standards.

I'm concerned that even though PSR-X is great, it doesn't appear to cover all the use-cases we've raised here. Notably there's no mention of multi-line `if` statements that I could see. Not sure if that means you can't do them, or there's just no accepted syntax for them yet :/

Is there an open issue for dropping Drupal's current standards and moving to PSR-X across the board? If so, that sounds like something that should be linked to from here as basically all meta-discussion around code styling becomes moot once that goes through.

In the meantime we could pretty easily re-write the current proposal, deferring everything that PSR-X has an opinion on to PSR-X and leaving only the "gaps" open to discussion here. That way we'd still be "PSR compatible" to the best of our knowledge, if we ever did make the switch.

So... Can I just take a couple of steps back and say... I think we established our standards in the Drupal project, a number of years ago, because the code was a mess, and we needed to say "Put the {} here" and "Make sure there are spaces around operators" and things like that so that the code would become more readable and more uniform.

For the most part, I think our code is pretty readable and uniform now, and I wonder if all of this stuff about wrapping lines isn't mostly an edge case that doesn't come up very often that can be left completely unstated. Are there terrible code examples in our current code base or being proposed in patches that warrant these standards being established? Or is the only problem we really need to address that for some unknown reason Coder is advocating splitting things up that shouldn't be split up?

As a note, our current standards on wrapping are at:
http://drupal.org/coding-standards#linelength

My alternate proposal would be that we replace this with:
-----
There is no particular standard for how long code lines can be (but keep in mind that comment lines cannot exceed 80 characters, and longer code lines are generally less readable that shorter lines).

If you have an extremely long line of code that you want to wrap to break it into multiple lines, follow these guidelines:
- If you are breaking an array declaration, put each element on its own line, rather than having a few elements on each line. Follow a similar practice with other structures (for instance, a conditional should be broken up into one condition per line rather than several per line).
- Indent an additional two spaces when you break code up. If there is logical nesting in the structure you are breaking up, indent an additional two spaces with each level of nesting.
- Never break up function declarations, for() lines or foreach() lines. Keep them on a single line.
- Consider whether defining local variables to break up complex expressions into parts may make the code more readable (self-documenting if you choose good variable names) than just breaking up a long line by wrapping.
-----

The current standards say "In general, all lines of code should not be longer than 80 chars." which is not really reasonable for function calls.

As well as just extending the current standards page, we may need to update some of what's already there.

@jhodgdon, Webchick makes a totally valid point in that we should look to existing standards first before going off on our own tangent. The alternative proposal in #63 *seems* good, but for the purpose of discussion it would be good to see code examples along with the description for each rule - if only because at the moment statements like "Follow a similar practice with other structures" could be interpreted as either the current proposal's #6 or your own objection to it, depending on who you are.

I'll have a go at mixing the current proposal with PSR and I'll update the summary, I think you'll like it more than the current proposal as uses the verbose syntax you've been advocating from the start. If we can get a consensus on that, *then* let's look at trying to write a shorter synopsis that can be merged into the existing standards.

Wrapping is not so edge-case that we should be flippant about coming to a consensus or dismissive of concerns people are raising. By design the "options-style-array" has become almost ubiquitous in D7 and above for interacting with the API - if you don't have a standard for wrapping function calls the only way to keep your API calls readable is to define the array as a variable and pass it in every time, which is tedious and arguably not even "a good thing to do". The problem is exacerbated when t() starts appearing in those arrays.

Currently, in the wild you're much less likely to see:

$message = "Your @thing has completely broken. Please fix it on the <a href="@url">configuration page</a>";
$substitutions = array(
  '@thing' => $var,
  '@url' => 'http://example.com',
);
watchdog('myModule', $message, $substitutions, WATCHDOG_ERROR);

Than this:

watchdog('myModule', "Your @thing has completely broken. Please fix it on the <a href="@url">configuration page</a>", array('@thing' => $var, '@url' => 'http://example.com'), WATCHDOG_ERROR);

But really, is the first example even better? You're setting two variables that are presumably only going to be used once and thrown away, just to keep your code legible. What we're all saying that we want is something like (using PSR-2 here) but our coding standards don't yet make it clear how to achieve this:

watchdog(
  'myModule',
  "Your @thing has completely broken. Please fix it on the <a href="@url">configuration page</a>",
  array(
    '@thing' => $var,
    '@url' => 'http://example.com',
  ),
  WATCHDOG_ERROR
);

I'm going to try making a mix of PSR + what we have and add it to the summary. It's probably worth seeing what comes out of that.

Please note that adopting PSR-1 and PSR-2 has been discussed and won't fixed a couple of times already, and isn't really up for discussion.

In general, see #1693672: PSR-2 and PSR-1 for code formatting. (though there are duplicate issues)

Further reading here:
http://www.technosophos.com/content/when-standard-bad-standards-body
http://engineeredweb.com/blog/practical-problems-psr-2/

At a quick glance the PEAR standards have a write-up on splitting up long if statements:
http://pear.php.net/manual/en/standards.control.php#standards.control.sp...

And for splitting function calls onto separate lines:
http://pear.php.net/manual/en/standards.funcalls.php#standards.funcalls....

These are at least closer to our standards than PSR and have examples.

@sun, good to know. I'm still going to post a summary that shows how it would look (for comparison) as I've pretty much finished it already.

@Cottser, I'm going to steal that paragraph about if statements from PEAR and make a few minor revisions to the current proposal as per #59 and #60 ;)

Keeping the operators at the beginning of the line has two advantages: It is trivial to comment out a particular line during development while keeping syntactically correct code (except of course the first line). Further is the logic kept at the front where it's not forgotten. Scanning such conditions is very easy since they are aligned below each other.

Unfortunately, both PSR and the PEAR standards go for a 4 space indentation, so regardless of whatever other inconsistencies there are we can't just use their examples verbatim for ourselves :(

Neither PEAR or PSR allow for putting the first argument of a function call inline with the function call, or pushing the 'array(' into the line above. I'm going to retract what I said earlier in #48 and just accept that I've always been "doing it wrong" as far as a few published standards are concerned >.<

Updated issue summary remaining tasks

mmk, I've updated the summary.

For all proposals:

- Added PEAR reasoning behind pushing operators to the start of a line in a multi-line if statement
- Moved where to the if/else rules
- Stated that multi-line if statements SHOULD be commented

New things introduced by PSR-2:

- Explicit "soft" line limit of 120 characters
- Multi-line function *declarations* are allowed

I think that, given that the discussion so far explicitly pulled out those two things that means we *don't* want to adopt PSR-2 rules as things stand.

I also added my *interpretation* of the "alternative" proposal put forward in #63 but I might have got this wrong due to the way it was worded. The only difference between that and the current proposal that I could see seems to be that all multi-line structures are "one indented item per line" and so implicitly "first argument on first line" is not allowed in any context.

I'm personally going to +1 the "alternative" proposal as it stands at the moment because even though it is more verbose, it seems to be more in line with other PHP standards adopted elsewhere and it is more consistent by treating all multi-line structures as the same thing.

Let me know if I've gotten anything wrong. Thanks.

Edit: I just realised that the alternative proposal would also apply to if statements. Updating the summary now...

Update as at #68

fwiw, i would like to be able to keep the first condition in an if statement on the first line, like in the PEAR standard.

I'm generally in favour of the proposal, but it doesn't say what to do with along if() condition that has nested conditions, eg:

    if (isset($entity_info['bundle keys']) && (count($entity_info['bundles']) == 1 && key(reset($entity_info['bundles'])) == $entity_type)) {

Should nested conditions go on one line? If they need to be broken because themselves they are too long, should they get indented an extra level?

Update as at #69

Hi,
nice to see this being discussed.
I realize we should look at PSR-2 and other projects, but there is some room for bikeshed still left. So here are my thoughts..

Point 3 of the issue summary:
Keeping the operators at the beginning of the line has two advantages: It is trivial to comment out a particular line during development while keeping syntactically correct code (except of course the first line). Further is the logic kept at the front where it's not forgotten. Scanning such conditions is very easy since they are aligned below each other.
Very much agree.

I even played with the following sometimes, but I think that's a bad idea :)

if (TRUE
  && $a === $b
  && $x === $y
) {

Here every condition is on a newline and can be commented out or moved up or down, and have its own comment. TRUE (or 1) is the neutral element for &&, FALSE is the neutral element for ||. But again, I don't think this will get any support.

I wonder why we have 4 spaces indent before each && operator? In the rest of Drupal it is just 2 indent. And I don't see anything mentioned about this in PSR-2.
If we do it like below, then the first condition is even aligned with the others.

if ( $a === $b
  && $x === $y
) {

The ) { should be on a new line for my taste.

@joachim (#71):

Should nested conditions go on one line? If they need to be broken because themselves they are too long, should they get indented an extra level?

I sometimes do this, but undecided about the closing bracket:

if ( $daddy === 'rich'
  && $mama === 'good looking'
  && ( $fish === 'jumping'
    || $cotton === 'high'
  )
) {

P.S.: I want to mention I personally prefer the yoda condition, but the current unwritten policy is against that, so I kept that out of the above examples.

Coding standards decisions are now supposed to be made by the TWG

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

Marking that it needs announcement for final discussion because it seems like we were close to a consensus before this stalled.

I would really like to be able to do something like this (which I think is the easiest to read):

public function __construct(ConfigFactoryInterface $cf,
                            EntityTypeManagerInterface $etm,
                            CacheTagsInvalidatorInterface $ci,
                            ModuleHandlerInterface $mh,
                            EntityFieldManagerInterface $efm,
                            EntityTypeBundleInfoInterface $etbi) {
  parent::__construct($cf, $etm, $ci, $mh);
  $this->entityFieldManager = $efm;
  $this->entityTypeBundleInfo = $etbi;
  // ...
}

The other ways that have been proposed would make this more difficult to read.

What else needs to be done from outside of the Technology Working Group to take this forward?

Can we please also have an example of an if condition with multiple nested conditions. I believe it would be more readable when the operators at the same level are at the same indentation

Option 1

// Comment for the if condition.
// Comment for the first condition.
if ($some_very_long_variable_1 === $value1
    // Comment for the second condition.
    || $some_very_long_variable_2 === $value2
    // Comment for the nested condition.
    // Comment for the first condition in the nested condition.
    || ($some_very_long_variable_3 === $value3
    // Comment for the second condition in the nested condition.
    && $some_very_long_variable_4 === $value4
    // Comment for the third condition in the nested condition.
    && $some_very_long_variable_5 === $value5)
    // Comment for the sixth condition.
    || $some_very_long_variable_6 === $value6
) {
  // Do something.
}

Option 2

// Comment for the if condition.
// Comment for the first condition.
if ($some_very_long_variable_1 === $value1
    // Comment for the second condition.
    || $some_very_long_variable_2 === $value2
    // Comment for the nested condition.
    // Comment for the first condition in the nested condition.
    || ($some_very_long_variable_3 === $value3
        // Comment for the second condition in the nested condition.
        && $some_very_long_variable_4 === $value4
        // Comment for the third condition in the nested condition.
        && $some_very_long_variable_5 === $value5)
    // Comment for the fifth condition.
    || $some_very_long_variable_6 === $value6
) {
  // Do something.
}

Option 3

// Comment for the if condition.
// Comment for the first condition.
if ($some_very_long_variable_1 === $value1
    // Comment for the second condition.
    || $some_very_long_variable_2 === $value2
    // Comment for the nested condition.
    || (
        // Comment for the first condition in the nested condition.
        $some_very_long_variable_3 === $value3
        // Comment for the second condition in the nested condition.
        && $some_very_long_variable_4 === $value4
        // Comment for the third condition in the nested condition.
        && $some_very_long_variable_5 === $value5
    )
    // Comment for the sixth condition.
    || $some_very_long_variable_6 === $value6
) {
  // Do something.
}

I prefer Option 3 as it makes the logic much more clearer than the other two. Also each condition is independent and conditions can be taken out or added at any level without causing inadvertent errors.

Option 4

// Comment for the first condition.
$temp1 = $some_very_long_variable_1 === $value1;
// Comment for the second condition.
$temp2 = $some_very_long_variable_2 === $value2;

// Comment for the first condition in the nested condition.
$temp3 = $some_very_long_variable_3 === $value3;
// Comment for the second condition in the nested condition.
$temp4 = $some_very_long_variable_4 === $value4;
// Comment for the third condition in the nested condition.
$temp5 = $some_very_long_variable_5 === $value5;
// Comment for the sixth condition.
$temp6 = $some_very_long_variable_6 === $value6;

// Comment for the if condition.
if ($temp1 || $temp2 || ($temp3 && $temp4 && $temp5) || $temp6) {
  // Do something.
}

Another possible option:

if ($a == $value1 || $b == $value2 || $this->isComplexCondition(...)) {
}

protected function isComplexCondition(...) {
}

The TWG discussed this on our call today - given that we have had new proposals since #75 that propose new options - we feel we need an update to the issue summary to finalize the options before announcing for final discussion. Tagging appropriately.

Untagging needs announcement as we really need a summary update.

> Function declarations MUST be written on a single line; they should never wrap multiple lines.

The landscape has changed since that was written: constructors of classes with DI have horrendously long declarations, and they would be much easier to read and amend with wrapping.

When declaring 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.

When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them.

I think that means we've have this:

  public function __construct(
    EntityTypeInterface $entity_type,
    EntityTypeManagerInterface $entity_type_manager,
    PreloadableRouteProviderInterface $route_provider
    ) {
    parent::__construct($entity_type, $entity_type_manager, $route_provider);
  }

The declaration lines having the same indentation as function code puts me off a bit. I think it hinders readability because it makes it less clear where the declaration ends and the code begins. I'm not sure what we could do to improve that though.

I still think this is preferable to long constructors that go way off the screen in a text editor.

In places where I've seen function declarations split like this, I always thought the closing paren and opening curly brace are back at the same level of indentation as the "public function" keywords. Like so:

  public function __construct(
    EntityTypeInterface $entity_type,
    EntityTypeManagerInterface $entity_type_manager,
    PreloadableRouteProviderInterface $route_provider
  ) {
    parent::__construct($entity_type, $entity_type_manager, $route_provider);
  }

It's a small difference, but I think it helps the readability considerably, and (IMHO) completely solves the problem of not quickly seeing where the declaration ends and the code begins.

However, I'm not sure the best way to document the difference. Given this text you quoted:

When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them.

there doesn't seem to be enough to specify which of these two indentation styles are intended. Perhaps something like this:

When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them, at the same level of indentation as the start of the function declaration itself.

? Or something. ;)

And yes, I agree that either of these split-line approaches is preferable to doing it all on a huge single line.

> always thought the closing paren and opening curly brace are back at the same level of indentation as the "public function" keywords.

That looks much better!

Although I don't want to risk starting a bikeshed war, if we do this for wrapping function declarations, I think it'd make sense to do a similar thing when wrapping if () across multiple lines. For example, the issue summary has the following indentation/wrapping proposal for a long if ():

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;
}

I think the following is more readable, for similar reasons to why I think #84 is better:

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;
}

In addition to being more readable (IMHO), it makes it easier to add more clauses without unnecessarily touching the last clause (reducing diffs and potential merge conflicts), makes it easy to comment out the last clause (which the motivation for this construct claims is possible, but actually isn't for both the first *and* last clauses), etc.

Yes, I very much support #84 and #86. I have done it like this for a long time, and it is the way to go.

In future PHP versions, PHP might even allow a comma after the last parameter. I think I saw this being discussed somewhere.

EDIT: See also #72

+1 to 86.

With regard to function declarations, I've just noticed how another benefit of the line wrapping is that it makes it really easy to see that the create() call and the __construct() parameters match up:

  public function __construct(
    array $configuration,
    $plugin_id,
    $plugin_definition,
    CheckoutFlowInterface $checkout_flow,
    EntityTypeManagerInterface $entity_type_manager,
    EntityFormBuilderInterface $entity_form_builder,
    EntityDisplayRepositoryInterface $entity_display_repository
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition, $checkout_flow, $entity_type_manager);

    $this->entityFormBuilder = $entity_form_builder;
    $this->entityDisplayRepository = $entity_display_repository;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, CheckoutFlowInterface $checkout_flow = NULL) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $checkout_flow,
      $container->get('entity_type.manager'),
      $container->get('entity.form_builder'),
      $container->get('entity_display.repository')
    );
  }

One line for each thing, and I can see how each call to the container for a service matches a parameter.

This has stalled for *checks watch* FIVE YEARS :(

Perhaps we should split it up into separate issues to make it easier to get to an agreement?

10 years! Eep :(

I'd say 3 issues:

- function calls
- function declarations
- language constructs (I can't see any examples of this in the issue so far, at least with a quick text search for the phrase)

I've been stacking conditionals like this in custom glue code on Drupal projects for several years.

if (
  DRUPAL_TEST_IN_CHILD_SITE &&
  !headers_sent() &&
  (
    !defined('SIMPLETEST_COLLECT_ERRORS') ||
    SIMPLETEST_COLLECT_ERRORS
  )
) {
  // ...
}

This is literally the last example in the issue summary called out as "Alternative examples", but this form has several benefits over the previous suggestions.

1.- the first conditional is on a line by itself so that all conditions line up with same indentation.
2.- #1 makes this consistent with multi-line functions args that start a newline after the opening paren, for coherence and consistency.
3.- Indent 2 characters not four so that IDEs configured for Drupal to indent by 2 will work automatically. The added benefit is that if indentation is consistent with multi-line function signatures and multi-line function calls that indent 2 chars, docs and examples are less confusing. 4-char indent appears to have been introduced when the closing part ) { was at the end of the last conditional instead of on a line by itself, to differentiate between the code expressions inside the conditional versus the stacked logic conditions. #86 was pretty close except the indentation was left at 4 characters instead of changed to 2.
4.- I would humbly propose placing the logical operators (&& and ||) at the end of the line instead of the beginning, to help readability and code elegance by having expressions line up vertically just like parameters in function signatures and function calls, a la:

function myfunc(
  $param,
  $param2,
  $param3,
  $param4
) {
  // ...
}

if (
  !empty($param) ||
  !empty($param2) ||
  !empty($param3)
) {
  // ...
}

better than:

if (
  !empty($thing)
  || !empty($that)
  || !empty($theother)
) {

better than first conditional on one line:

if (!empty($thing)
  || !empty($that)
  || !empty($theother)
) {

better than 4 char indentation:

if (!empty($thing)
    || !empty($that)
    || !empty($theother)
) {

By "better" I basically mean easy to read, easy to write, consistent and maintainable.

Maintainability is important. if a patch adds a condition before or after any condition in the list, you see a very simple, readable one-line diff, as whitespace doesn't need to change as the code changes in the conditional...

if (
+ !empty($newthing) ||
  !empty($thing) || 
  !empty($that) ||
  !empty($theother)
) {

better than:

- if (!empty($thing) || 
+ if (!empty($newthing) ||
+   !empty($thing) || 
  !empty($that) ||
  !empty($theother)
) {

better than:

- if (!empty($thing) || !empty($that) ||  !empty($theother)) {
+ if (!empty($newthing) || !empty($thing) || !empty($that) || !empty($theother)) {

Caveat 1: In these examples the conditionals have been shortened for brevity; in real-world these would typically be much longer lines of code like previous examples.

Caveat 2: I'm not a core developer as was called out in "Remaining Tasks" as being marked "done"... so I'm plainly aware my vote perhaps doesn't count as much as others here, but hopefully some of the points I bring up would resonate, and be considered before being labeled as bikeshedding.

There is some precedence for placing the logic operands at the end of the line.

https://github.com/prettier/plugin-php/blob/main/tests/if/if.php#L80-L94

This issue was discussed at #3282227: Coding Standards Meeting 2022-06-07 2100 UTC.
We agree that progress will be easier if this were split into several issues. This was also suggested by @joachim in #90. And all the issues should use the language from the PSR coding standards.

What are the suggestions for how to split this up?

I read through the issue thinking about how to divide this up,

I agree with #90 that these 3 make sense, but I also think the line length discussion should be separate as well. I found two other issues about length. #3173782: Increase line length limit to 120 and #3039007: Relax the rule that arrays may not span more than 80 characters.

• function calls
• function declarations
• language constructs
• line length

Created a new issue for function declarations, #3295249: Allow multi-line function declarations

As PHP native attributes are used in core it's one more multiline statement with named arguments and sub-items