Problem/Motivation
FILE: ...eb/modules/contrib/content_close/src/Controller/ContentCloseController.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
9 | WARNING | The class short comment should describe what the class does and
| | not simply repeat the class name
--------------------------------------------------------------------------------
FILE: ...on/drupal9/web/modules/contrib/content_close/src/Form/ContentCloseForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
13 | WARNING | The class short comment should describe what the class does and
| | not simply repeat the class name
65 | WARNING | t() calls should be avoided in classes, use
| | \Drupal\Core\StringTranslation\StringTranslationTrait and
| | $this->t() instead
74 | WARNING | t() calls should be avoided in classes, use
| | \Drupal\Core\StringTranslation\StringTranslationTrait and
| | $this->t() instead
--------------------------------------------------------------------------------
FILE: ...tribution/drupal9/web/modules/contrib/content_close/content_close.info.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
1 | WARNING | "core_version_requirement" property is missing in the info.yml
| | file
--------------------------------------------------------------------------------
FILE: ...ontribution/drupal9/web/modules/contrib/content_close/content_close.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
42 | ERROR | The array declaration extends to column 100 (the limit is 80).
| | The array content should be split up over multiple lines
--------------------------------------------------------------------------------
Time: 308ms; Memory: 10MB
Steps to reproduce
Execute command: phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,js,info,txt,md,yml,twig content_close/
Proposed resolution
Fix all the issues for Drupal and DrupalPractice standards
Remaining tasks
Patch Review
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | interdiff-4-7.txt | 1.43 KB | nitin_lama |
| #7 | phpcs-issue-fixes-3357104-7.patch | 3.39 KB | nitin_lama |
| #4 | phpcs-issue-fixes-3357104-4.patch | 3.37 KB | ashutosh ahirwal |
| #2 | Screenshot 2023-04-28 at 6.27.42 PM.png | 54.6 KB | ashutosh ahirwal |
| coding-standard-fixes.patch | 2.85 KB | urvashi_vora |
Issue fork content_close-3357104
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
ashutosh ahirwal commentedPatch applied cleanly
Moving to RTBC
Comment #3
avpadernoThe patch could apply, but that does not mean it is correct.
Lines are allowed to be longer than 80 characters, as long as they are easier to read. In this case, the changed code is not easier to read; the code should stay as it was.
No, the short description for a class does not start with Implements. A class does not implement itself.
Furthermore, that short description is just repeating the class name. It does not say anything about the class purpose.
Comment #4
ashutosh ahirwal commentedHi
I have tried to fix the issue according to #3.
please review.
Comment #5
avpadernoYet, the existing code is easier to understand.
No, a class does not provide functionality for itself. Classes provide functionality for any class/function using them. Since that is what any class does, a description like that is not helpful.
Only the first word must be capitalized.
I am not sure that description is sufficient. content close form does not say me much.
Comment #6
nitin_lamaComment #7
nitin_lamaAddressed #5.
For #5.1 i get it that the previous code is easier to understand but as per php code sniffer this has to be fixed since the array declaration extends to column 100 (the limit is 80).
Providing updated patch.
Comment #8
nitin_lamaComment #9
avpadernoCode lines are not required to be shorter than 81 characters. Line length and wrapping, part of the Drupal coding standards, says:
(Emphasis is mine.)
The existing code is easier to understand.
That is not a correct description, since a controller class does not implement content nor does it return "theme with values." If that were a correct description, it would be too generic; the description must say what exactly that class does.
That description is as clear as the existing description. Content close configuration is not a set phrase, which means it does not have any meaning that makes clear what that class purpose is.
Comment #10
nitin_lamaComment #13
avpadernoComment #16
mahtab_alam commented