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

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:

Comments

urvashi_vora created an issue. See original summary.

ashutosh ahirwal’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new54.6 KB

Patch applied cleanly
Moving to RTBC

avpaderno’s picture

Priority: Normal » Minor
Status: Reviewed & tested by the community » Needs work

The patch could apply, but that does not mean it is correct.

-      $url = Url::fromRoute('content_close.page', ['time' => $time, 'content_type' => $content_type])->toString();
+      $url = Url::fromRoute('content_close.page',
+              [
+                'time' => $time,
+                'content_type' => $content_type,
+              ])->toString();

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.

 /**
- * ContentCloseController class.
+ * Impements ContentCloseController class.
  */
 class ContentCloseController extends ControllerBase {

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.

ashutosh ahirwal’s picture

Status: Needs work » Needs review
Issue tags: +Coding standards
StatusFileSize
new3.37 KB

Hi
I have tried to fix the issue according to #3.
please review.

avpaderno’s picture

Status: Needs review » Needs work
-      $url = Url::fromRoute('content_close.page', ['time' => $time, 'content_type' => $content_type])->toString();
+      $url = Url::fromRoute('content_close.page', [
+        'time' => $time, 'content_type' => $content_type,
+        ])->toString();

Yet, the existing code is easier to understand.

 /**
- * ContentCloseController class.
+ * Class to provide functionality for ContentCloseController.
  */

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.

 /**
- * ContentCloseForm close.
+ * Class to build the Content Close Form.
  */

Only the first word must be capitalized.
I am not sure that description is sufficient. content close form does not say me much.

nitin_lama’s picture

Assigned: Unassigned » nitin_lama
nitin_lama’s picture

Status: Needs work » Needs review
StatusFileSize
new3.39 KB
new1.43 KB

Addressed #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).

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

Providing updated patch.

nitin_lama’s picture

Assigned: nitin_lama » Unassigned
avpaderno’s picture

Status: Needs review » Needs work
     if ($content_type == $form_id && $time < $request_time) {
-      $url = Url::fromRoute('content_close.page', ['time' => $time, 'content_type' => $content_type])->toString();
+      $url = Url::fromRoute('content_close.page', [
+        'time' => $time,
+        'content_type' => $content_type,
+      ])->toString();

Code lines are not required to be shorter than 81 characters. Line length and wrapping, part of the Drupal coding standards, says:

  • Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 characters.
  • Control structure conditions may exceed 80 characters, if they are simple to read and understand.

(Emphasis is mine.)

The existing code is easier to understand.

 /**
- * ContentCloseController class.
+ * Controller class to implement content and return theme with values.
  */

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.

 /**
- * ContentCloseForm close.
+ * Content close configuration form.
  */
 class ContentCloseForm extends ConfigFormBase {
 

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.

nitin_lama’s picture

Assigned: Unassigned » nitin_lama

bharath-kondeti made their first commit to this issue’s fork.

avpaderno’s picture

Assigned: nitin_lama » Unassigned

mahtab_alam made their first commit to this issue’s fork.

mahtab_alam’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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