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
- https://www.drupal.org/u/larowlan (2023-12-06)
- https://www.drupal.org/u/dww (2023-12-05)
- 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
Create this issue in the Coding Standards queue, using the defined templateAdd supportersCreate a Change Record- Review by the Coding Standards Committee
- Coding Standards Committee takes action as required
- Tagged with 'Needs documentation edits' if Core is not affected
- Discussed by the Core Committer Committee, if it impacts Drupal Core
- Documentation updates
- Edit all pages
- Publish change record
- Remove 'Needs documentation edits' tag
- If applicable, create follow-up issues for PHPCS rules/sniffs changes
For a full explanation of these steps see the Coding Standards project page
Issue fork coding_standards-3406368
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
Comment #2
larowlan+1 from me to make this consistent with if and else and enforced by a phpcs rule
Comment #3
dwwYes, please, good find. Adding myself (and @larowlan) as official supporters. Thanks!
Comment #4
arkener commented+1 makes sense to keep this consistent with other controle structures like `switch ()`
Comment #5
donquixote commentedComment #6
jonathan1055 commentedAdding support, so step 2 is now done. Tagging for "Needs change record"
Comment #7
quietone commentedNeeds work for the change record
Comment #8
borisson_Added change record.
Comment #9
drunken monkeyI expanded the text of the change record a bit, it was a bit too concise for my taste.
Comment #10
borisson_Comment #12
donquixote commentedI 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.
Comment #13
donquixote commentedMulti-value conditions can be refined later:
Comment #14
donquixote commentedThat cspell for "colour" is out of scope here.
Comment #15
borisson_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?
Comment #16
quietone commentedI 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?
Comment #17
donquixote commentedThis note was meant mostly for regex find/replace ninjas.
Maybe it is indeed too much.