Contrib.social

Convention or recommendation for line breaks in constructor parameter signature with promoted properties

Open on Drupal.org →
Created on 1 July 2024, 12 months ago

Problem/Motivation

Currently I don't see a convention when and how parameter signatures should be broken to multiple lines.

In 📌 Use PHP 8 constructor property promotion for existing code Needs work we are converting constructors to use constructor property promotion.
In the existing PR there are no line breaks between the parameters.
This is a consequence of not having a convention or recommendation.

In Drupal core, a regex search that having line breaks between promoted property parameters is actually the majority.
However, there are still enough cases where promoted properties are on a single line.

E.g. this code in SystemTestController does have the line breaks, and I think it is good:
(I looked for one with attributes and promoted properties, although not all properties are promoted)

  public function __construct(
    #[Autowire(service: 'lock')]
    LockBackendInterface $lock,
    #[Autowire(service: 'lock.persistent')]
    LockBackendInterface $persistent_lock,
    AccountInterface $current_user,
    RendererInterface $renderer,
    MessengerInterface $messenger,
    public ?KillSwitch $killSwitch = NULL,
    public KillSwitch|null $killSwitch2 = NULL,
  ) {

In vendor code (symfony, drush etc) there is a mix: Some packages (drush) break these signatures to multiple lines, others (symfony) keep it in one line.

Benefits

Line breaks make long signatures easier to read, and also easier to work with when adding new parameters, reviewing git commits etc.

Three supporters required

  1. https://www.drupal.org/u/ → {userid} (yyyy-mm-dd they added support)
  2. https://www.drupal.org/u/ → {userid} (yyyy-mm-dd they added support)
  3. https://www.drupal.org/u/ → {userid} (yyyy-mm-dd they added support)

Proposed changes

1. https://www.drupal.org/docs/develop/standards/php/php-coding-standards#f... →

Argument lists may be split across multiple lines, where each subsequent line is indented once.

Parameter lists of functions and methods may be split across multiple lines, where each subsequent line is indented once.

Splitting the list of parameters is strongly recommended, if:
- The signature contains one or more promoted properties (for a constructor method), or
- At least one of the parameters has a parameter attribute, or
- The parameter list would be very long.

Parameter attributes can be placed on the same line as the parameter, or on a separate line.
They should be placed on a separate line, if the attribute itself has arguments, or if there are multiple attributes.

The part about attributes could also be handled in a separate issue.

I am replacing "arguments" with "parameters", I think this is the more word to use here.
My understanding is that "arguments" are the values we pass to a function, whereas "parameters" are the slots in the signature that receive these values.

I also mention explicitly that this is about functions and methods, I think this will reduce confusion for people who read this page for the first time.

It can be discussed whether this should be a recommendation or a convention.
My idea would be that at least for new code this should be followed.

Remaining tasks

  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Discussed by the Core Committer Committee, if it impacts Drupal Core
  7. Final review by Coding Standards Committee
  8. Documentation updates
    1. Edit all pages
    2. Publish change record
    3. Remove 'Needs documentation edits' tag
  9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a full explanation of these steps see the Coding Standards project page →

🌱 Plan
Status

Active

Component

Coding Standards

Created by

🇩🇪Germany donquixote

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @donquixote
  • Comment 12 months ago →
    solideogloria

    If there are promoted properties, then I think it should be a convention to have each parameter on its own line. It's really helpful for readability. Since autowiring and other PHP attributes may be added as well, it would be far too long and unreadable to have parameters all in the same line.

  • Comment 11 months ago →
    🇳🇱Netherlands bbrala Netherlands

    This issue was discussed in #3458726: Coding Standards Meeting Tuesday 2024-07-16 2100 UTC → .

    We seem to be positive on this, but the IS needs some love.

    Also mentioned was this rule: https://github.com/slevomat/coding-standard/issues/1684

  • Comment 11 months ago →
    🇦🇹Austria drunken monkey Vienna, Austria

    +1

    Good idea. I’d also support making this required for new code, at least for the first two “strong recommendations”. By supplying a maximum line length (e.g., 120 characters) we could even make the third one a hard rule, too. (I couldn’t really be enforced with just “very long” as the criterion.)

  • Comment 11 months ago →
    🇳🇿New Zealand quietone

    This needs work on the proposed text section. At least, the comments there need to move elsewhere.

  • Status changed to Needs work 11 months ago
  • Comment 11 months ago →
    🇳🇿New Zealand quietone
  • Comment 4 months ago →
    🇳🇿New Zealand quietone
  • Comment about 1 month ago →
    🇩🇪Germany donquixote

    At least, the comments there need to move elsewhere.

    Not sure what you mean by that.
    To me it seems clear that the proposed text is inside the quote, and what is below is comments.

    I am adding a separator heading "Notes", I hope this helps.

  • Comment about 1 month ago →
    🇩🇪Germany donquixote

    I’d also support making this required for new code, at least for the first two “strong recommendations”.

    I would be ok with that.
    I was hoping that something weaker would be more likely to land, but I have no objection to hard rules :)

    By supplying a maximum line length (e.g., 120 characters) we could even make the third one a hard rule, too. (I couldn’t really be enforced with just “very long” as the criterion.)

    Ok for me, but this could broaden the scope quite a lot.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024