[1.x] Log Alert Rules

Thank you for applying!

Before giving links helpful to understand how the review process works, what to expect from a review, and what to do to avoid a review takes more time than needed, I would like to thank all the reviewers for the work they do.
These applications are volunters-driven, which also means it is not possible to predict when an application will be marked fixed and the applicant will get the permission to opt projects into security advisory policy. While we aim to make an application as quick as possible, it is also important for us that more people review the project used for an application. In this way, we make sure applications do not miss some important points that should be instead reported.
Applications are not meant to be complete debugging sessions that eliminate every existing bug, though. I apologize if sometimes applications seem to go into too-detailed reviews.

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 enable GitLab CI for the project and fix the PHP_CodeSniffer errors/warnings it reports.
• 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 will not be changed by this application; once this application is closed, you will be able to change the project status from Not covered to Opt into security advisory coverage. This is possible only 14 days after the project is created.
  
  Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
• Only the person who created the application will get the permission to opt projects into security advisory coverage. No other person will get the same permission from the same application; that applies also to co-maintainers/maintainers of the project used for the application.
• 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 project moderator before posting the first comment on newly created applications. Project moderators 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.

FILE: log_alert_rules.module

For a new module that aims to be compatible with Drupal 10 and Drupal 11, I would rather implement hooks as class methods as described in Support for object oriented hook implementations using autowired services.

/**
 * @file
 * Primary module hooks for Log Alert Rules.
 */

Drupal does not have primary and secondary hooks. Instead of that, it is preferable to use the usual description: “Hook implementations for the [module name] module”, where [module name] is the name of the module given in its .info.yml file.

• 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

main is acceptable as branch name, but it is not yet fully supported on drupal.org.

With contributed projects, it is still not possible to create an issue for the main branch. For projects used in these applications, for which is still suggested to work with release-specific branches, it should be better to avoid using a branch with that name.

src/Controller/AlertRuleListBuilder.php

List builders are not controllers; their namespace should not contain Controller.
Drupal core started to retrieve controllers using PHP attributes; removing from the Controller directory classes that are not used for controllers is necessary to avoid Drupal looks for controllers in classes that are not controllers. See #3311365: Use PHP attributes for route discovery and #3584795: Remove non-controller classes from module Controller namespaces/folders for more information.

src/Controller/AlertRuleToggleController.php

Since that class does not use methods from the parent class, or it uses a single 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.

With Drupal 10.3 and higher versions, a controller class does not need to implement create(); using AutowireTrait, it just needs to implement the class constructor.

  /**
   * Exports all rules or a selected set based on query IDs.
   */

Documentation comments for methods and functions need to document parameters and return value.

src/Entity/AlertRule.php

Projects that are compatible with Drupal 10 or higher versions should use attributes instead of annotations. This means requiring at least Drupal 10.3, but this is not an issue, considering which Drupal core versions are nowadays supported.

    // Check for missing delimiters (most common mistake).
    if (!empty($pattern) && !in_array($pattern[0], ['/', '#', '~', '!', '@', ';', '`', '%'], TRUE)) {
      return 'Patterns must be wrapped in delimiters (e.g., /pattern/ or #pattern#). Did you forget the forward slashes?';
    }
    // Check for Perl-style m;; or m// delimiters.
    if (str_starts_with($pattern, 'm') && strlen($pattern) > 1 && !ctype_alnum($pattern[1])) {
      return 'Perl-style m// delimiters are not supported in PCRE. Use /pattern/ instead.';
    }
    return 'Ensure the pattern uses valid PCRE syntax with delimiters (e.g., /pattern/i). Test at regex101.com using the PCRE2 (PHP) flavor.';

Strings shown in the user interface must be translatable.

src/Form/AlertRuleImportForm.php

Since the class is final, and cannot be extended, its properties can be private instead of protected.

    $yaml = trim((string) $form_state->getValue('import_data'));
    if ($yaml === '') {
      $form_state->setErrorByName('import_data', $this->t('Import YAML is required.'));
      return;
    }

The error message should make clear that the entered string contained just spaces, which is not the expected value.

      $this->messenger()->addStatus($this->t('Created @count alert rule(s): @rules', [
        '@count' => count($results['created']),
        '@rules' => implode(', ', $results['created']),
      ]));

For translatable strings that change basing on a number (as in this case), there is ::formatPlural().

LICENSE.txt

The content of the LICENSE.txt file is expected to be the same of the LICENSE.txt file used by Drupal core.

composer.json

The Drupal.org Composer Façade automatically adds the core requirements for the project basing on what entered in the .info.yml file. As noted in Add a composer.json file:

• The core requirements in the composer.json file are not generated by the façade for Git based checkouts that do not use the façade. For those cases, an entry for drupal/core may be required in the composer.json file.
• When the composer.json file contains the core requirements, those must be compatible with the same version set in the .info.yml file. A mismatch in compatibility may create issues.

This point is not suggesting a change is necessary; its purpose is to make sure you know what the Drupal.org Composer façade does, and that, when adding core requirements also in the composer.json file, it is necessary to double check they are compatible with the core requirements in the .info.yml file.

I cannot think of any workflow where Composer is not used to gather the projects necessary for a site, and their dependencies, considering that Composer is required by Drupal core to gather its dependencies, but that does not mean that having the core requirements also in the composer.json file is not necessary in specific cases.

log_alert_rules.module

A new module that aims to be compatible with latest Drupal releases is expected to implement hooks as class methods as described in Support for object oriented hook implementations using autowired services.
It requires increasing the minimum required Drupal version, but that is not an issue, since not all the Drupal releases are supported.

log_alert_rules.services.yml

Services can be autowired starting with Drupal 9.3.x. This means that it is no longer necessary to give a list of service arguments in the .services.yml file; they will be retrieved from the constructor definition.