Allow multi-line function declarations

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

Adding new template as best I can.

As I reviewed this I realized that there are outstanding issues for two of the changes here. There are ✨ Revise coding standards to use IETF RFC 2119 standards Closed: won't fix and #2928856: Adopt the PSR-12 standard for PHP7 return types → . Because of the I have removed those changes and am tagging for a change record update.

I also removed repeated text and used a list format. That is what I see in the existing standards.

Setting to Needs Review so the doc changes can be reviewed and then the CR updated.

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.

This is tagged for change record updates which I think should be done before the announcement. Have they been done?

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.1.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

There are already instance of this in core, for example, mstrelan pointed out in Slack, \Drupal\Core\Extension\DatabaseDriver::__construct

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, 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?

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?

I have made the edit to the doc page and published the CR

Forgot to remove the tag.

Who can followup with issues for Code etc.

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

The sniff is Squiz.Functions.MultiLineFunctionDeclaration but I can find no documentation for it.

Core is using that sniff

  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine">
    <severity>0</severity>
  </rule>
  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.CloseBracketLine">
    <severity>0</severity>
  </rule>
  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.ContentAfterBrace">
    <severity>0</severity>
  </rule>
  <!-- Standard yet to be finalized on this (https://www.drupal.org/node/1539712). -->
  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.FirstParamSpacing">
    <severity>0</severity>
  </rule>
  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.Indent">
    <severity>0</severity>
  </rule>

I tested by changing \Drupal\Core\DrupalKernel::handleException to

  protected function handleException(
    \Exception $e,
    $request,
    $type,
  ) {

I then ran the commit-code-checks and there were no errors. So, Coder is not preventing using this new standard.

I changed to incorrect indentation as shown.

  protected function handleException(
    \Exception $e,
       $request,
    $type,
  ) {

And tested again and this passes phpcs.

I then remove this rule

  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.Indent">
    <severity>0</severity>
  </rule>

And tested again. This time the indentation error was found. However, phpcs reports heaps of other errors in core. The ones I checked had correct indentation. Plus it was reporting errors from sniffSquiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine which was not changed. That seems wrong.

Created 📌 Add rule for multi-line function declarations Fixed

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

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

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 :)

📌 Fix Drupal.Functions.MultiLineFunctionDeclaration coding standard Needs review

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.

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