[1.0.x] Cron Fail Alert

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

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.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

1. Fix phpcs issue.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml cron_fail_alert/

FILE: cron_fail_alert/src/EventSubscriber/CronFailAlertSubscriber.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 148 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

2. FILE: cron_fail_alert.routing.yml

_permission: 'administer cron_fail_alert configuration'

'administer cron_fail_alert configuration' permission is not defined in the module.

Rest looks fine to me.

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

cron_fail_alert_mail Use of \Drupal::translation()->translate()

When injection is not possible, use Drupal\Core\StringTranslation\TranslatableMarkup to create a new object.
Only calls to t(), $this->t() and new TranslatableMarkup() are picked up by the d.o strings translation matching.

CronFailAlertSubscriber::cronStatusCheck() Untranslated string and setting mixup

This string uses concatenation instead of translation, where "on Site:" is missed.
$subject = $this->cronFailAlertSettings->get('subject') . ' on Site: ' . $this->systemSite->get('name');

Non-blocking comments

CronFailAlertSubscriber::onKernelResponse() logic error

The value of cron_fail_alert_last_check_timestamp is never updated if cron has never failed, from cronStatusCheck() doing "Return FALSE if cron has not failed". This means that if everything is working fine, the timestamp is not stepped, and every page load will be checking to see if cron has failed, which does not seem to be the point of the code to avoid running the full check on every request.
The same outcome happens if the site fails to send the email - when this happens, the full check and attempt to send the email will happen on every page request.
I would imagine both of these has the potential to slow down a site significantly.

Use of t() is implicitly deprecated, but not labelled as such. D10 t() docs states When possible, use the \Drupal\Core\StringTranslation\StringTranslationTrait $this->t(). Otherwise create a new \Drupal\Core\StringTranslation\TranslatableMarkup object directly.. Use of t() is still supported, but is no longer recommended since D8. Not a blocking issue now.

The issue with setting the timestamp only when TRUE is returned from checkCronStatus() still exists, however this is more debugging than reviewing and I shouldn't really be doing it. This is it. It appears there is actually a new bug in this too.
The value of cron_fail_alert.last_check_timestamp should be set regardless of the return value as the full expensive check has been done and does not wish to run the check again for at least $frequency. By not setting it, every page request will check to see if cron has failed until the time that cron does in fact fail AND the mail succeeds - neither of these outcomes is guaranteed. Only then will timestamp be updated and it will have a break for 15 minutes (by default).
eg. Input initial values into the code

• On every request .. event listener of KernelEvents::RESPONSE
• $frequency = 15 (default)
• $lastCheckTimestamp = 0 (module first installed, and/or cron never failed)
• $minutesSinceLastCheck = (1692199541 - 0) / 60 = 28203325
• ($minutesSinceLastCheck < $frequency) = (28203325 < 15) == FALSE
• $lastCron = 1692199500
• $minutesAgo = (1692199541 - 1692199500) / 60 = 0
• $tolerance = 20 (default)
• • This next line is a bug due to missing parenthesis to dictate order of operations(!$minutesAgo >= $tolerance) will always return false because !$minutesAgo == 0, eg (0 >= 20)
  • It should read (!($minutesAgo >= $tolerance)), or since double negatives are hard to read, ($minutesAgo < $tolerance)
  • As it stands, every page request will generate a cron failed email
  • Continuing on, assuming this is fixed
• (!($minutesAgo >= $tolerance)) = (!(0 >= 20)) == TRUE, or better ($minutesAgo < $tolerance)
• return FALSE
• cron_fail_alert.last_check_timestamp NOT set
• Next page request does the exact same thing

Try it with cron_fail_alert.last_check_timestamp is set to 1692195900, it will do exactly the same thing.
It may be detrimental to a site, this is not a blocking issue.

For the subject on the email, I'd argue that it should be using TranslatableMarkup instead of FormattableMarkup - the comment you linked here, plus the 2 lines before it, state that because there is translation needed in the string being built, it should use TranslatableMarkup. The string is not just being concatenated, it contains english text in need of translation.

As it stands the text "on Site" would not be translated.

The only item pushing this to "Needs Work" is that string not being translated. Otherwise, despite the other notes, you're RTBC.

I am removing the PAreview: review bonus tag, as per Review bonus, since a review has been already done.

I am changing priority as per Issue priorities.

