Problem/Motivation

The current coding standards specify:

Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later.

This is not really explicit.

On top of that we also don't specify trailing commas for annotations yet.

Proposed resolution

1 Change the paragraph after the second example in the arrays coding standards

Current text

Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later.

Proposed text

Note that, as seen above, in multi-line arrays there MUST be a comma after the last array element. This helps prevent parsing errors if another element is placed at the end of the list later.

2. Change the example and notes afterwards for annotations.

Current text
/**
(rest of the documentation for this class)
 *
 * @Plugin(
 *   id = "aggregator",
 *   title = @Translation("Default fetcher"),
 *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler.")
 * )
 */
class DefaultFetcher implements FetcherInterface {

Drupal standards notes:

  • It is better to use a specific plugin type, like @EntityType, rather than the generic @Plugin.
  • Each key of a plugin annotation should be on its own line.
  • Annotations are functional code, so individual lines should not be wrapped.
  • For more information on how annotations are used for plugins, see the plugin documentation.
Proposed text
**
(rest of the documentation for this class)
 *
 * @Plugin(
 *   id = "aggregator",
 *   title = @Translation("Default fetcher"),
 *   description = @Translation("Downloads data from a URL using Drupal's HTTP request handler."),
 * )
 */
class DefaultFetcher implements FetcherInterface {

Drupal standards notes:

  • It is better to use a specific plugin type, like @EntityType, rather than the generic @Plugin.
  • Each key of a plugin annotation should be on its own line.
  • Annotations are functional code, so individual lines should not be wrapped.
  • Same as for arrays, you MUST also include a comma after the last element in the definition (and in the definitions of any nested annotations).
  • For more information on how annotations are used for plugins, see the plugin documentation.

Remaining tasks

  1. Agree on documentation changes
  2. After the consultation period (in upcoming announcement) discuss at the next CS meeting.

Comments

dawehner created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented

+1
I think we currently already reject patches that don't do this (at least I do, but I think in Core it's the same), so we should just be explicit about it.
And until a few days ago I didn't even know it was allowed in annotations, so maybe even more important to mention it for those.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue summary: View changes

Added a suggested phrasing to the IS.

I wonder, though, whether the "prevent parsing errors" argument really still holds, with probably almost everybody using an IDE. It's still a good idea because it makes changing the array easier (and leads to a more consistent look) but the argument seems flimsy.

Also, I used "SHOULD", since "MUST" seemed a bit strong for just a comma. On the other hand, we currently more or less enforce it, so maybe it should be "MUST" after all?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

I wonder, though, whether the "prevent parsing errors" argument really still holds, with probably almost everybody using an IDE. It's still a good idea because it makes changing the array easier (and leads to a more consistent look) but the argument seems flimsy.

Well, its still small things. I often ignore the IDE for a bit, so saving those few seconds sum up over time.

Personally I'm a huge fan of the diff argument to be honest.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented

+1

#3, according to http://www.ietf.org/rfc/rfc2119.txt, "MUST" must be used for a requirement, and I think this should be a hard requirement.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue summary: View changes

#3, according to http://www.ietf.org/rfc/rfc2119.txt, "MUST" must be used for a requirement, and I think this should be a hard requirement.

Yes, I'm aware, I just sounded so excessive. But of course, it is (or should be) a hard requirement (we don't want to commit patches without it), so "MUST" makes sense.
Adapted the IS for that (and slightly changed phrasing for annotations).

krknth’s picture

krknth CreditAttribution: krknth as a volunteer and at Valuebound commented
Issue summary: View changes

Updated issue summary with latest array coding standard link.
https://www.drupal.org/docs/develop/standards/coding-standards#array

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Active » Reviewed & tested by the community
Issue tags: +needs announcement for final discussion

+1, I think we have enough agreement here to proceed in the process.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented

*bump*

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

*bump*

Should this define what 'where possible' means?

Since this went RTBC, PHP now allows trailing commas in:

- function calls since PHP 7.3
- function declarations since PHP 8

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes
Issue tags: -Needs announcement for final discussion +final discussion

Update the IS in preparation for an announcement.

donpwinston’s picture

donpwinston CreditAttribution: donpwinston commented

"Always add trailing commas, if possible"

I think rules like this are silly. The PSR guidelines are sufficient. Should be just focusing on just Drupal specific things.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think we need to resolve #10. I personally would not want trailing commas at the end of function declarations, unless maybe they're multi-line, which I think we have another issue for.

We can maybe say 'MUST use trailing commas where possible for multi-line declarations'.

Or 'MUST use trailing commas for arrays and annotations' and leave function declarations and calls for another issue.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

+1 , this makes merges a lot easier because of readability and less noise.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Looking at the issue summary this only mentions arrays and annotations, so I think we can leave multi-line function declarations/calls undefined.

We probably need to add the same rules for attributes as annotations, but I guess we don't have anything to add to yet.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue tags: -final discussion +Approved, +Needs documentation updates

There was no further discussion of this change in the latest coding standard meeting. @catch and @bbrala support the changed. I am adding tags per step 8 of the CS process on the project page.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

We didn't discuss changes to Coder here. That needs to be done.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I think core might already have the coder rule for arrays enabled after #2572613: Fix 'Drupal.Array.Array.CommaLastItem' coding standard - coder and core getting ahead of the docs.

However we should probably open a coder issue for annotations?

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Title: Always add trailing commas, if possible » Always add trailing commas, for arrays and annotations
Related issues: +#3380949: Ensure trailing commas in annotations

Followup made, #3380949: Ensure trailing commas in annotations
Updated the title to show the scope of this issue

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs documentation updates

I have updated the coding standard pages and confirmed the change using the 'compare' option in the history.

Per step 10, I have removed the tag and set the issue to "Fixed".

Thanks everyone!

Status: Fixed » Closed (fixed)

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