[D8] SendInBlue module

Review of branch 8.x-1.2 (Got report from codesniffer)

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/sendinblue.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: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/sendinblue.module
------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
------------------------------------------------------------------------------------------------
11 | WARNING | Global constants should not be used, move it to a class or interface
12 | WARNING | Global constants should not be used, move it to a class or interface
13 | WARNING | Global constants should not be used, move it to a class or interface
14 | WARNING | Global constants should not be used, move it to a class or interface
83 | WARNING | Messages are user facing text and must run through t() for translation
87 | WARNING | Messages are user facing text and must run through t() for translation
------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/ConfigurationSendinblueForm.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
39 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
42 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
49 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
60 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
89 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
-----------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/LogoutForm.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
38 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
53 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
57 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
61 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/SubscribeForm.php
---------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
---------------------------------------------------------------------------------------------------------
54 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
141 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
146 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
226 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/RegisteringUserForm.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 9 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
48 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
49 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
51 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
52 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
52 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
57 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
59 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
61 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
72 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
89 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/TransactionnalEmailForm.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 15 WARNINGS AFFECTING 14 LINES
------------------------------------------------------------------------------------------------------------------------------------------
36 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
38 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
58 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
66 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
67 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
69 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
70 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
70 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
76 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
77 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
91 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
103 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
110 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
123 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
124 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
------------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Form/SignupForm.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 44 WARNINGS AFFECTING 44 LINES
------------------------------------------------------------------------------------------------------------------------------------------
41 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
47 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
48 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
62 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
68 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
72 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
90 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
93 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
94 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
101 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
102 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
119 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
127 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
133 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
135 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
143 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
164 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
183 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
202 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
204 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
212 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
219 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
231 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
234 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
239 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
242 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
247 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
250 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
259 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
262 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
263 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
277 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
284 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
286 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
287 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
292 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
294 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
295 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
300 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
303 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
308 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
311 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
389 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
391 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
------------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Entity/Controller/SignupListBuilder.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 9 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
41 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
42 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
43 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
59 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
65 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
70 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
73 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
83 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
96 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
96 | WARNING | Unused variable $actions.
-----------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Plugin/Derivative/SendinblueBlock.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------
25 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
-----------------------------------------------------------------------------------------------------------------------------------------

FILE: /Users/baskaran/Sites/devdesktop/drupal-8.8.1/modules/contrib/sendinblue/src/Plugin/Block/SendinblueBlock.php
-------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------
26 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------------

Please solve these errors/warnings also.
------ ----------------------------------------------
Line src/Form/SubscribeForm.php
------ ----------------------------------------------
127 Variable $type might not be defined.
137 Variable $enumerations might not be defined.
242 Variable $templage_id might not be defined.
------ ----------------------------------------------

------ ----------------------------------------------------------------------
Line src/Routing/SubscribeRoutes.php
------ ----------------------------------------------------------------------
19 \Drupal calls should be avoided in classes, use dependency injection
instead
------ ----------------------------------------------------------------------

------ ----------------------------------------------------------------------
Line src/SendinblueMailin.php
------ ----------------------------------------------------------------------
28 \Drupal calls should be avoided in classes, use dependency injection
instead
31 \Drupal calls should be avoided in classes, use dependency injection
instead
52 \Drupal calls should be avoided in classes, use dependency injection
instead
74 \Drupal calls should be avoided in classes, use dependency injection
instead
77 Variable $body might not be defined.
92 \Drupal calls should be avoided in classes, use dependency injection
instead
111 \Drupal calls should be avoided in classes, use dependency injection
instead
114 Variable $body might not be defined.
------ ----------------------------------------------------------------------

------ -----------------------------------------
Line src/SendinblueManager.php
------ -----------------------------------------
355 Variable $subject might not be defined.
363 Variable $subject might not be defined.
369 Variable $subject might not be defined.
------ -----------------------------------------

Fixes the issue in #8

Added Pareview Link in description

Thanks for your contribution. It looks great. Please find my suggestions below.

Coding Standard issues

FILE: /Users/amitsharma/Sites/drupal-9.0.0-beta2/modules/contrib/sendinblue/src/Form/SubscribeForm.php
------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------
 18 | ERROR | Missing member variable doc comment
------------------------------------------------------------------------------------------------------


