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

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

Oops! I didn't notice that 🤦‍♀️. Thanks for letting me know. I've fixed it and made some more improvements. This is now ready for another review now.

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.

Non-blocking comments

@ELC, thanks a lot for your feedback. I've made the changes you suggested, and you can see them in this commit: link to commit.

Here are the updates I've made:

Changes to 'cron_fail_alert_mail' Function
I've used the 't()' function to translate the subject in the 'cron_fail_alert_mail' function, which helps with language translation.

Fix for 'CronFailAlertSubscriber::cronStatusCheck()'
Instead of using string translation, I'm now using FormattableMarkup because my string is combined with other parts. You can find more details here: link to FormattableMarkup.

Additionally, I've done the following improvements:

• Reorganized the logic and structure of the code to make it easier to read.
• Made changes to comments and code to make everything clearer.
• Renamed the cronStatusCheck() method to checkCronStatus() for better understanding.
• Changed the name of the method cronFailAlertSendEmail() to sendCronFailAlertEmail() to make it consistent and clear.
• Improved the naming of variables.

Regarding the following points:

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 line of code found at this link is always set to "false" during its initial execution. This occurs when the "checkCronStatus" process is carried out.
• This specific line ensures that we adhere to the frequency limit set by the administrator, as described in this link.
• If this limit is surpassed, we proceed to send an email and also update the value of the "cron_fail_alert.last_check_timestamp" state.

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.

• After successfully sending the email, the value of cron_fail_alert.last_check_timestamp will get updated.
• The same line of code (link to line) ensures that we only proceed if enough time has passed since the last check.
• As a result, no more emails will be sent until that specific point in time.

Feel free to let me know if you have any further questions or suggestions.

Use of t() is implicitly deprecated, but not labelled as such. D10 t() docs states . 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 run 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)
  • Continuing on, assuming this is fixed
• (!($minutesAgo >= $tolerance)) = (!(0 >= 20)) == TRUE
• 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.