Allow multi-line function declarations

How are the closing parenthesis and opening brace indented?

Could we have an example? I think that would help with understanding the proposal.

+1 for option A.

The PSR-12 linked on Option A in issue summary points to several examples, and we do need to adapt these to Drupal's 2-char indentation style instead of 4-char from those examples, so here goes:

// A simple function example.
function a_very_long_function_name(
  ClassTypeHint $arg1,
  &$arg2,
  array $arg3 = []
) {
  // function body
}

// A simple class method example.
class ClassName{
  public function aVeryLongMethodName(
    ClassTypeHint $arg1,
    &$arg2,
    array $arg3 = []
  ) {
    // method body
  }
}

// A simple function example with return types.
function a_very_long_function_name_with_return_type(
  string $foo,
  string $bar,
  int $baz
): string {
  return 'foo';
}

// A class method example with return types.
class ReturnTypeVariations {
  public function aFunctionWithReturnType(
    string $foo,
    string $bar,
    int $baz
  ): string {
    return 'foo';
  }
}

Thanks!

I am very much +1 for this.

Adds to readability, and makes diff and history much easier to understand with functions with lots of parameters.

Fixing typo :-)

#6 also covers my feelings on this, +1 on option A

I would go for option A as well... but I do find the 'just one level indent' of the args a pain. It results in the code becoming less easy to parse the function signature from the body. I much prefer a much larger indent - not necesarily as much as "to the same column as the opening (" as some formats suggest, but something like 8..12 spaces.

Option A:

class ClassName{
  public function aVeryLongMethodName(
    ClassTypeHint $arg1,
    &$arg2,
    array $arg3 = []
  ) {
    // method body
  }

Modified:

class ClassName{
  public function aVeryLongMethodName(
                ClassTypeHint $arg1,
                &$arg2,
                array $arg3 = []
  ) {
    // method body
  }

+1 for Option A - especially for promoted properties (PHP 8+) this greatly increases readibility of constructors.

Also +1 to option A, same reasoning in my comments over in #3278431: Use PHP 8 constructor property promotion for existing code

+1. BTW are we also planning on allowing using a trailing comma?

// A simple function example with return types.
function a_very_long_function_name_with_return_type(
  string $foo,
  string $bar,
  int $baz,
): string {
  return 'foo';
}

+1 to trailing commas -- though require, rather than allow.

We require them in multi-line arrays, so it would be consistent to require them here too.

FWIW, +1 from me on this for all of these reasons:
* readability
* consistency with other non Drupal PHP projects
* ease of adding attributes for auto-injection later

Trailing comma is available since PHP 8.0 https://php.watch/versions/8.0/trailing-comma-parameter-use-list

+1 to A+trailing comma

I'm not a fan of how it looks like, but I'll get used to it (as if you care :-P) and adopting PSR is always worth it.

It looks like this has overwhelming support for option A. We need to update the summary with examples, including proposed changes to docs.

Otherwise, I think this is ready for public announcement and to finalize the steps to get it adopted.

How about this for an update. I took the code examples from #5 and added trailing comma to last arg.

Added #14 to issue summary.

whitespace fix in code sample.

+1 to option A with required trailing commas.

I removed the "Needs issue summary update" but forgot to set the status back to NR.

The issue summary looks good to me, and the issue has been open for 9 months with no disagreement so I think this is ready to move to RTBC.

2.0.0 is out from https://github.com/php-fig/per-coding-style/releases/tag/2.0.0

Indeed, and https://github.com/php-fig/per-coding-style/blob/2.0.0/spec.md#45-method... includes:

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. For example:

namespace Vendor\Package;

class ClassName
{
    public function aVeryLongMethodName(
        ClassTypeHint $arg1,
        &$arg2,
        array $arg3 = [],
    ) {
        // method body
    }
}

...
When you have a return type declaration present, there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parenthesis with no spaces between the two characters. For example:

