Updated: Comment #N

Problem/Motivation

RequestCloseSubscriber::onTerminate calls the global system_run_automated_cron function. This may backfire on lean requests, i.e whenever drupal is not bootstrapped fully.

Proposed resolution

Turn system_run_automated_cron into its own proper subscriber for the KernelEvents::TERMINATE event.

Remaining tasks

Review

User interface changes

None

API changes

None

CommentFileSizeAuthor
#5 2220687-convert-system_run_automated_cron-3.patch4.8 KBznerol
#1 2220687-convert-system_run_automated_cron.patch4.76 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Active » Needs review
FileSize
2220687-convert-system_run_automated_cron.patch4.76 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2220687-convert-system_run_automated_cron.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/modules/system/lib/Drupal/system/AutomaticCronSubscriber.php
@@ -0,0 +1,89 @@
+ * Contains Drupal\system\AutomaticCronSubscriber.

    First note: Is there a reason this is not part of core but part of system module? ... It would be also nice if it would be part of a "EventSubscriber" namespace, just for consistency. 

  2. +++ b/core/modules/system/lib/Drupal/system/AutomaticCronSubscriber.php
@@ -0,0 +1,89 @@
+      if (!isset($cron_last) || (REQUEST_TIME - $cron_last > $threshold)) {

    let's add another pair of braces around REQUEST_TIME - $cron_list, to be sure it works as we want.

znerol’s picture

znerol CreditAttribution: znerol commented

Thanks for the review.

Is there a reason this is not part of core but part of system module?

I feel that it should be part of the module because it accesses configuration defined by the module (system.cron / threshold.autorun).

It would be also nice if it would be part of a "EventSubscriber" namespace, just for consistency.

Yes indeed. I saw Drupal\system\SystemConfigSubscriber and was mislead into doing it like that one. That is probably not the best example.

let's add another pair of braces around REQUEST_TIME - $cron_list

Let's just replace those convoluted conditions with something which is more readable.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2220687-convert-system_run_automated_cron-3.patch4.8 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Awesome!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

I still think we should do #1599622: Run poormanscron via a non-blocking HTTP request but this is good clean-up independent of that issue.

Committed/pushed to 8.x, thanks!

  • Commit 906f131 on 8.x by catch: 
    Issue #2220687 by znerol: Convert system_run_automated_cron into a...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.