[2.0.x] Email Octopus

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

8.x-1.2 is a wrong branch name, but a correct tag name. master isn't a branch name used in Drupal.org repositories.

Those branches need to be removed.

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

        if (empty($json['data'])) {
          $message = "No data from API";
          $this->loggerFactory->get('Email Octopus')->warning($message);
          return [];
        }

The $message parameter passed to the LoggerInterface methods must be a literal string, not a variable.

      catch (\Exception $e) {
        $this->loggerFactory->get('Email Octopus')->warning($e);
        throw $e;
      }

For exceptions, the function used to log them should be watchdog_exception(); alternatively, code similar to that should be used.

/**
 * Class Octopus Subscribe Form.
 *
 * @package email_octopus
 */
class OctopusSubscribeForm extends FormBase implements ContainerInjectionInterface {

The FormBase class already implements that interface.

  /**
   * The logger channel factory service.
   *
   * @var \Drupal\Core\Logger\LoggerChannelFactoryInterface
   */
  protected $loggerFactory;

  /**
   * The messenger.
   *
   * @var \Drupal\Core\Messenger\MessengerInterface
   */
  protected $messenger;

The base class uses traits to handle the messenger service and the logger service. There is no need to use a property for them; it's sufficient to call setMessenger() and setLoggerFactory() and use the FormBase methods to get those services back.

    $form['content'] = [
      '#markup' => '<div class="newsletter-sub"><h3>' . $title . '</h3><p>' . $body . '</p><span class="button subscribe-button">Subscribe</span></div>',
    ];

Strings shown in the user interface needs to be translated.

    if ($form_state->getValue('email') == NULL) {
      $form_state->setErrorByName('email', $this->t("<div class=erros>Please enter email</div>"));
    }

Drupal core already uses a CSS class for errors; it should not be the validation handler to add them.

function email_octopus_uninstall() {
  \Drupal::configFactory()->getEditable('octopus.adminsettings')->clear('api_key')->save();
}

Deleting the configuration values used by modules is already done from Drupal core when modules are uninstalled. There is no need to do that.

The class constructor calls setMessenger() and setLoggerFactory() passing the services it gets as arguments. When a class method needs those services, it calls messenger() and getLogger(), which are methods added to the FormBase class from traits it uses.

'#markup' => '<div class="newsletter-sub"><h3>' . $this->t($title) . '</h3><p>' . $this->t($body) . '</p><span class="button subscribe-button">Subscribe</span></div>'

The first argument of $this->t() must be a literal string. It cannot be a variable, and it's a possible security issue, to pass values entered by users directly to $this->t(). Those "dynamic values" can be placed in a translatable string with placeholders, but in this case the full markup must be passed as first argument to $this->t(). The example code shown in FormattableMarkup::placeholderFormat() makes clearer what I mean.

The commits have been done in the wrong branch.
The correct branch is 2.0.x; 2.0.x-dev-3315315 is the wrong branch and it uses a wrong branch name, like 2.0.x-dev. Both those branches needs to be removed.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, 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.