[D9] Campaign Monitor Webform Handler

This review uses the Project Application Review Template.

Automated Review

Automated review is not working. I ran phpcs manually with both the Drupal and DrupalPractice standards. Neither generated errors

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

The module uses branching at the Major semantic version (ie. 2.x .) The guideline is unclear as to any preferences between a new branch at the major or minor release. The example in the guideline shows a new branch for a minor release (ex. 2.0.x)

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Partially Follows] the guidelines for in-project documentation and/or the README Template.

The format is correct, but the content has not been completed.

Code long/complex enough for review

[Yes: Limited] the guidelines for project length and complexity. Project is detailed but the commits submitted by the applicant are older and limited in number.

Secure code

[Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (+) Update README.md file

2. This review is of the project overall. The intent of the security advisory coverage application though, is to evaluate the individual submitting the request. The submitter's last commit to the project was in July 2015 for a first version at 8.x. Eight additional commits were for 7.x in December of 2014. There was one project page update in 2020. The commits by the submitter look reasonable. Given the age and number of commits I'm not sure if there is enough information for the application based on this project alone. It did notice a significant number of commits to other projects.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

For these applications, we need at least a branch where the code has been committed from the user who applies. In this case, in the 2.x branch there are commits from other users. This means we cannot understand what the user who applies understands about writing secure code that follows the coding standards and correctly uses the Drupal API, since the code has been written from more users.

This application can continue, with a project/branch where most of the commits have been done from the user who applies. (The ideal would be a project where the commits for a branch are done from the user who applies.)

See my previous comment.

• 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

core_version_requirement: ^8.8.2 || ^9 || ^10

The minimum required Drupal 8.x version should be 8.8.3, when using semantic versions. Earlier Drupal versions don't handle semantic versions correctly.

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    $instance = parent::create($container, $configuration, $plugin_id, $plugin_definition);
    $instance->loggerFactory = $container->get('logger.factory')->get('webform_campaign_monitor');
    $instance->campaignMonitorRESTClient = $container->get('campaign_monitor_rest_client');
    return $instance;
  }

The purpose of ContainerInjectionInterface::create() (or similar methods like ContainerFactoryPluginInterface::create()) is passing the injected dependencies to the class constructor, non initializing the instance properties.

      catch (ClientException $exception) {
        \Drupal::messenger()->addMessage("Something went wrong with your subscription." . $exception->getMessage(), "warning");
      }

The first argument passed to MessengerInterface::addError(), MessengerInterface::addMessage(), MessengerInterface::addStatus(), and MessengerInterface::addWarning() must be a translatable string that uses placeholders.

      $this->loggerFactory->info("Subscription have not sent to Campaign Monitor, because of " .
        $this->configuration['trigger'] .
      " value: " . print_r($wf_submission[$this->configuration['trigger']], TRUE));
    }

The $message parameter passed to the LoggerInterface methods must be a literal string and use placeholders.

Services and dependency injection in Drupal 8+ / Injecting dependencies into controllers, forms and blocks says:

Every object which uses Drupal\Core\DependencyInjection\ContainerInjectionInterface (for example, controllers, forms, and blocks), must implement the create() factory method to pass the dependencies into the class constructor.

Passing the dependencies to the constructor makes sense, as initializing the constructed object is the constructor's responsibility. It's what other static methods do too, for example Url::fromEntityUri() or Url::fromRouteUri().

protected static function fromRouteUri(array $uri_parts, array $options, $uri) {
  $route_parts = explode(';', $uri_parts['path'], 2);
  $route_name = $route_parts[0];
  if ($route_name === '') {
    throw new \InvalidArgumentException("The route URI '{$uri}' is invalid. You must have a route name in the URI. e.g., route:system.admin");
  }
  $route_parameters = [];
  if (!empty($route_parts[1])) {
    parse_str($route_parts[1], $route_parameters);
  }
  return new static($route_name, $route_parameters, $options);
}

WebformCampaignMonitorHandler still needs to implements its constructor, which is what WebformHandlerBase::create() calls with new static().

How WebformHandlerBase::create() is written, it doesn't allow to easily extend it. You should extend create() to pass all the dependencies to __construct(), which then initializes all the properties, including the WebformHandlerBase's properties.

@le72 If you made the other changes, feel free to change status. That isn't an issue you can resolve in your module.

The only change you can eventually do is requiring a specific Webform version, if that change is present starting with Webform 6.x. This isn't a change required from these applications, though.

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.

I thank all the reviewers.