This thread has been idle, in the needs work state with no activity for several months. Therefore, I am assuming that you are no longer pursuing this application, and I marked it as Closed (won't fix).

If this is incorrect, and you are still pursuing this application, then 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.

The priority cannot be changed back to Normal yet. See Issue priorities.

Hello @hemangi.gokhale,

Here's a quick feedback:

- Update PHP Annotations to PHP Attributes where applicable (including tests)
- The module is missing the composer.json file. See Add a composer.json file for more info.
- The schema defines the field "to", but it's missing in the default module configuration object
- New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use property promotion

Example:

public function __construct(
   protected EntityTypeManagerInterface $entityTypeManager,
   protected EntityRepositoryInterface $entityRepository,
   protected DateFormatterInterface $dateFormatter,
   TranslationInterface $translation,
 ) {
   $this->setStringTranslation($translation);
 }

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

1. FILE: cron_fail_alert.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.
It would require increasing the minimum Drupal 10 version supported, but Drupal 10.1 is no longer supported.

/**
 * @file
 * Primary module hooks for Cron fail alert module.
 */

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.

2. FILE: src/EventSubscriber/CronFailAlertSubscriber.php

  /**
   * The cron fail alert settings object.
   *
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  protected ImmutableConfig $cronFailAlertSettings;

  /**
   * The site config object.
   *
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  protected ImmutableConfig $systemSite;

  /**
   * The language manager.
   *
   * @var \Drupal\Core\Language\LanguageManagerInterface
   */
  protected LanguageManagerInterface $languageManager;

  /**
   * The logger factory.
   *
   * @var \Drupal\Core\Logger\LoggerChannelFactoryInterface
   */
  protected LoggerChannelFactoryInterface $loggerFactory;

  /**
   * The mail manager service.
   *
   * @var \Drupal\Core\Mail\MailManagerInterface
   */
  protected MailManagerInterface $mailManager;

  /**
   * The state key/value store.
   *
   * @var \Drupal\Core\State\StateInterface
   */
  protected StateInterface $state;

  /**
   * The request stack.
   *
   * @var \Symfony\Component\HttpFoundation\RequestStack
   */
  protected RequestStack $requestStack;

  /**
   * Constructs event subscriber.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory.
   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   *   The language manager.
   * @param \Drupal\Core\Logger\LoggerChannelFactoryInterface $logger_factory
   *   The logger factory.
   * @param \Drupal\Core\Mail\MailManagerInterface $mail_manager
   *   The mail manager service.
   * @param \Drupal\Core\State\StateInterface $state
   *   The state key/value store.
   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   *   The request stack.
   */
  public function __construct(ConfigFactoryInterface $config_factory, LanguageManagerInterface $language_manager, LoggerChannelFactoryInterface $logger_factory, MailManagerInterface $mail_manager, StateInterface $state, RequestStack $request_stack) {
    $this->cronFailAlertSettings = $config_factory->get('cron_fail_alert.settings');
    $this->systemSite = $config_factory->get('system.site');
    $this->languageManager = $language_manager;
    $this->loggerFactory = $logger_factory;
    $this->mailManager = $mail_manager;
    $this->state = $state;
    $this->requestStack = $request_stack;
  }

New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use constructor property promotion.

      $errorMessage = $this->t('Failed to send email notification to @email.', ['@email' => $to]);
      $this->loggerFactory->get('cron_fail_alert')->error($errorMessage);

    $successMessage = $this->t('Email notification sent successfully to @email', ['@email' => $to]);
    $this->loggerFactory->get('cron_fail_alert')->notice($successMessage);

The $message parameter passed to the LoggerInterface methods must be a literal string that uses placeholders. It's not a translatable string returned from t()/$this->t(), a string concatenation, a value returned from a function/method, nor a variable containing an exception object.

3. FILE: src/Form/SettingsForm.php

With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code.

    $form['image_toolkit'] = [
      '#type' => 'radios',
      '#title' => $this->t('Select an image processing toolkit'),
      '#config_target' => 'system.image:toolkit',
      '#options' => [],
    ];

Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.
For this change, it is necessary to require at least Drupal 10.3, but that is not an issue, since Drupal 10.2.x is no longer supported.

  /**
   * The email validator.
   *
   * @var \Drupal\Component\Utility\EmailValidatorInterface
   */
  protected EmailValidatorInterface $emailValidator;

  /**
   * Cron fail alert SettingsForm constructor.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory
   *   The factory for configuration objects.
   * @param \Drupal\Core\Config\TypedConfigManagerInterface $typedConfigManager
   *   The typed config manager.
   * @param \Drupal\Component\Utility\EmailValidatorInterface $email_validator
   *   The email validator service.
   */
  public function __construct(ConfigFactoryInterface $configFactory, TypedConfigManagerInterface $typedConfigManager, EmailValidatorInterface $email_validator) {
    parent::__construct($configFactory, $typedConfigManager);
    $this->emailValidator = $email_validator;
  }

New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use constructor property promotion.

  public function __construct(ConfigFactoryInterface $configFactory, TypedConfigManagerInterface $typedConfigManager, EmailValidatorInterface $email_validator) {
    parent::__construct($configFactory, $typedConfigManager);

Given the changes in ConfigFormBase, the module cannot be compatible with Drupal releases before 10.2.0.

Remember to change status, when the project is ready to be reviewed. In this queue, projects are only reviewed when the status is Needs review.

I am changing priority as per Issue priorities.

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
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 also all the reviewers for helping with these applications.