[1.0.x] Advanced Mautic Integration

Thank you for applying!

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 smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

@gpietrzak Thank you for contributing!

Strengths

1. The project has a well defined mission, a good intro page, and the module is mature, in the sense that it includes a correct composer.json file, a good MODULE_info.yaml file, and the code layout matches what is expected of a Drupal module.

2. Correct use of t() functions.

3. Demonstrates awareness of how to use routes rather than hard-coding paths throughout code:
Url::fromRoute('advanced_mautic_integration.admin_settings_form') Has correctly implemented a module links.yml file and routing.yml file.

4. Commenting: Fulfills baseline commenting requirements both in .module and classes, and adding thoughtful and simple explanations where appropriate without adding clutter.

5. Services. Code demonstrates correct use of DI for loading services in classes, and \Drupal::service() within .module code.

  advanced_mautic_integration.visibility:
    class: Drupal\advanced_mautic_integration\VisibilityTracker
    arguments: [ '@config.factory', '@plugin.manager.condition', '@context.handler', '@context.repository' ]

6. Awareness of when adding cache tags is necessary.

  $cache->addCacheableDependency($bubbleable_metadata);
  $cache->applyTo($page);

7. Attention to detail: Module even implements hook_help(). Also writes Interface classes for his custom services rather than just a service with no Interface defined for it. A+

8. Demonstrates understanding of how to set up libraries. I would encourage adding a `version: 1.x` key to your library yaml in order to help aggregation figure out when cache needs to be changed.

tracking_events:
  version: 1.x
  js:
    js/tracking_events.js: {}
  dependencies:
    - core/drupal
    - core/once

Improvements

1. In MauticScript::getScript() I would encourage re-thinking the generation of the script in PHP. Instead I might suggest adding the dynamic part of the script to Drupal settings, and adding the static part of the script to your library yaml.

2. Since your module has configuration, I would encourage the use of a `config/schema/advanced_mautic_integration.schema.yml` file, and a `config/install/advanced_mautic_integration.settings.yml` file. This way, when the module is enabled, default values and data types for all the expected config can be counted on to exist, and you don't need to validate every time you call `$config->get()`. For example: 'trackPageviews' => $config->get('track.pageviews') ?? FALSE, just becomes 'trackPageviews' => $config->get('track.pageviews'); because you have a schema with data types for all your configuration variables, and you have already defined default values in your install :)

3. Wondering if some of your `private` vars should be `protected`

  public function __construct(
    private readonly ConfigFactoryInterface $configFactory,
    private readonly MauticApiWrapperInterface $apiWrapper,
    private readonly AccountProxyInterface $currentUser,
    private readonly RequestStack $requestStack,
  ) {}

or is the expectation that UserSynchronizer should be extensible and these should be visible to any child classes? I don't know what the plans are but I would tend to default to the most restrictive unless you need something more open. Same thing for the other classes where private is used, such as the VisibilityTracker class.

4. On your project page, you say

Unfortunately, the latest 3.1.0 release of the library has problems with Psr/Log. Therefore, I had to use the main branch. I will change it back when a new version of the library is released.

Would you be able to work around this in your wrapper class by decorating whatever method in there has the issue with Psr/Log? Do you have to use the raw Mautic API library?

5. Code style fixes: Using branch 1.0.x I see a few code style issues in the javascript library. You may also want to run this by eslint.

$ phpcs --standard=Drupal,DrupalPractice advanced_mautic_integration

FILE: ...html/web/modules/contrib/advanced_mautic_integration/js/tracking_events.js
--------------------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 48 | ERROR | [x] Expected 1 space before "?"; 0 found
 48 | ERROR | [x] Expected 1 space after "?"; 0 found
 48 | ERROR | [x] Expected 1 space before ":"; 0 found
 48 | ERROR | [x] Expected 1 space after ":"; 0 found
 56 | ERROR | [x] Expected 1 space before "?"; 0 found
 56 | ERROR | [x] Expected 1 space after "?"; 0 found
 56 | ERROR | [x] Expected 1 space before "?"; 0 found
 56 | ERROR | [x] Expected 1 space after "?"; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Time: 148ms; Memory: 6MB

So congratulations on the short list of code style issues flagged! Nicely done. This module is impressively implemented. And thank you for showing me this:

$visibility_collection = new ConditionPluginCollection($this->conditionManager, $visibility_config);
I was not aware you could do that.

I am changing priority as per Issue priorities → .

Thanks for the great feedback! I've added all the remaining issues to the module backlog for easier tracking.