Problem/Motivation

Resolve the PHP Coding standard issue.

FILE: /var/www/html/web/modules/custom/menus_attribute/menus_attribute.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
--------------------------------------------------------------------------------
 75 | WARNING | #options values usually have to run through t() for translation
 76 | WARNING | #options values usually have to run through t() for translation
 77 | WARNING | #options values usually have to run through t() for translation
 78 | WARNING | #options values usually have to run through t() for translation
 79 | WARNING | #options values usually have to run through t() for translation
--------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/custom/menus_attribute/menus_attribute.info.yml
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
 1 | WARNING | Remove "project" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
-------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/custom/menus_attribute/src/StorageHelper.php
----------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------
 12 | ERROR | Missing member variable doc comment
----------------------------------------------------------------------------

Steps to reproduce

Run the Code sniffer

Run the phpcs --standard="DrupalPractice,DrupalStandard" menus_attribute

Proposed resolution

Resolve the PHPCS issues.

Remaining tasks

Testing

User interface changes

API changes

Data model changes

Issue fork menus_attribute-3346900

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

arunkumark created an issue. See original summary.

arunkumark opened merge request !7

kunalgautam’s picture

kunalgautam commented
Status: Active » Needs review

PHPCS issues are resolved in MR of #2.

adil_siddiqui’s picture

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

MR reviewed and it is working fine...Marking as RTBC.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work 
+        '' => t('None(ie.same window)'),
+        '_blank' => t('New window (_blank)'),
+        '_top' => t('Top window (_top)'),
+        '_self' => t('Same window (_self)'),

Since those lines are changed, a space is missing before the opening parenthesis. ie.same is not the correct spelling, and i.e. would not even be necessary. (Instead, an article is necessary.)

+  /**
+   * Drupal\Core\Database\Database definition.
+   *
+   * @var Drupal\Core\Database\Database
+   */
   public $db;

That is not the comment used for properties. The namespace includes the \ at the beginning.

kkalashnikov opened merge request !8

kunalgautam’s picture

kunalgautam commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
+        '' => t('None(ie.same window)'),
+        '_blank' => t('New window(_blank)'),

See my previous comment. 

+  /**
+   * Database definition.
+   *
+   * @var Drupal\Core\Database\Database
+   */

The description is missing an article. Also, the usual description for that property does not say it is a definition.

kunalgautam’s picture

kunalgautam commented

Hello @apaderno thanks for reviewing.

Done all the changes you have mentioned in the comment.
Please review and let me know if anything is missing.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented 
+        '' => t('None(i.e.same window)'),
+        '_blank' => t('New window(_blank)'),
+        '_top' => t('Top window(_top)'),
+        '_self' => t('Same window(_self)'),
+        '_parent' => t('Parent window(_parent)'),

A space is missing before the opening parenthesis in the translated strings, in all those lines. Remove i.e. which is not necessary.

+  /**
+   * Database service.
+   *
+   * @var Drupal\Core\Database\Database
+   */

An article is missing from that description.

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

Updated the MR 8!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
+  /**
+   * Database Service Object.
+   *
+   * @var \Drupal\Core\Database\Database
+   */
   public $db;

The comment on the previous commit was better, although an article was missing. This comment still misses an article, it uses Object when it is not necessary, and wrongly capitalizes the words that are not at the beginning of the phrase.

kunalgautam’s picture

kunalgautam commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
+        '' => t('None (i.e.same window)'),
+        '_blank' => t('New window (_blank)'),
+        '_top' => t('Top window (_top)'),
+        '_self' => t('Same window (_self)'),
+        '_parent' => t('Parent window (_parent)'),

I have already commented on these change on comment #5; on comment #8 also said the changes reported on comment #5 were not done. There is still a line that has not been correctly changed.

+  /**
+   * The Database service.
+   *
+   * @var \Drupal\Core\Database\Database
+   */
   public $db;

Words that are not at the beginning of a sentence are not written capitalized, if they are not acronyms nor proper nouns. Database is neither a proper noun nor an acronym.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@kkalashnikov are you planning to work on this again?

themodularlab closed merge request !8

themodularlab closed merge request !7

  • themodularlab committed 71a43e28 on 8.x-1.x 
    Issue #3346900 by kkalashnikov, arunkumark, apaderno, Adil_Siddiqui,...
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
Status: Needs work » Fixed

This has been merged into the dev branch. It will be included in the next release. Thanks for everyone's help.

Status: Fixed » Closed (fixed)

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