Problem/Motivation

Normally, we ask that a space is put between a control structure keyword and the opening parentheses.
This is the case for `if (..)`, `foreach (..)` etc, and is enforced by the phpcs rules.

However, it is not currently enforced for `match (..)`.
In fact, we see it with and without the space:

With space:

~/projects/d8/www $ ag --count "(return|=) match \(" core/
core/tests/Drupal/Tests/Core/Cache/ChainedFastBackendFactoryTest.php:1
core/lib/Drupal/Core/Config/Schema/Mapping.php:1
core/lib/Drupal/Core/StringTranslation/ByteSizeMarkup.php:1
core/modules/system/system.install:1
core/modules/block_content/tests/src/Functional/Rest/BlockContentResourceTestBase.php:2
core/modules/block_content/src/BlockContentAccessControlHandler.php:1
core/modules/field_ui/tests/src/FunctionalJavascript/ManageFieldsTest.php:1
core/modules/jsonapi/tests/src/Functional/BlockContentTest.php:1

Without space:

~/projects/d8/www $ ag --count "(return|=) match\(" core/
core/lib/Drupal/Core/Database/StatementWrapperIterator.php:3
core/lib/Drupal/Core/Database/StatementPrefetchIterator.php:1
core/modules/pgsql/src/Driver/Database/pgsql/Schema.php:1
core/modules/field_ui/src/Form/EntityDisplayModeFormBase.php:3
core/modules/jsonapi/tests/src/Functional/FileTest.php:1
core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingRedundantTagsConstraintValidator.php:1
core/modules/file/tests/src/Functional/Rest/FileResourceTestBase.php:1

Benefits

Consistency..

Three supporters required

  1. https://www.drupal.org/u/larowlan (2023-12-06)
  2. https://www.drupal.org/u/dww (2023-12-05)
  3. https://www.drupal.org/u/jonathan1055 (2023-12-13)

Proposed changes

https://www.drupal.org/docs/develop/standards/php/php-coding-standards#c...
Explicitly mention `match (..)` as a control statement.

A match statement should have a space between the match keyword and the opening parenthesis.

return match ($value) {
  5 => 'win',
  default => 'loss',
};

Note that "match" can also be a method name.
In that case, no space should be added:

return $this->match($value);

Remaining tasks

  1. Create this issue in the Coding Standards queue, using the defined template
  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Tagged with 'Needs documentation edits' if Core is not affected
  7. Discussed by the Core Committer Committee, if it impacts Drupal Core
  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

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

donquixote created an issue. See original summary.

larowlan’s picture

+1 from me to make this consistent with if and else and enforced by a phpcs rule

dww’s picture

Issue summary: View changes

Yes, please, good find. Adding myself (and @larowlan) as official supporters. Thanks!

arkener’s picture

+1 makes sense to keep this consistent with other controle structures like `switch ()`

donquixote’s picture

jonathan1055’s picture

Title: Space in `match (..)`. » Require a space in `match (..)`
Issue summary: View changes
Issue tags: +Needs change record

Adding support, so step 2 is now done. Tagging for "Needs change record"

quietone’s picture

Status: Active » Needs work

Needs work for the change record

borisson_’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Added change record.

drunken monkey’s picture

I expanded the text of the change record a bit, it was a bit too concise for my taste.

borisson_’s picture

Issue summary: View changes

donquixote’s picture

I created an MR.
One thing to consider in the wording is that "match ()" is an expression, not a statement.
I also added a rule about trailing command, and one about having each arm on a new line.

I don't know if we need rules for space around the " => ", which is the same as for arrays.

donquixote’s picture

Multi-value conditions can be refined later:

return match ($x) {
  2, 3, 5, 7 => 'prime',
  4,  // <- newline allowed?
  9 => 'square',
  1 => 'one',
  default => 'other',
};
donquixote’s picture

That cspell for "colour" is out of scope here.

borisson_’s picture

I think the proposed text looks great, the next step is Review by the Coding Standards Committee.

I wonder if the note added in the MR Should go into a separate issue?

quietone’s picture

I pushed a suggested change. I repeated a sentence instead of expecting the reader to find the paragraph that this is the same as. Yes, even though it is just above. I think the 'note' heading isn't needed because of the phrase, 'to distinguish them from function calls.].

How to enforce this? Is there a Slevomat rule that can be used?

donquixote’s picture

I think the 'note' heading isn't needed because of the phrase, 'to distinguish them from function calls.].

This note was meant mostly for regex find/replace ninjas.
Maybe it is indeed too much.