Issue #3242724 by kunal.sachdev, phenaproxima, tedbow: Ensure cron runs...

Issue #3242724 by kunal.sachdev, phenaproxima, tedbow: Ensure cron runs frequently enough to run updates

automatic_updates.services.yml

@@ -70,3 +70,13 @@ services:
     class: Drupal\automatic_updates\Validator\CoreComposerValidator
     tags:
       - { name: event_subscriber }
+  automatic_updates.cron_frequency_validator:
+    class: Drupal\automatic_updates\Validator\CronFrequencyValidator
+    arguments:
+      - '@config.factory'
+      - '@module_handler'
+      - '@state'
+      - '@datetime.time'
+      - '@string_translation'
+    tags:
+      - { name: event_subscriber }

src/Validator/CronFrequencyValidator.php

+<?php
+
+namespace Drupal\automatic_updates\Validator;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\automatic_updates\Event\ReadinessCheckEvent;
+use Drupal\Component\Datetime\TimeInterface;
+use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\State\StateInterface;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\StringTranslation\TranslationInterface;
+use Drupal\Core\Url;
+use Drupal\package_manager\ValidationResult;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Validates that cron runs frequently enough to perform automatic updates.
+ */
+class CronFrequencyValidator implements EventSubscriberInterface {
+
+  use StringTranslationTrait;
+
+  /**
+   * The error-level interval between cron runs, in seconds.
+   *
+   * If cron runs less frequently than this, an error will be raised during
+   * validation. Defaults to 24 hours.
+   *
+   * @var int
+   */
+  protected const ERROR_INTERVAL = 86400;
+
+  /**
+   * The warning-level interval between cron runs, in seconds.
+   *
+   * If cron runs less frequently than this, a warning will be raised during
+   * validation. Defaults to 3 hours.
+   *
+   * @var int
+   */
+  protected const WARNING_INTERVAL = 10800;
+
+  /**
+   * The cron frequency, in hours, to suggest in errors or warnings.
+   *
+   * @var int
+   */
+  protected const SUGGESTED_INTERVAL = self::WARNING_INTERVAL / 3600;
+
+  /**
+   * The config factory service.
+   *
+   * @var \Drupal\Core\Config\ConfigFactoryInterface
+   */
+  protected $configFactory;
+
+  /**
+   * The module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
+
+  /**
+   * The state service.
+   *
+   * @var \Drupal\Core\State\StateInterface
+   */
+  protected $state;
+
+  /**
+   * The time service.
+   *
+   * @var \Drupal\Component\Datetime\TimeInterface
+   */
+  protected $time;
+
+  /**
+   * CronFrequencyValidator constructor.
+   *
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The config factory service.
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   * @param \Drupal\Core\State\StateInterface $state
+   *   The state service.
+   * @param \Drupal\Component\Datetime\TimeInterface $time
+   *   The time service.
+   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation
+   *   The translation service.
+   */
+  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, StateInterface $state, TimeInterface $time, TranslationInterface $translation) {
+    $this->configFactory = $config_factory;
+    $this->moduleHandler = $module_handler;
+    $this->state = $state;
+    $this->time = $time;
+    $this->setStringTranslation($translation);
+  }
+
+  /**
+   * Validates that cron runs frequently enough to perform automatic updates.
+   *
+   * @param \Drupal\automatic_updates\Event\ReadinessCheckEvent $event
+   *   The event object.
+   */
+  public function checkCronFrequency(ReadinessCheckEvent $event): void {
+    $cron_enabled = $this->configFactory->get('automatic_updates.settings')
+      ->get('cron');
+
+    // If automatic updates are disabled during cron, there's nothing we need
+    // to validate.
+    if ($cron_enabled === CronUpdater::DISABLED) {
+      return;
+    }
+    elseif ($this->moduleHandler->moduleExists('automated_cron')) {
+      $this->validateAutomatedCron($event);
+    }
+    else {
+      $this->validateLastCronRun($event);
+    }
+  }
+
+  /**
+   * Validates the cron frequency according to Automated Cron settings.
+   *
+   * @param \Drupal\automatic_updates\Event\ReadinessCheckEvent $event
+   *   The event object.
+   */
+  protected function validateAutomatedCron(ReadinessCheckEvent $event): void {
+    $message = $this->t('Cron is not set to run frequently enough. <a href=":configure">Configure it</a> to run at least every @frequency hours or disable automated cron and run it via an external scheduling system.', [
+      ':configure' => Url::fromRoute('system.cron_settings')->toString(),
+      '@frequency' => static::SUGGESTED_INTERVAL,
+    ]);
+
+    $interval = $this->configFactory->get('automated_cron.settings')->get('interval');
+
+    if ($interval > static::ERROR_INTERVAL) {
+      $error = ValidationResult::createError([$message]);
+      $event->addValidationResult($error);
+    }
+    elseif ($interval > static::WARNING_INTERVAL) {
+      $warning = ValidationResult::createWarning([$message]);
+      $event->addValidationResult($warning);
+    }
+  }
+
+  /**
+   * Validates the cron frequency according to the last cron run time.
+   *
+   * @param \Drupal\automatic_updates\Event\ReadinessCheckEvent $event
+   *   The event object.
+   */
+  protected function validateLastCronRun(ReadinessCheckEvent $event): void {
+    // Determine when cron last ran. If not known, use the time that Drupal was
+    // installed, defaulting to the beginning of the Unix epoch.
+    $cron_last = $this->state->get('system.cron_last', $this->state->get('install_time', 0));
+
+    // @todo Should we allow a little extra time in case the server job takes
+    //   longer than expected? Otherwise a server setup with a 3-hour cron job
+    //   will always give this warning. Maybe this isn't necessary because the
+    //   last cron run time is recorded after cron runs. Address this in
+    //   https://www.drupal.org/project/automatic_updates/issues/3248544.
+    if ($this->time->getRequestTime() - $cron_last > static::WARNING_INTERVAL) {
+      $error = ValidationResult::createError([
+        $this->t('Cron has not run recently. For more information, see the online handbook entry for <a href=":cron-handbook">configuring cron jobs</a> to run at least every @frequency hours.', [
+          ':cron-handbook' => 'https://www.drupal.org/cron',
+          '@frequency' => static::SUGGESTED_INTERVAL,
+        ]),
+      ]);
+      $event->addValidationResult($error);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    return [
+      ReadinessCheckEvent::class => 'checkCronFrequency',
+    ];
+  }
+
+}

