Problem/Motivation

PHPCS reported few errors and warnings mentioned in text document "PHPCS-report.txt"

Steps to reproduce

Execute the command: phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,js,info,txt,md,yml,twig integration_chargebee/

Proposed resolution

Fix all the errors and warnings

Remaining tasks

Patch Review.

Excluded "\Drupal calls should be avoided in classes, use dependency injection instead" error because it is already in progress with "https://www.drupal.org/project/integration_chargebee/issues/3360299"

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.

avpaderno’s picture

Status: Needs review » Needs work
   requirements:
+    # Access Admin Page Permission
     _permission: 'access administration pages'

The warning is the following.

The administration page callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.

Adding a comment before the _permission line does not change the required permission for those routes.

Rajan Kumar made their first commit to this issue’s fork.

rajan kumar’s picture

Status: Needs work » Needs review

Please review and merge code.

avpaderno’s picture

Status: Needs review » Needs work
-    ->fields('sub', ['uid', 'subscription_id', 'plan_id', 'status', 'current_term_start', 'current_term_end'])
+    ->fields('sub', [
+      'uid',
+      'subscription_id',
+      'plan_id',
+      'status',
+      'current_term_start',
+      'current_term_end',
+    ])

Code lines are allowed to exceed 80 characters, if they are more readable. The existing code is more readable than the changed code.

     _controller: '\Drupal\integration_chargebee\Controller\PaymentController::success'
     _title: 'Thank you'
   requirements:
-    _access: 'TRUE'
+    _permission: 'access content'

The report does not say that _access: 'TRUE' must be removed from that route definition, but that a comment explaining why _access: 'TRUE' is used must be added.

     _controller: '\Drupal\integration_chargebee\Controller\CheckoutController::plan'
     _title: 'Subscribe Plan'
   requirements:
-    _access: 'TRUE'
-
+    _permission: 'access content'

The same holds true for this route definition.

+   * The ConfigFactory.
+   *
    * @var \Drupal\Core\Config\ConfigFactory

ConfigFactory is misspelled.

+   * @param \Drupal\Core\Config\ConfigFactory $configFactory
+   *   The renderer.

The description is wrong.

+  /**
    * Construct a new AuthController object.
    *
+   * @param \Drupal\Core\Config\ConfigFactory $configFactory

Since that comment is change, also the method description must be changed: The description for a constructor must start with Constructs a new followed by the class name (including its namespace), and end with object.

   /**
-   * integration_chargebee Service.
+   * Integration_chargebee Service.

integration_chargebee is the module machine name and it does not describe this service's purpose.
Service is misspelled, since it must be spelled using lowercase letters.

+  /**
    * Drupal config.
    *
    * @var \Drupal\Core\Config\ConfigFactoryInterface
    */
   protected $configFactory;

Since that documentation comment is changed, also the property description must be changed, as property descriptions must not start with Drupal.

+  /**
+   * The RequestStack service.
+   *
+   * @var Symfony\Component\HttpFoundation\RequestStack
+   */
+  protected $requestStack;

The request stack. is a better description.

    *   The config factory for the form.
+   * @param Symfony\Component\HttpFoundation\RequestStack $requestStack
+   *   The request service.

The request stack. is a better description.

- /**
+  /**
    * Entity manager.
    *
    * @var \Drupal\Core\Entity\EntityTypeManagerInterface
    */
   protected $entityTypeManager;
 
- /**
+  /**
    * Database connection.
    *

Since those comments have been changed, a definite article must be added to both the descriptions.

+  /**
+   * Class Constructor.
    *
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory for the form.
    */
+  public function __construct(ConfigFactoryInterface $config_factory) {

The description for a constructor must start with Constructs a new followed by the class name (including its namespace), and end with object.

rajan kumar’s picture

Status: Needs work » Needs review
avpaderno’s picture

Status: Needs review » Needs work
+    # Access granted in all circumstances.
     _access: 'TRUE'

That does not explain why.

   /**
+   * The Config Factory.
+   *
    * @var \Drupal\Core\Config\ConfigFactory

Only the first word in the description must be capitalized.

+  /**
+   * Construct a new CheckoutController object.

The class namespace is missing.

 /**
- * An example controller.
+ * An User controller.
  */

The indefinite article to use with user is the other one.

    *   Connection.
    * @param \Drupal\Core\Messenger\Messenger $messenger
    *   Messenger.

Since that documentation comment is changed, the definite article needs to be added to both those descriptions.

 /**
- * Class ChargebeePlanForm.
+ * Class Chargebee Plan Form.

That is still repeating the class name. Adding spaces does not change that.

    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory for the form.

Since that documentation comment is changed, that description must be changed too: The config factory. is sufficient.

+  /**
+   * Create tableselect for plans list.
+   */
+  public function integrationChargebeeCreatePlansSelectTable($plans) {

The verb must be declined to the third person singular.

rajan kumar’s picture

Status: Needs work » Needs review
avpaderno’s picture

Status: Needs review » Needs work
shashank5563’s picture

Status: Needs work » Needs review
shashank5563’s picture

@apaderno, For the permission issue, I will create another issue. Now, I merging this issue.

shashank5563’s picture

Status: Needs review » Fixed

@apaderno, I am marking fixed this issue. I will create another issue for the permission.

Status: Fixed » Closed (fixed)

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