[1.1.x] Mandatly | Cookie Compliance and Consent solution

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, enable GitLab CI for the project, and fix what reported from the phpcs job. This help to fix most of what reviewers would report.
• For the time this application is open, only your commits are allowed. No other people, including other maintainers/co-maintainers can make commits.
• 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.
• Nobody else will get the permission to opt projects into security advisory policy. If there are other maintainers/co-maintainers who will to get that permission, they need to apply with a different module.
• 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. The configuration used for these tools needs to be the same configuration used by GitLab CI, stored in the GitLab Templates repository.
• 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.

I do not have time for a full review, but the following point needs to be fixed.

Projects hosted on drupal.org are licensed under GPLv2+, the same license used from Drupal core. If you are licensing a project under a different license, it cannot he hosted on drupal.org. More details are given in Drupal Git Contributor Agreement & Repository Usage Policy.

All code that is a derivative work of Drupal (typically PHP code, including but not limited to: core patches, modules, themes, etc) committed to Drupal.org's git repository is licensed as GPL version 2.0 and later (official short identifier: “GPL-2.0-or-later”). This means that the code is licensed under GPLv2, and there exists an option that allows downstream recipients to re-license the code to be under a later version of GPL.

For code licensed under GPLv3, see See I want to release my work under GPL version 3 or under GPL version 2-only. Can I do so and host it on Drupal.org?

No. You can release your work under any GPL version 2 or later compatible license. However, you may only check it into Drupal's Git repositories if you are releasing it under the same license as Drupal itself, that is GPL version 2 or later, allowing users to choose between the terms of the GPL version 2 or the terms in any new versions as updated by the FSF. If you are unable or unwilling to do so, do not check it into a Drupal Git repository.

Furthermore, is the account used to create this application a shared account?

It must be GPLV2-or-later. It still is GPLv3.

I also asked if the account used to create this application is a shared account.

The question about the account is still without answer.

This project is licensed under the GNU General Public License v3.0 only

• https://git.drupalcode.org/project/mandatly_cookie_compliance/-/blob/1.1...
• https://git.drupalcode.org/project/mandatly_cookie_compliance/-/blob/1.1...
• https://git.drupalcode.org/project/mandatly_cookie_compliance/-/blob/1.1...

Initially it was an shared account, but not now.

May you put your first name and last name on your account?

Remove also the 1.1.1-rc1 branch, which has the wrong name.

May you put your first name and last name on your account?

You need to put your first and last name on your account.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
• The single review points are not ordered, not even by importance

mandatly_cookie_compliance.module

\Drupal::logger('mandatly_cookie_compliance')->notice('mandatly_cookie_compliance_page_attachments triggered');

Remove any debugging code, such as the lines logging when a hook is invoked.

  switch ($route_name) {
    case 'help.page.mandatly_cookie_compliance':
      $output = '<p>' . t('Visit the project page for more information:') . ' <a href="https://www.drupal.org/project/mandatly_cookie_compliance" target="_blank">https://www.drupal.org/project/mandatly_cookie_compliance</a>.</p>';
      return $output;
  }

Translatable strings do no concatenate strings but use placeholders.

src/Controller/BannerController.php