FILE: /Users/amitsharma/Sites/drupal-9.0.0-beta2/modules/contrib/sendinblue/src/SendinblueMailin.php
----------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------------
 17 | ERROR | Missing member variable doc comment
 18 | ERROR | Missing member variable doc comment
 19 | ERROR | Missing member variable doc comment
 20 | ERROR | Missing member variable doc comment
----------------------------------------------------------------------------------------------------

sendinblue/src/SendinblueManager.php

/**
   * Generate List page when log in.
   *
   * @return string
   *   A html of list page.
   */
  public static function generateListLogin() {
    $access_token = self::updateAccessToken();
    return 'https://my.sendinblue.com/lists/index/access_token/' . ($access_token);
  }

  /**
   * Generate Campaign page when log in.
   *
   * @return string
   *   A html of campaign.
   */
  public static function generateCampaignLogin() {
    $access_token = self::updateAccessToken();
    return 'https://my.sendinblue.com/camp/listing/access_token/' . ($access_token);
  }

  /**
   * Generate Statistic page when log in.
   *
   * @return string
   *   A html of statistic page.
   */
  public static function generateStatisticLogin() {
    $access_token = self::updateAccessToken();
    return 'https://my.sendinblue.com/camp/message/access_token/' . ($access_token);
  }

You should use a constant or configuration for the BASE_URL of the external URL (https://my.sendinblue.com). This should not be hardcoded at multiple places in code.

/**
   * Get subscriber data by email on drupal table.
   *
   * @param string $email
   *   An email address.
   *
   * @return string
   *   A details of subscriber.
   */
  public static function getSubscriberByEmail($email) {
    $record = \Drupal::database()->select('sendinblue_contact', 'c')
      ->fields('c', ['email'])
      ->condition('c.email', $email)
      ->execute()->fetchAssoc();

    return $record;
  }

  /**
   * Add subscriber on drupal table.
   *
   * @param array $data
   *   A data to add in table.
   */
  public static function addSubscriberTable(array $data = []) {
    \Drupal::database()->insert('sendinblue_contact')->fields(
      [
        'email' => $data['email'],
        'info' => $data['info'],
        'code' => $data['code'],
        'is_active' => $data['is_active'],
      ]
    )->execute();
  }

Please use dependency injection in place of static calls to different services like database.

sendinblue/src/Entity/Controller/SignupListBuilder.php

   $list_name = SendinblueManager::getListNameById($settings['subscription']['settings']['list']);
    $list_labels = Link::fromTextAndUrl($list_name, Url::fromUri('https://my.sendinblue.com/users/list/id/?utm_source=drupal_plugin&utm_medium=plugin&utm_campaign=module_link' . $settings['subscription']['settings']['list']));

 

same as previous please create a constant for external URL.

The rest looks good to me.

Review of the 8.x-1.x branch (commit 4a01dba): looks good to me no error found

Thanks for your contribution!

1. sendinblue_page_attachments(): why do you add you admin library to every page? You should only add it on the render array of admin pages?
2. SendinblueManager: do not call mail() directly, use Drupal's configured mail system instead. This is currently a blocker because developers expect that on a dev site no emails go out if they have configured a dev mail system. If you are using mail() directly then you are bypassing that assumption and a developer might send unintended mails from their dev site.
3. SignupAccessControlHandler: a default of allowing access at the end of the checkAccess() method is not a good idea. A default to deny access to avoid any access bypass problems is more secure.
4. class Signup: the doc blocks seems copied from an example module? Please replace all wrong example docs with real docs about your entity.
5. ConfigurationSendinblueForm::validateForm(): Du not change configuration in a validation method, that should be done in the submit method.
6. ConfigurationSendinblueForm::validateForm(): do not flush caches in a validation method, should also be done on submit only. drupal_flush_all_caches() s expensive, can you only flush the menu related caches?
7. class RegisteringUserForm: doc block talks about SMTP, but it looks like the class is dealing with user registrations? Please update all your class doc comments.
8. I double checked that the D7 permission related vulnerability is fixed on the 7.x-1.x branch, so we should be good there.

1. The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722
2. class Signup: doc block talks about a contact entity, but this seems to be something else? Please update all your class doc comments.

Otherwise looks good to me.

Thanks for your contribution, Loïc!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.