Problem/Motivation
As per #3440075: Add rule for multi-line function declarations we now allow multi line function declarations. This sniff was released in coder 8.3.24. Issues: #3440075: Add rule for multi-line function declarations and #3440603: Validate trailing comma in multi-line function declaration. We need to enable the correct config for that sniff and make sure all is fine.
Steps to reproduce
Proposed resolution
Update coder
Update phpcs.xml.dist
Remaining tasks
Update coderUpdate phpcs.xml.distValidate outputFix issues
User interface changes
n.a.
API changes
n.a.
Data model changes
n.a.
Release notes snippet
The coding standards rule Drupal.Functions.MultiLineFunctionDeclaration has been enabled.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 3443117-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #18 | 3443117-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3443117
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:
- 3443117-11.x
changes, plain diff MR !7693
- 3443117-coding-standard-allow
changes, plain diff MR !7688
Comments
Comment #2
bbralaChecked against the meta issue, seems nothing is related/adjecent.
Comment #3
bbralaAfter running locally, there are quite a few issues in core regarding this coding standard.
Comment #5
bbralaRan with:
gitlab-ci-local --variable CI_PIPELINE_SOURCE=merge_event "<0001f9f9> PHP Coding standards (PHPCS)" --variable COMPOSER_ROOT_VERSION=10.3.x-dev --needsMost can be automaticly fixed it seems.
Comment #6
bbralaAdded extra ignore to DrupalKernel because that had a different failure after update.
Fixed all issues automaticly, yay. Cleaning up a lot of code.
Comment #7
longwaveThanks, MR looks good for 10.3.x, we will also need another MR to apply against 11.x.
Comment #8
quietone commentedYay indeed!
Making the title the same style as other coding standard issues in the core queue. And switching the parent and related.
Comment #9
longwaveAlso DemoUmamiProfileTest has failed twice, I thought it was random so I re-ran the job but it failed again.
Comment #11
longwaveOpened MR for 11.x.
Comment #12
bbralaThe drupal 11 MR looks good and is all green. I'll see if I can find out what is failing in 10.3
Comment #13
bbralaThere is no reason afaik that that test should fail. Rebased, trying again. Also looked if perhaps php version was an issue, but nothing notable... Arg.
Comment #14
bbralaSeems there are more failures on that test in other places. Suspecting its a HEAD issue in 10.3.x
Comment #15
bbralaThe core issue has been fixed. Mr for 10.3 should pass again.
https://www.drupal.org/project/drupal/issues/3443248
Comment #16
bbralaComment #17
smustgrave commented#3443248: Can't log out on translated site in tests - causes issues for 10.3 umami test was committed so probably just needs a rebase.
Comment #18
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #19
longwaveComment #20
bbralaRebased and fixed 2 remaining (new) issues in 11.x
Comment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #22
bbralaBot is lying. It applies.
Comment #23
nod_Comment #24
longwaveComment #25
smustgrave commentedMR 7693 appears to need manual rebase.
Comment #27
bbralaComment #28
bbralaUpdated new code to standard again and fixed a conflict with 11.x
Comment #29
longwaveYay, this is really nice cleanup, let's land this before the betas.
Comment #35
catchLooks great. Might be the quickest we've been compliant with a coding standard after it's agreed? Committed/pushed to 11.x/11.0.x/10.4.x/10.3.x respectively.
Comment #37
bbralaThanks catch. And maybe it is ;)
Comment #39
teknocat commentedIn reviewing my current codebase, I can't seem to get drupal/coder 8.3.24 due to the following dependency chain:
I have Drush 12.5.2 installed which, at the time of this writing, is the latest version. I'm currently running Drupal 10.3.
Can somebody advise me on how to update? Alternatively I could update my phpcs.dist.xml file to reference globally installed drupal/coder sniffs as well, where I have 8.3.24 installed. However, that doesn't seem like the best approach and is less portable if other devs working on the same project don't also have the sniffs installed globally in the same location as me.
Comment #40
xjmWe don't release-notes-ify CS rules anymore unless they're functionally disruptive somehow (e.g. requiring typehints or whatnot). Thanks!