Fix the issues reported by phpcs

Added patch for the above fixes here, please review it.

Hi mrinalini9

I applied patch #2 to the “Simple Popup Blocks” module and confirmed that the mentioned PHPCS issues are resolved. Please see the screenshots attached.

For your review.
Thank you.

   /**
    * Messenger object.
    *
-   * @var Messenger
+   * @var \Drupal\Core\Messenger\Messenger
    */
   protected $messenger;
 
   /**
    * Connection object.
    *
-   * @var Connection
+   * @var \Drupal\Core\Database\Connection
    */
   private $database;

Since those documentation comments are edited, the descriptions must be edited too, since they are missing an article each.
It is not necessary to say Connection object; connection is sufficient.

   /**
+   * The Config Factory object.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected $configFactory;

Only the first word in the description is capitalized. The other words are not capitalized.

+  /**
    * {@inheritdoc}
    */
-  public function __construct(Messenger $messenger, Connection $database) {
+  public function __construct(Messenger $messenger, Connection $database, ConfigFactoryInterface $config_factory) {

{@inheritdoc} is not used for class constructors.

   /**
    * Messanger object.
    *
-   * @var Messenger
+   * @var \Drupal\Core\Messenger\Messenger
    */
   protected $messenger;

Since that documentation comment is edited, also the short description need to be edited. It misses an article and there is a typo in Messanger.

+  /**
    * SimplePopupBlocksAddForm constructor.
    *
-   * @param EntityTypeManager $typeManager
-   * @param Messenger $messenger
-   * @param Connection $database
+   * @param \Drupal\Core\Entity\EntityTypeManager $typeManager
+   *   The entity type manager.
+   * @param \Drupal\Core\Messenger\Messenger $messenger
+   *   The messenger service.
+   * @param \Drupal\Core\Database\Connection $database
+   *   The database connection.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The Config Factory.
    */
-  public function __construct(EntityTypeManager $typeManager, Messenger $messenger, Connection $database ) {
+  public function __construct(EntityTypeManager $typeManager, Messenger $messenger, Connection $database, ConfigFactoryInterface $config_factory) {

The short description for a constructor starts with Constructs a new followed by the class name (namespace included) and ends with object.

-    $config = \Drupal::service('config.factory')->getEditable('simple_popup_blocks.popup_'.$form_state->getValue('uid'));
+    $config = \Drupal::service('config.factory')->getEditable('simple_popup_blocks.popup_' . $form_state->getValue('uid'));

Form classes should use dependency injection, not calls to \Drupal methods. The configuration factory has been already injected and it should be used.

   /**
    * Save an entry in the simple_popup_blocks table.
    *
    * @param array $entry
    *   An array containing all the fields of the database record.
-   *
-   * @param Connection $database
+   * @param \Drupal\Core\Database\Connection $database
    *   Connection object.

Since that documentation comment is edited, the verb used in the short description needs to be edited too, as it needs to use the third person singular.

Fixed the issues mentioned in #4. Please review.

   /**
-   * Connection object.
+   * Connection.

The article is missing. Since the constructor parameter is described as The database connection. the same description should be used here.

   /**
-   * {@inheritdoc}
+   * The Config Factory object.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected $configFactory;

That property is already defined from the parent class. This class does not need to define it again.

+  /**
+   * Constructs a new SimplePopupBlocksController object.
+   *

The short description is missing the class namespace.

-    $configFactory = \Drupal::service('config.factory');
+    $configFactory = $this->configFactory();
     $configs = $configFactory->listAll('simple_popup_blocks');

There is not a configFactory() method. The method is ControllerBase::config().

   /**
-   * Messanger object.
+   * The messenger service.
    *
-   * @var Messenger
+   * @var \Drupal\Core\Messenger\Messenger
    */
   protected $messenger;

That property is already defined by the parent class. Check which methods are inherited from the parent classes and verify which ones should be used.

+   * @var \Drupal\Core\Config\ConfigFactoryInterface
    */
-  public function __construct(EntityTypeManager $typeManager, Messenger $messenger, Connection $database ) {
+  protected $configFactory;

That property is already defined from the parent class.

-    $config = \Drupal::service('config.factory')->getEditable('simple_popup_blocks.popup_'.$uid);
+    $config = $this->configFactory->getEditable('simple_popup_blocks.popup_' . $uid);

The method to use is $this->config().

 class SimplePopupBlocksEditForm extends SimplePopupBlocksAddForm {
 
+  use StringTranslationTrait;
+
+  /**
+   * The configuration factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected ConfigFactoryInterface $configFactory;
+
   /**
    * {@inheritdoc}
    */
@@ -18,19 +29,31 @@ class SimplePopupBlocksEditForm extends SimplePopupBlocksAddForm {
     return 'simple_popup_blocks_edit_form';
   }
 
+  /**
+   * {@inheritdoc}
+   */
   protected function getEditableConfigNames() {
     return [
       'simple_popup_blocks_add_form.settings',
     ];
   }

Given the defined methods, I get the parent class already has a $configFactory property this class does not need to redefine.

+  /**
+   * Constructs a AddToAnySettingsForm object.
+   *
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The factory for configuration objects.
+   */
+  public function __construct(ConfigFactoryInterface $configFactory) {
+    $this->configFactory = $configFactory;
+  }

Since that code is in the SimplePopupBlocksEditForm.php file, the constructor cannot be for the AddToAnySettingsForm class.
The constructor description must start with Constructs a new followed by the class name (namespace included).

-   * @param Connection $database
+   * @param \Drupal\Core\Database\Connection $database
    *   Connection object.

Since the description used in another file is The database connection. that should be the description used in this case too.

-   * @param Messenger $messenger
+   * @param \Drupal\Core\Messenger\Messenger $messenger
    *   Messenger object.

The article is missing from the description.

Working on this.

Update patch and interdiff file attached.

Including the class namespace in constructor description exceeds the character limit and show and error in phpcs.
Assuming this can be ignored. All the suggestion mentioned in #7 📌 Fix the issues reported by phpcs Needs review are updated in the patch.

Please review.

fixed some phpcs issues

 function simple_popup_blocks_update_8104(&$sandbox) {
 
-    $trigger_width = [
-      'type' => 'int',
-      'not null' => FALSE,
-      'default' => NULL,
-      'description' => "Trigger width in px",
-    ];
+  $trigger_width = [
+    'type' => 'int',
+    'not null' => FALSE,
+    'default' => NULL,
+    'description' => "Trigger width in px",
+  ];

Since that code is changed, the empty line after the function declaration must be removed.

 use Drupal\Core\Database\Connection;
-use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Entity\EntityTypeManager;
 use Drupal\Core\Messenger\Messenger;
-use Drupal\Core\Url;
 use Drupal\Core\Form\ConfigFormBase;
-use Drupal\Core\Form\FormInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Config\ConfigFactoryInterface;

use statements must be ordered alphabetically. This is also reported from the recent rulesets for Drupal.

    * Object of EntityTypeManager.
    *
-   * @var EntityTypeManager
+   * @var \Drupal\Core\Entity\EntityTypeManager

Since that comment has been changed, even the description must be changed.

-   * Messanger object.
+   * The messenger service.
    *
-   * @var Messenger
+   * @var \Drupal\Core\Messenger\Messenger

It is not necessary to say service.

   /**
-   * Database object.
+   * Connection.
    *
-   * @var Connection
+   * @var \Drupal\Core\Database\Connection
    */
   protected $database;

The usual description is The database connection.

+  /**
+   * Constructs a new SimplePopupBlocksAddForm object.
+   *
+   * @param \Drupal\Core\Entity\EntityTypeManager $typeManager
+   *   The entity type manager.
+   * @param \Drupal\Core\Messenger\Messenger $messenger
+   *   The messenger service.
+   * @param \Drupal\Core\Database\Connection $database
+   *   The database connection.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The Config Factory.
+   */

The documentation comment for constructors is not mandatory anymore. If it is added, the class name in the description must include the namespace.

+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The factory for configuration objects.

The configuration factory. is sufficient.

    * Save an entry in the simple_popup_blocks table.
    *
    * @param array $entry
    *   An array containing all the fields of the database record.
-   *
-   * @param Connection $database
-   *   Connection object.
-   *
-   * @param Messenger $messenger
-   *   Messenger object.
+   * @param \Drupal\Core\Database\Connection $database
+   *   The database connection.
+   * @param \Drupal\Core\Messenger\Messenger $messenger
+   *   The messenger service.

Since that comment is changed, Save must be replaced with Saves.

-   * @param Connection $database
+   * @param \Drupal\Core\Database\Connection $database
    *   Connection object.
-   *
-   * @param Messenger $messenger
+   * @param \Drupal\Core\Messenger\Messenger $messenger
    *   Messenger object.

Those descriptions are each missing a definite article.

Patch updated.
Please review.

Please ignore the #13 patch.

I have tested and got one phpcs issue. I have fixed in new patch.

-      if (windowWidth < values.trigger_width) {return null;};
+      if (windowWidth < values.trigger_width) {return NULL;};

In Javascript, null, true, and false are used.

+   * Constructs a new.
+   *
+   * Drupal\simple_popup_blocks\Controller\SimplePopupBlocksController.
+   * object.

A method description goes on a single line.

+   * @param \Drupal\Core\Messenger\Messenger $messenger
+   *   The messenger.
+   * @param \Drupal\Core\Database\Connection $database
+   *   The messenger.

The second parameter is not the messenger, which is the first parameter.

+   * The config factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface

There is no need to define that property, as it is already provided by the parent class.

+ $data = $this->configFactory->get($config);

The parent class provides config() for exactly that purpose.

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

That property is already defined from the parent class (ConfigFormBase).

-      '#default_value' => t("custom-css-id"),
+      '#default_value' => $this->t("custom-css-id"),

The default value must be the previously entered value, which is stored in the configuration object.

-    $config = \Drupal::service('config.factory')->getEditable('simple_popup_blocks.popup_'.$uid);
+    $config = $this->configFactory->getEditable('simple_popup_blocks.popup_' . $uid);

In a class that extends ConfigFormBase, the correct method to call is $this->config().

@Dharmendra.s any update on this? Can I make this unassigned?

I am resetting the Assigned field, since the assigned person has not worked on the issue in the past 24 hours.

It looks like changes in # 3291171 📌 Coding standards problems Fixed (commit ed93d92a) was only applied to 8.x-2.x and was not applied to 8.x-3.x.
Do we need to cherry-pick it?

I create MR #15, please make a review.

chaitanyadessai → closed merge request !18

Hi @Nikolay Shapovalov,

Applied MR !15 against version 8.x-3.x-dev, no issues found.

contrib git:(main) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig simple_popup_blocks/
➜  contrib git:(main) ✗

Will change the status to RTBC
Thank you.