tests/src/Kernel/AutomaticUpdatesKernelTestBase.php

@@ -60,6 +60,11 @@ abstract class AutomaticUpdatesKernelTestBase extends KernelTestBase {
     // update to 9.8.1 is available.
     $this->setCoreVersion('9.8.0');
     $this->setReleaseMetadata(__DIR__ . '/../../fixtures/release-history/drupal.9.8.1.xml');
+
+    // Set a last cron run time so that the cron frequency validator will run
+    // from a sane state.
+    // @see \Drupal\automatic_updates\Validator\CronFrequencyValidator
+    $this->container->get('state')->set('system.cron_last', time());
   }
 
   /**

tests/src/Kernel/ReadinessValidation/CronFrequencyValidatorTest.php

+<?php
+
+namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\automatic_updates\Event\ReadinessCheckEvent;
+use Drupal\automatic_updates\Validator\CronFrequencyValidator;
+use Drupal\package_manager\ValidationResult;
+use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+use PHPUnit\Framework\AssertionFailedError;
+
+/**
+ * @covers \Drupal\automatic_updates\Validator\CronFrequencyValidator
+ *
+ * @group automatic_updates
+ */
+class CronFrequencyValidatorTest extends AutomaticUpdatesKernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = [
+    'automatic_updates',
+    'package_manager',
+    'package_manager_bypass',
+  ];
+
+  /**
+   * Tests that nothing is validated if updates are disabled during cron.
+   */
+  public function testNoValidationIfCronDisabled(): void {
+    $this->config('automatic_updates.settings')
+      ->set('cron', CronUpdater::DISABLED)
+      ->save();
+
+    $validator = new class (
+      $this->container->get('config.factory'),
+      $this->container->get('module_handler'),
+      $this->container->get('state'),
+      $this->container->get('datetime.time'),
+      $this->container->get('string_translation')
+    ) extends CronFrequencyValidator {
+
+      /**
+       * {@inheritdoc}
+       */
+      protected function validateAutomatedCron(ReadinessCheckEvent $event): void {
+        throw new AssertionFailedError(__METHOD__ . '() should not have been called.');
+      }
+
+      /**
+       * {@inheritdoc}
+       */
+      protected function validateLastCronRun(ReadinessCheckEvent $event): void {
+        throw new AssertionFailedError(__METHOD__ . '() should not have been called.');
+      }
+
+    };
+    $this->container->set('automatic_updates.cron_frequency_validator', $validator);
+    $this->assertCheckerResultsFromManager([], TRUE);
+  }
+
+  /**
+   * Data provider for ::testLastCronRunValidation().
+   *
+   * @return array[]
+   *   Sets of arguments to pass to the test method.
+   */
+  public function providerLastCronRunValidation(): array {
+    $error = ValidationResult::createError([
+      'Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a> to run at least every 3 hours.',
+    ]);
+
+    return [
+      'cron never ran' => [
+        0,
+        [$error],
+      ],
+      'cron ran four hours ago' => [
+        time() - 14400,
+        [$error],
+      ],
+      'cron ran an hour ago' => [
+        time() - 3600,
+        [],
+      ],
+    ];
+  }
+
+  /**
+   * Tests validation based on the last cron run time.
+   *
+   * @param int $last_run
+   *   A timestamp of the last time cron ran.
+   * @param \Drupal\package_manager\ValidationResult[] $expected_results
+   *   The expected validation results.
+   *
+   * @dataProvider providerLastCronRunValidation
+   */
+  public function testLastCronRunValidation(int $last_run, array $expected_results): void {
+    $this->container->get('state')->set('system.cron_last', $last_run);
+    $this->assertCheckerResultsFromManager($expected_results, TRUE);
+
+    // After running cron, any errors or warnings should be gone.
+    $this->container->get('cron')->run();
+    $this->assertCheckerResultsFromManager([], TRUE);
+  }
+
+  /**
+   * Data provider for ::testAutomatedCronValidation().
+   *
+   * @return array[]
+   *   Sets of arguments to pass to the test method.
+   */
+  public function providerAutomatedCronValidation(): array {
+    return [
+      'default configuration' => [
+        NULL,
+        [],
+      ],
+      'every 6 hours' => [
+        21600,
+        [
+          ValidationResult::createWarning([
+            'Cron is not set to run frequently enough. <a href="/admin/config/system/cron">Configure it</a> to run at least every 3 hours or disable automated cron and run it via an external scheduling system.',
+          ]),
+        ],
+      ],
+      'every 25 hours' => [
+        90000,
+        [
+          ValidationResult::createError([
+            'Cron is not set to run frequently enough. <a href="/admin/config/system/cron">Configure it</a> to run at least every 3 hours or disable automated cron and run it via an external scheduling system.',
+          ]),
+        ],
+      ],
+    ];
+  }
+
+  /**
+   * Tests validation based on Automated Cron settings.
+   *
+   * @param int|null $interval
+   *   The configured interval for Automated Cron. If NULL, the default value
+   *   will be used.
+   * @param \Drupal\package_manager\ValidationResult[] $expected_results
+   *   The expected validation results.
+   *
+   * @dataProvider providerAutomatedCronValidation
+   */
+  public function testAutomatedCronValidation(?int $interval, array $expected_results): void {
+    $this->enableModules(['automated_cron']);
+    $this->installConfig('automated_cron');
+
+    if (isset($interval)) {
+      $this->config('automated_cron.settings')
+        ->set('interval', $interval)
+        ->save();
+    }
+    $this->assertCheckerResultsFromManager($expected_results, TRUE);
+
+    // Even after running cron, we should have the same results.
+    $this->container->get('cron')->run();
+    $this->assertCheckerResultsFromManager($expected_results);
+  }
+
+}