Since that class does not use any method from the parent class, it does not need to use ControllerBase as parent class. Controllers do not need to have a parent class; as long as they implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface, they are fine.

  /**
   * Mandatly Cookie compliance Settings.
   */
  public function displayBanner() {

That description is not for that method.
The description of the return value is missing.

        catch (RequestException $e) {
          \Drupal::logger('mandatly_cookie_compliance')->error('Error fetching demo banner settings: @message', ['@message' => $e->getMessage()]);
        }

Any dependency must be injected using the dependency injection container, except in the case the dependency is used only from static methods.

    $form = $this->formBuilder->getForm('Drupal\mandatly_cookie_compliance\Form\SettingsForm');
    // $form = $this->formBuilder->getForm('Drupal\mandatly_cookie_compliance\Form\HeaderForm');
    return [
      'form' => $form,
      'banner' => [
        '#markup' => '<div id="cookie-banner-container"></div>',
        '#attached' => [
          'library' => [
            'mandatly_cookie_compliance/cookie_banner',
          ],
        ],
      ],
    ];

There is no need to use a controller to show a form. A form can be associated to a route.

src/Form/SettingsForm.php

      $form['dynamic_section']['demo_text'] = [
        '#type' => 'item',
        '#markup' => '<div class="demo">Demo mode is currently active and displays a dummy banner on your website. To implement actual banner, you can turn off demo mode.</div>',
      ];

Any text shown in the user interface must be translatable.

    $config = $this->configFactory()->getEditable('mandatly_cookie_compliance.settings');
    $show_banner = (int) $form_state->getValue('show_banner');
    $demo_mode = (int) $form_state->getValue('demo_mode');

There is no need to use $this->configFactory() when there is $this->config().

1. FILE: README.md

The README file is missing the required sections - Requirements, and Configuration.

2. FILE: mandatly_cookie_compliance.info.yml

core_version_requirement: ^9 || ^10 || ^11

FILE: composer.json

"drupal/core": "^8 || ^9 || ^10 || ^11"

Inconsistent drupal core dependencies.

3. FILE: mandatly_cookie_compliance.module

/**
 * @file
 * Vaules from remote file.
 */

The usual description for a .module file is “Hook implementations for the [module name] module”, where [module name] is the module name given in the .info.yml file.

/**
 * Function for Preprocessing.
 */
function mandatly_cookie_compliance_preprocess_page(&$variables) {

The correct doc comment is "Implements hook_preprocess_page()".

4. FILE: css/toggle-switch.css

Twig code needs to be correctly indented. Drupal uses two spaces for indentation, not four spaces or tabs.

5. FILE: src/Controller/BannerController.php

// $form = $this->formBuilder->getForm('Drupal\mandatly_cookie_compliance\Form\HeaderForm');

Remove commented code.

FILE: README.md

The README file is still missing the required sections - Requirements, and Configuration. It should follow the content and formatting described in README.md template.

FILE: mandatly_cookie_compliance.module

/**
 * Hook implementations for the mandatly_cookie_compliance module.
 */
function mandatly_cookie_compliance_page_attachments(array &$attachments) {

The documentation comment for hook_page_attachments() is currently missing. It should be added separately and not merged with the module’s general description to maintain clarity and follow Drupal coding standards.

@srtmdev I suggest reading the links in #3

Specifically in the what to expect:

Unfortunately, the application queue does occasionally experience a large backlog, and applications may sit in the queue up to a year before getting reviewed. 

There is no Service Level Agreement(SLA) in this process.

The application will be processed, when is a 'wait and see', it could be today, it could be several months before anyone makes another review. Following the priority steps listed in the links given may reduce the amount of time waiting, although there is no gurantee it will.

Note that the process does not enroll your project in the security advisory opt in process, it tests a developer(yourself) to have the permission to do so. It is not the module being tested here, it is yourself as an individual and if/when process completes you will opt the project in not the reviewers in this queue.

I'm assuming based on your issues that your primarily doing this so that the company module is not subject to D.O. suggesting users publicly disclose security vulnerabilities, if so you are unfortunately caught in a portion of D.O. that is not very corporate or security industry friendly and will just have to 'wait it out'.

I am changing priority as per Issue priorities.

It seems to me that this module contains too few code to be able to properly review.
Though at a first quick look, here are some of the issues that I noticed. These issues do not reflect everything:
There are still phpcs errors (Standard and Practice), with many \Drupal calls instead of dependency injection. There are also some questionable choices like the following lines in the form's submitForm:

\Drupal::service('router.builder')->rebuild();
\Drupal::cache()->invalidateAll();
\Drupal::service('cache_tags.invalidator')->invalidateTags(['config:mandatly_cookie_compliance.settings']);

The module is working with configuration, but there's no schema definition and no default configuration.
A better choice for the module package would be package: User interface.
There's no help page.

I am changing priority as per Issue priorities.

This thread has been idle, in the needs work state with no activity for some months.

May you confirm you are still pursuing this application? If this is the case, and you made commits basing on what previously reported, or you can answer the questions previously asked, please change the status to Needs review.

This thread has been idle, in the Needs work state with no activity for about six months or more; the application has been created about 11 months ago or more. Therefore, I marked it as Closed (won't fix).

If this is incorrect, and you are still pursuing this application, please feel free to re-open it and set the issue status to Needs work or Needs review, depending on the current status of your code.