Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a part of the attempt to fix #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard
This issue is created to tackle the sub-sniff Drupal.Commenting.FunctionComment.ParamNameNoMatch
To review:
$ composer install
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer
$ cd core
$ ../vendor/bin/phpcs -ps
Should result in no errors found.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-2722609-33-36.txt | 2.12 KB | mfernea |
#36 | drupal-coding-standards-2722609-36.patch | 27.29 KB | mfernea |
#33 | 2722609-33.patch | 26.7 KB | jofitz |
#33 | interdiff-29-33.txt | 1.56 KB | jofitz |
#29 | 2722609-29.patch | 26.65 KB | jofitz |
Comments
Comment #2
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedPlease find attached a patch that fixes the reported issue.
Do note that although this is a function comment fix patch, there are a couple of code changes in core/lib/Drupal/Component/Assertion/Inspector.php where the comment was correct but the code needed fixing.
Comment #3
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedComment #4
Mile23Updated issue summary with review instructions.
No phpcs errors other than unrelated Drupal.Commenting.FunctionComment.ThrowsComment.
Manual review says this...
Pretty sure these are set up that way for a reason. Don't change the method signature.
Put the |null after the type.
Comment #5
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedComment #6
chishah92 CreditAttribution: chishah92 at Blisstering Solutions commentedI have reverted the changes as suggested with a new patch
Thanks!
Chirag
Comment #8
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedThanks for the review Mile23. The last patch failed to apply. Rerolled for latest 8.2.x head and added interdiff against #2 as that was what was reviewed against.
Comment #9
Mile23Needs reroll since core/phpcs.xml.dist changed.
All lines like this should extend out to an 80 character wrap.
Should be...
View or current display to exclude. Either a:
All on one line.
Comment #10
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedThanks again for the review Mile23. I have rerolled the patch and have made the suggested changes.
Comment #11
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedComment #12
Mile23Needs a reroll for changed phpcs.xml.dist.
The rest looks pretty reasonable.
Comment #13
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedRerolled patch. BTW I was able to apply the patch with
git apply --3way patchname
. interdiff was not giving any output so did a diff.Comment #14
Mile23These must have been introduced since that patch:
There were some other errors in the log, but I deleted them and they're being handled here: #2747073: Fix Drupal CS regressions for Coder 8.2.8
Comment #15
drupradRerolled patch along with resolved issue as par comment #14
Comment #16
Mile23These docblocks need to stay because the functions use parameters.
It looks like we don't have a standard for this: https://www.drupal.org/coding-standards/docs#param
Plus coder can't adapt to just @param with a type and no variable name.
So we could wait on the whole thing until we have a standard for this, or we could re-scope this issue a little bit and only handle
Drupal.Commenting.FunctionComment.ParamCommentNotCapital
.Comment #20
mfernea CreditAttribution: mfernea at AmeXio commentedSince the patch is more than 1 year old and Drupal.Commenting.FunctionComment.ParamCommentNotCapital is fixed in #2902726: Fix 'Drupal.Commenting.FunctionComment.ParamCommentNotCapital' coding standard I guess it's best to focus only on Drupal.Commenting.FunctionComment.ParamNameNoMatch in this issue.
I also updated the review instructions.
Comment #21
mfernea CreditAttribution: mfernea at AmeXio commentedHere is the patch.
Comment #22
martin107 CreditAttribution: martin107 as a volunteer commentedpatch no longer applies.
and testbot is reporting a coding standard error to resolve.
I will lurk on this issue, with a hope of moving it to RTBC as soon as possible.
It is tricky every patch of this type causes others patches to need to reroll... I guess the only way to minimise reroll hell is to get each one committed before proceeding to the next.
Comment #23
martin107 CreditAttribution: martin107 as a volunteer commentedComment #24
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #25
mfernea CreditAttribution: mfernea at AmeXio commentedTwo more fixes required.
Comment #26
martin107 CreditAttribution: martin107 as a volunteer commentedI have had a careful comb through of all the changes... in each case the appropriate changes has been made.
Wow we had a lots of little screw-ups ... with a project of this age ... I guess that is to be expected.
test report no failures.
+1 from me
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved the "Needs reroll" tag.
Comment #28
martin107 CreditAttribution: martin107 as a volunteer commentedWe are going through a pattern of commit an issue then reroll all similar issues ... repeat
The only solution is to slow the rate of review and wait until the next active issue get committed. :(
Comment #29
jofitz CreditAttribution: jofitz at ComputerMinds commentedSimple re-rolls like that take no time at all.
Comment #30
martin107 CreditAttribution: martin107 as a volunteer commentedThanks ...
There is a scaling problem ... if I review 10 issues in a day .. that is potentially 9 rerolls.
Well 4 reviews in motion is doable ... but still unwelcome.
Anyway.. this looks good.
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commented@martin107 Maybe postpone a few?
Comment #32
catchThe conflicts are all due to the phpcs.yml.dist changes - if we moved those to one patch on one issue, the code style fixes could go in separately without conflicts.
Noticed a couple of things with the patch:
$args is no longer used, so we could skip both lines?
Same here.
$old_name and $new_name are more descriptive, why not change the argument variables instead of the docs
The description should match the param but now doesn't.
Comment #33
jofitz CreditAttribution: jofitz at ComputerMinds commentedI noticed a number of other documentation errors in Drupal\Core\Executable\ExecutablePluginBase, but did not consider them relevant to this issue so have opened a follow-up: #2904711: Missing and incorrect parameter, return, and thrown exception data types in Drupal\Core\Executable\ExecutablePluginBase.
Comment #35
mfernea CreditAttribution: mfernea at AmeXio commentedI rerun the tests and everything seems to be fine now. The error was not related to the code in this patch.
Modifications look fine. I agree with Jo's notes for 1. and 2..
Comment #36
mfernea CreditAttribution: mfernea at AmeXio commentedI took another look at the params' comments that we've modified in this issue and I think 3 more modifications are needed.
Comment #37
martin107 CreditAttribution: martin107 as a volunteer commentedRegarding the changes $args issue ... yep the patch is correct.
From 31,
I don't to stop other eyes passing over the issues completely .. it is a tricky balance.
My plan is to try and work down the list from top to bottom ....and stick on one 'til done.
I am happy with all the recent changes.... they improve things and I don't want to get hung up on what is out of scope and what is not.
Comment #38
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!