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

CommentFileSizeAuthor
#12 no errors.txt12.36 KBguilhermevp
#6 3207567-6.patch59.59 KBquietone
#6 phpcs-output.txt9.87 KBquietone
#6 interdiff-2-6.txt13.95 KBquietone
#2 3207567-2.patch41.96 KBquietone

Issue fork drupal-3207567

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:

About issue forks

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3207567-2.patch41.96 KB

Making a start.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work

I 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 a Missing 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, where phpcs is being ran.
This because of the fact that failing the sniff Drupal.Commenting.FunctionComment.MissingParamComment throws an ERROR, not a WARNING:

  63 | ERROR | Missing parameter comment

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?

quietone’s picture

quietone commented
Assigned: Unassigned » quietone

Doesn'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.

spokje’s picture

spokje
Primary language Dutch
commented

Doesn't it pass testbot because commit-code-check is only run on the files in the patch and not the entire code base?

As usual, you're hitting the nail on the head @quiteone.

Thanks for guiding this lost spirit back into the land of reason. :)

quietone’s picture

quietone commented
StatusFileSize
new interdiff-2-6.txt13.95 KB
new phpcs-output.txt9.87 KB
new 3207567-6.patch59.59 KB

Still more to do here. I've included the output of phpcs showing the remaining files to fix..

quietone’s picture

quietone commented
Assigned: quietone » Unassigned
quietone’s picture

quietone commented
Issue tags: +Coding standards
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje

Creating MR with 3207567-6.patch as a base.

Spokje opened merge request !548

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
guilhermevp’s picture

guilhermevp commented
StatusFileSize
new no errors.txt12.36 KB

Testing f761bc09 for <rule ref="Drupal.Commenting.FunctionComment.MissingParamComment"/> returned zero errors.

Not moving to RTBC due to merge request error.

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Bug Smash Initiative

Thanks @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

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented

Right...

A gazillion merges with HEAD and some random test-failures later: Back to NR.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

A gazillion merges with HEAD and some random test-failures later: Back to NR.

He said, whilst _not_ putting it on NR in the same comment :/

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented

Rebased MR against 9.3.x.

guilhermevp’s picture

guilhermevp commented

Found one error testing phpcs, added merge request addressing this error, please review.

guilhermevp’s picture

guilhermevp commented
Status: Needs review » Needs work

Passing the sniff returned this error:

FILE: /home/guilhermevp/Ambientes/drupalOrg/9.3D/drupal/core/tests/Drupal/Tests/DocumentElement.php
---------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------
 40 | ERROR | Missing parameter comment
    |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
---------------------------------------------------------------------------------------------------

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.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @guilhermevp and @longwave :)

quietone’s picture

quietone commented
Issue summary: View changes

I applied the MR and ran phpcs on the codebase and no other instances were detected.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Drupal's styleguide for optional parameter, see: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm....

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @daffie for the eagle-eyed review.

- Resolved all threads
- Merge latest 9.3.x

daffie’s picture

daffie commented
Status: Needs review » Needs work

Sorry, I found a couple more. After these ones it is RTBC for me.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @daffie!

Sorry, I found a couple more.

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.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
There is a rule change in phpcs.xml.dist.
For me it is RTBC.

@Spokje: Great work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
FILE: /Users/alex/dev/drupal/core/modules/jsonapi/tests/src/Functional/UserTest.php
-------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------
 805 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 806 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
-------------------------------------------------------------------------------------------------


FILE: /Users/alex/dev/drupal/core/modules/file/src/Upload/FileUploadResult.php
------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------
 53 | ERROR | Missing parameter comment
    |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 74 | ERROR | Missing parameter comment
    |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 86 | ERROR | Missing parameter comment
    |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
------------------------------------------------------------------------------------------------

Some new things to fix.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar

Working on it.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented

@Spokje - Can you please make the MR ready as it's in draft state now & I have done the mentioned changes.

spokje’s picture

spokje
Primary language Dutch
commented

Rebased MR on Drupal 9.4.x, marked it as ready and merged it with the latest changes in the 9.4.x branch.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Reviewed & tested by the community

I checked ALL comments added and ran core/scripts/dev/commit-code-check.sh. I did not find any issue so moving to RTBC.

quietone’s picture

quietone commented
Issue summary: View changes

@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.

<ol>
  <li>Applied patch</li>
  <li>$ cd core/</li>
  <li>$ ../vendor/bin/phpcs -ps</li>
  <li>$</li>
</ol>
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs release note

I 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!

  • alexpott committed 576bd75 on 9.4.x 
    Issue #3207567 by Spokje, quietone, guilhermevp, yogeshmpawar, daffie,...

  • alexpott committed 2126f4c on 9.3.x 
    Issue #3207567 by Spokje, quietone, guilhermevp, yogeshmpawar, daffie,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
Issue tags: -Needs release note +9.3.0 release notes

Spokje closed merge request !548

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Ah okay. Thanks @quietone.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.