Problem/Motivation
This is a part of the attempt to fix #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard
This issue is created to work on the sub-sniff Drupal.Commenting.FunctionComment.MissingParamComment
.
The sniff implements the @param: Function parameters coding standard.
Proposed resolution
Add comments, as best as possible, to allow the sniff to pass. Further improvements to the comments can be made in other issues.
Remaining tasks
Commit
Release notes snippet
The following coding standards check has been enabled in core:
Drupal.Commenting.FunctionComment.MissingParamComment
Comment | File | Size | Author |
---|---|---|---|
#12 | no errors.txt | 12.36 KB | guilhermevp |
Issue fork drupal-3207567
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
quietone CreditAttribution: quietone as a volunteer commentedMaking a start.
Comment #3
SpokjeI have no clue how this could pass TestBot, because if I apply the patch in #2 and run
phpcs
I get 28 files where there's aMissing parameter comment
left.This is in no way a comment on the work of @quietone, but I would have expected TestBot to die in the
Custom Command
phase, wherephpcs
is being ran.This because of the fact that failing the sniff
Drupal.Commenting.FunctionComment.MissingParamComment
throws an ERROR, not a WARNING:I checked the patch, and the sniff
Drupal.Commenting.FunctionComment.MissingParamComment
is removed from the exclude list in/core/phpcs.xml.dist
.@quietone: If you rerun
phpcs
after applying the patch, do you also get 28 more files to fix?Comment #4
quietone CreditAttribution: quietone as a volunteer commentedDoesn't it pass testbot because commit-code-check is only run on the files in the patch and not the entire code base?
Yes, there is more work to do, I applied the patch and reran phpcs and got 35 files with errors. Assigning to myself to do over this weekend.
Comment #5
SpokjeAs usual, you're hitting the nail on the head @quiteone.
Thanks for guiding this lost spirit back into the land of reason. :)
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedStill more to do here. I've included the output of phpcs showing the remaining files to fix..
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedComment #8
quietone CreditAttribution: quietone as a volunteer commentedComment #9
SpokjeCreating MR with
3207567-6.patch
as a base.Comment #11
SpokjeComment #12
guilhermevp CreditAttribution: guilhermevp at CI&T commentedTesting f761bc09 for
<rule ref="Drupal.Commenting.FunctionComment.MissingParamComment"/>
returned zero errors.Not moving to RTBC due to merge request error.
Comment #13
SpokjeThanks @guilhermevp
I merged the HEAD of
9.2.x
back into the MR.Let's see what TestBot thinks in https://www.drupal.org/pift-ci-job/2043894
Comment #14
SpokjeComment #15
SpokjeRight...
A gazillion merges with HEAD and some random test-failures later: Back to
NR
.Comment #16
SpokjeHe said, whilst _not_ putting it on
NR
in the same comment :/Comment #18
SpokjeRebased MR against
9.3.x
.Comment #19
guilhermevp CreditAttribution: guilhermevp at CI&T commentedFound one error testing phpcs, added merge request addressing this error, please review.
Comment #20
guilhermevp CreditAttribution: guilhermevp at CI&T commentedPassing the sniff returned this error:
If you wanna I can take care of it @Spokje, but since you been tackling this issue maybe you wanna do it.
(felt intrusive doing it in
59cef53c
)Probably a never ending work as more fixes are incoming not complying with the rule.
Comment #21
SpokjeThanks @guilhermevp and @longwave :)
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedI applied the MR and ran phpcs on the codebase and no other instances were detected.
Comment #23
daffie CreditAttribution: daffie commentedDrupal's styleguide for optional parameter, see: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm....
Comment #24
SpokjeThanks @daffie for the eagle-eyed review.
- Resolved all threads
- Merge latest
9.3.x
Comment #25
daffie CreditAttribution: daffie commentedSorry, I found a couple more. After these ones it is RTBC for me.
Comment #26
SpokjeThanks @daffie!
You should never apologize for doing the right thing.
(Hmmm, not sure how we ended up being in the closing scene of an 80's family comedy episode there...)
Resolved all threads, back to NR.
Comment #27
daffie CreditAttribution: daffie commentedAll code changes look good to me.
There is a rule change in phpcs.xml.dist.
For me it is RTBC.
@Spokje: Great work.
Comment #28
alexpottSome new things to fix.
Comment #29
yogeshmpawarWorking on it.
Comment #30
yogeshmpawarComment #31
yogeshmpawarComment #32
yogeshmpawar@Spokje - Can you please make the MR ready as it's in draft state now & I have done the mentioned changes.
Comment #33
SpokjeRebased MR on Drupal 9.4.x, marked it as ready and merged it with the latest changes in the 9.4.x branch.
Comment #34
paulocsI checked ALL comments added and ran
core/scripts/dev/commit-code-check.sh
. I did not find any issue so moving to RTBC.Comment #35
quietone CreditAttribution: quietone as a volunteer commented@paulocs, thanks. Since commit-code-check only runs for changed files we need to apply the patch and run a scan on all of core. The steps are in #4 of the issue summary for #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard
I used the following steps to confirm that the sniff does not find any other occurrences in core. Leaving at RTBC.
Comment #36
alexpottI removed the file that was being incorrectly added back to core.
I think we need a release note to document that core ships with this sniff configured.
Committed and pushed 576bd7543d to 9.4.x and 2126f4c764 to 9.3.x. Thanks!
Comment #39
longwaveComment #41
paulocsAh okay. Thanks @quietone.