[1.0.x] Inactive User Management

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 → .

1. FILE: src/Form/InactiveUserNotificationSettingsForm.php

  /**
   * {@inheritdoc}
   */
  public function __construct(

FILE: src/Notification/InactiveUserNotification.php

  /**
   * The InactiveUserNotification constructor.
   */
  public function __construct(

FILE: src/Settings/GetNotificationSettings.php

  /**
   * The constructor.
   */
  public function __construct(StateInterface $state) {

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be "Constructs a new [class name] object", where [class name] includes the class namespace.

2. FILE: inactive_user_management.module

/**
 * @file
 * Inactive user management module functions.
 */

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.

Thank you for reviewing. I corrected the above mentioned points.

Rest looks fine to me.

Let’s wait for a Code Review Administrator to take a look and if everything goes fine, you will get the role.

Changing issue priority as described in: https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or-distribution-project/security-coverage/reviewing/issue-priorities →

Hi @nadja_stu,

I think there is security risk implication (DoS Risk) if the module processes all users at once, it could overload the server on sites with thousands of users, leading to a self-inflicted denial-of-service.
This risk can be mitigated by implementing a queue worker or batch process on the sendNotificationToInactiveUsers method.

Hi @alvarodemendoza,

Thanks for reviewing. I am not sure I understand you correctly. Would you use a queue worker/batch process to load all the user? Or would you use it for sending the mail? Or maybe for both those actions?

Thank you very much

Hi @nadja_stu,

I would use it for sending the emails. Queue worker would be my choice as many hosting providers limit the cron max execution time. The batch will mitigate the risk too but may not complete on some hosting providers.

You are more than welcome. Have a wonderful day,

That service is called from a hook_cron() implementation. system.cron, the route which executes cron tasks is already protected from unauthorized accesses, since it requires a value somebody who does not administer the site can know. Furthermore, it is not possible to invoke cron tasks when they have been already invoked, and they are not done. (See Cron::run().)

That said, using a queue for handling all the inactive user accounts is better: hook_cron() implementations do not have illimited time, and the time they have must be used from all the hook implementations.

In Drupal 11, hook implementations have been replaced by classes. See Support for object oriented hook implementations using autowired services → .

Hi,
Thank you both for claryifing.

I added a queue worker to my project to send the emails. I also implemented the cron hook as a class (including backwards-compatibility).

Hi @nadja_stu

1. FILE = src/Hook/InactiveUserManagementHooks.php

public function cron(): void {
    /** @var \Drupal\Core\State\StateInterface $drupalState */
    $state = \Drupal::service('state');
    $emailState = $state->get(NotificationSettings::NOTIFICATION_SETTINGS_PREFIX . '_enabled');
    if ($emailState) {
      /** @var \Drupal\inactive_user_management\Notification\InactiveUserNotification $userNotification */
      $userNotification = \Drupal::service('inactive_user_notification');
      $userNotification->sendNotificationToInactiveUsers();
    }
    // Manually process the queue.
    /** @var \Drupal\Core\Queue\QueueFactory $queueFactory */
    $queueFactory = \Drupal::service('queue');
    $queue = $queueFactory->get('inactive_user_notification');
    $queueWorkerManager = \Drupal::service('plugin.manager.queue_worker');
    $queueWorker = $queueWorkerManager->createInstance('inactive_user_notification');
    while ($item = $queue->claimItem()) {
      $queueWorker->processItem($item->data);
      $queue->deleteItem($item);
    }
  }

You should use dependency injection instead of \Drupal calls in classes, implementing ContainerInjectionInterface.

According to this issue: https://www.drupal.org/project/drupal/issues/3493453 📌 [meta] Clean up hook classes in core Active , there is not yet a proper practice for dependency injection in hook classes. Therefore I checked how they did it in Core. They didn't use any dependency injection there. So I figured I wait until I can do it correct rather than just do it.
What are your thoughts on this?

Yes, @nadja_stu you are right, although it is not mandatory, I think it is a good practice, to access any of the services provided by Drupal via the service container to ensure the decoupled nature of these systems is respected.
Services and dependency injection in Drupal →

Actually, a class that implements hooks does not need to use \Drupal for its dependencies. As described in Support for object oriented hook implementations using autowired services → , those hook classes are handled as autowired services, without any need to define them as services.

Looking for how to write a similar class for a contributed module I maintain, I found NodeHooks, which is correctly implemented as autowired service.

That said, hooks are implemented as classes starting from Drupal 11.1. For previous releases, including Drupal 11.0, the code to use is the one reported on Backwards-compatible Hook implementation for Drupal versions from 10.1 to 11.0 → .

Hi @avpaderno,
Please correct me if i got it wrong.
So you are saying that there is no need to define it in the services.yml file, because it is autowired, correct?
But it is required if I want to provide backwards-compatible hook implementations, isn't it?

Currently I implemented it as described in Backwards-compatible Hook implementation for Drupal versions from 10.1 to 11.0 →

So what excatly would you change?
Should I split it in two branches, so I have two versions one for Drupal 11.0.x and one for Drupal 11.x? Similar to what I saw in the NodeHooks example?
Like you said one with the backwards-compatible version for 11.0.x and one with a correctly implemented autowire service for 11.x?

Sorry for my confussion, but I really want to learn how to do it the right way. Thank you so much!

Since the module is defined as compatible only with Drupal 11, I would rather define it as compatible with Drupal 11.1 and higher releases. In that way, you just need to write a class which uses code similar to the code NodeHooks uses.

In that way, it is sufficient to change the core_version_requirement: ^11 line to core_version_requirement: ^11.1, remove all the calls to \Drupal methods, and define the dependencies as constructor's parameters.

Thank you, I corrected the Hook and the version requirement according your suggestions.

I will review the project tomorrow.