Running phpcs --standard=Drupal for this project reports the following issues.

src/Form/MailSettingsForm.php
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  8 | WARNING | [x] Unused use statement
 10 | WARNING | [x] Unused use statement

src/Plugin/Mail/GraphMail.php
FOUND 2 ERRORS AND 16 WARNINGS AFFECTING 18 LINES
   5 | WARNING | [x] Unused use statement
   7 | WARNING | [x] Unused use statement
   8 | WARNING | [x] Unused use statement
  14 | WARNING | [x] Unused use statement
  16 | WARNING | [x] Unused use statement
  19 | WARNING | [x] Unused use statement
  23 | WARNING | [x] Unused use statement
  74 | ERROR   | [x] Missing function doc comment
 150 | ERROR   | [x] Array closing indentation error, expected 10 spaces but found 11
 172 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 176 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 177 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 182 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 183 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 184 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 185 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 186 | WARNING | [x] A comma should follow the last multiline array item. Found: ]
 197 | WARNING | [x] A comma should follow the last multiline array item. Found: )

Comments

apaderno created an issue. See original summary.

kuntal_d’s picture

Status: Active » Needs review
StatusFileSize
new5.6 KB

Injected dependencies in GraphMail plugin.
Please review.

avpaderno’s picture

Issue summary: View changes

I will make this issue broader, since phpcs reports also when a class doesn't inject its dependencies.

avpaderno’s picture

Issue summary: View changes
avpaderno’s picture

Status: Needs review » Needs work
+  /**
+   *
+   */
   public static function create(ContainerInterface $container) {

A documentation comment for a method defined in an interface should not be empty.

+  /**
    * Creates a new \Drupal\graph_mail\Plugin\Mail\GraphMail object.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, LoggerChannelFactoryInterface $logger) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $configFactory, LoggerChannelFactoryInterface $logger) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->logger = $logger;
+    $this->config = $configFactory;
+
   }

When adding new parameter to a method, those parameters need to be added at the end. Also, the code should be backward compatible.

-      $guzzle = new \GuzzleHttp\Client();
+      $guzzle = new Client();

That class is used to implement a service.

chandreshgiri gauswami’s picture

Assigned: Unassigned » chandreshgiri gauswami

I will work on this issue.

avpaderno’s picture

Title: Inject all the dependencies in the create() method » Fix all the issues reported by phpcs
chandreshgiri gauswami’s picture

Assigned: chandreshgiri gauswami » Unassigned
Status: Needs work » Needs review
StatusFileSize
new8.7 KB

Providing patch with fix.

kuntal_d’s picture

StatusFileSize
new112.28 KB

Applied the patch #8. It applied cleanly and resolved all coding standard issues. Attaching screenshot for the reference.

avpaderno’s picture

Status: Needs review » Needs work

Thank you for working on this!

+  /**
+   * {@inheritdoc}
+   */
   public static function create(ContainerInterface $container) {
     return new static(
       $container->get('config.factory'),
-      $container->get('language_manager')
+      $container->get('language_manager'),
+      $container->get('plugin.manager.mail'),
+      $container->get('logger.factory')
     );
   }

Since the code uses only a single logger channel, there is no need to store the logger factory. Only the logger channel is necessary.

-  public function __construct(ConfigFactoryInterface $config_factory, LanguageManagerInterface $language_manager) {
+  public function __construct(ConfigFactoryInterface $config_factory, LanguageManagerInterface $language_manager, MailManagerInterface $mail_manager, LoggerChannelFactoryInterface $logger) {
     parent::__construct($config_factory);
     $this->languageManager = $language_manager;
+    $this->mailManager = $mail_manager;
+    $this->logger = $logger;
   }

When a dependency is added, the code should be similar to the following one, used by Drupal core in GDToolkit::__construct().

public function __construct(array $configuration, $plugin_id, array $plugin_definition, ImageToolkitOperationManagerInterface $operation_manager, LoggerInterface $logger, ConfigFactoryInterface $config_factory, StreamWrapperManagerInterface $stream_wrapper_manager, FileSystemInterface $file_system = NULL) {
  parent::__construct($configuration, $plugin_id, $plugin_definition, $operation_manager, $logger, $config_factory);
  $this->streamWrapperManager = $stream_wrapper_manager;
  if (!$file_system) {
    @trigger_error('The file_system service must be passed to GDToolkit::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/3006851.', E_USER_DEPRECATED);
    $file_system = \Drupal::service('file_system');
  }
  $this->fileSystem = $file_system;
}

(I am referring to what that code does with $file_system, which is a parameter with a default value.)

-  public function __construct(array $configuration, $plugin_id, $plugin_definition, LoggerChannelFactoryInterface $logger) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, LoggerChannelFactoryInterface $logger, ConfigFactoryInterface $config_factory) {
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
     $this->logger = $logger;
+    $this->config = $config_factory;
+
   }

Since StringTranslationTrait is used, the constructor needs to also call setStringTranslation() passing the translation service, which will then need to be one of the constructor's parameters.
There should not be any empty line before the closing curly parenthesis.

-      $message = t('Something went wrong: @error', ['@error' => $e->getMessage()]);
-      \Drupal::logger('graph_mail')->log($message, 'error');
+      $message = $this->t('Something went wrong: @error', ['@error' => $e->getMessage()]);
+      $this->logger->get('graph_mail')->log($message, 'error');

The first argument of log() must be a literal string, not the output of $this->t() or t(). I would also use code similar to the code shown for watchdog_exception(). (I won't call watchdog_exception(), as that is going to be deprecated.)

avpaderno’s picture

chandreshgiri gauswami’s picture

Assigned: Unassigned » chandreshgiri gauswami

I will work on it.

chandreshgiri gauswami’s picture

Assigned: chandreshgiri gauswami » Unassigned
Status: Needs work » Needs review
StatusFileSize
new9 KB

Attaching new patch with specified changes.

Only below point is not done. I saw multiple references using the same method which I have implemented. Someone else may also provide new patch with this point.

-      $message = t('Something went wrong: @error', ['@error' => $e->getMessage()]);
-      \Drupal::logger('graph_mail')->log($message, 'error');
+      $message = $this->t('Something went wrong: @error', ['@error' => $e->getMessage()]);
+      $this->logger->get('graph_mail')->log($message, 'error');
avpaderno’s picture

Assigned: Unassigned » avpaderno
avpaderno’s picture

Status: Needs review » Postponed

Before fixing the issues reported by phpcs, I will implement some other changes that will change or reduce the required dependencies.

avpaderno’s picture

Issue summary: View changes

  • apaderno committed 0e817eb on 1.0.x
    Issue #3320937 by Chandreshgiri Gauswami, apaderno: Fix all the issues...
avpaderno’s picture

Assigned: avpaderno » Unassigned
Status: Postponed » Fixed

Status: Fixed » Closed (fixed)

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