    public function anotherFunction(
        string $foo,
        string $bar,
        int $baz,
    ): string {
        return 'foo';
    }

🎉 We do a few things differently (e.g. 2 spaces for indent, the { on class declarations, etc), but otherwise, this is exactly what we're proposing. Ship it! 😉

When you have a return type declaration present, there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parenthesis with no spaces between the two characters.

I added this blurb to the IS. I noticed it before but thanks for catching this to be more explicit.

mention Drupal's coding conventions that explicitly differ from PSR-12 in IS

🤦‍♂️

+1 on Option A

Draft CR

Looks good, I'll update the CR

I created a similar improvement to allow multi-line conditions: #3392321: Relax the "Conditions should not be wrapped into multiple lines" rule for more code readability - please support it too ;)

RTBC++

+1 from me, and the CR looks good (and easier to read than the version in the issue summary).

+1, agree with everything here, supporting Option A.

This was discussed at the coding standards committee meeting and @dww, @bbrala, @AaronMcHale and myself were +1.

I think this needs to go out in an announcement now.

Looks to me like they were done? https://www.drupal.org/node/3396762/revisions/view/13284559/13286250 but I'm not 100% confident those were the same changes you tagged for, so leaving the tag on...

I made a minor change to proposed text, to swap the last two bullet points around. Logically it makes sense to talk about the final item in the list before talking about the closing parenthesis and opening brace.

I also notice that the following text has been dropped from the existing standard

Arguments with default values go at the end of the argument list. Always attempt to return a meaningful value from a function if one is appropriate

Was this by accident? Is the order of arguments still important? I think it is, and that first sentence should be added back.

I know this is RTBC and these changes should not impact the upcoming blog post. Its just the details of the wording, we all agree that the new standard will go ahead.

Arguments with default values go at the end of the argument list. Always attempt to return a meaningful value from a function if one is appropriate

I think this is already enforced somewhere - maybe phpstan? I'm not sure it is really coding style as such, feels like an actual bug if optional arguments aren't at the end.

feels like an actual bug if optional arguments aren't at the end.

Pretty sure it's a 500 error in PHP 8.1

It’s deprecated since 8.0.0, https://3v4l.org/JPtv7

This was discussed at core committers meeting on Wed December 5th (UTC)
There were no objections

Completing steps 6 and 7.

Next step is the documentation edits and core issue for phpcs rule (which may be a case of relaxing a rule in this instance).

I know it's too late now but trailing commas are ugly, unnecessary extra work for the parser and only useful when you're adding another arg later on. They should be removed from the array requirement, not added elsewhere!

Hi Nigel Cunningham,
Yes you are right that it is a bit late, we have completed step 7 out of 9 and we have 29 followers on the issue and no disagreement on the standard as proposed. Thank you for making the suggestion, everyone's input is appreciated and hopefully you will take part in other Coding Standards issues, there are plenty more that need discussion and agreements. But this one is pretty much done and dusted :-)
Jonathan

That (and many like it) don't adhere to the standard we're laying out:

When the argument list is split across multiple lines

• The first item in the list must be on the next line.
• There must be only one argument per line.
• The last argument in the list must use a trailing comma.
• The closing parenthesis and opening brace must be placed together on their own line with one space between them.

Current:

  public function __construct(
    string $root,
    protected Extension $module,
    protected string $driverName,
    protected array $discoveredModules) {
    $this->root = $root;
    $this->type = 'database_driver';
  }

Per above "must" statements (3rd and 4th bullets), this should become:

  public function __construct(
    string $root,
    protected Extension $module,
    protected string $driverName,
    protected array $discoveredModules,
  ) {
    $this->root = $root;
    $this->type = 'database_driver';
  }

Don't we want a sniff about that? If this is our standard, we should adopt it core-wide and have our tooling enforce it. I presume a sniff that would flag this could be configured to ignore single line function declarations, right?

Thanks @dww, that's useful info. But writing sniffs, or finding existing ones to include, will be covered in step 9 of the process. That does not hinder the progress of getting the standard updated. Is that right?

[edit: core/phpcs.xml.dist has Squiz.Functions.MultiLineFunctionDeclaration
11.x/core/phpcs.xml.dist?ref_type=heads#L393-409 but checking and testing of these options will be done in a separate issue]

Currently we are at step 8 "Needs documentation updates". I can see there was a Slack thread on this at the meeting. If everything is agreed, can we proceed to update the docs?

Should this be marked as Fixed per the step 8 procedures or left open because step 9 work is needed?

8. Drupal.org coding standards pages are updated for approved proposals. Issues are marked "fixed" and the “Needs documentation updates” tag removed, change record published etc.
9. When a PHP standard is approved use the following to determine the next steps.

Yeah, we need to open the follow-up now to:

1. Find / enable sniffs to enforce this.
2. Update core to be compliant.

Only once that exists and is linked here can we fully close this one out.

Thanks,
-Derek

This was implemented in Coder, only the trailing comma rule is missing for now. Opened #3440603: Validate trailing comma in multi-line function declaration for that.

Klausi has released coder 8.3.24
https://www.drupal.org/project/coder/releases/8.3.24

This includes both the initial #3440075: Add rule for multi-line function declarations and the follow-up #3440603: Validate trailing comma in multi-line function declaration

For contrib gitlab pipelines, the Coder 8.3.24 will be used immediately. So be prepared for some failing contrib phpcs jobs and the need for support. What would be the mechanism by which we give maintainers notice that this is going to happen? This issue is still RTBC, but looks like all the 9 steps are done.

The change in Coder seems to work well, we upgraded to this on a work project yesterday and phpcbf correctly fixed both spacing and trailing commas on all our badly formatted multiline constructors - we use property promotion a lot but our formatting was not consistent before.

Happy to hear it is working - we have also deployed it over our work codebases and are quite happy with the auto fixes.

Made an issue for core, needs quite a few fixes :)

#3443117: Fix Drupal.Functions.MultiLineFunctionDeclaration coding standard

Just reporting here for info, and to help others, that the change to add a trailing comma is not compatible with PHPUnit 9.6.19 in Drupal 9.5 PHP 7.4 (which is what you get when using OPT_IN_TEST_PREVIOUS_MAJOR on gitlab pipelines). The phpunit test complains of a syntax error and gives

ParseError: syntax error, unexpected ')', expecting variable (T_VARIABLE)

So if you want to fix the PHPCS rule and still maintain the ability to test at 'Previous Major' you can ignore the rule on the next line:

    ModuleHandlerInterface $module_handler,
    // Trailing comma is incompatible with PHPUnit 9.6.19 in Drupal 9.5 PHP 7.4.
    // phpcs:ignore Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma
    EntityTypeManagerInterface $entity_type_manager
  ) {

THis is all done. Followup is posted, cr also.