Issue #3285631 by Theresa.Grannum, phenaproxima, tedbow: Make it impossible to...

Issue #3285631 by Theresa.Grannum, phenaproxima, tedbow: Make it impossible to call CronUpdater::begin() directly

src/CronUpdater.php

@@ -118,14 +118,15 @@ class CronUpdater extends Updater {
   /**
    * {@inheritdoc}
    */
-  public function begin(array $project_versions, ?int $timeout = 300): string {
-    // Prevent mischievous callers from starting an update even if unattended
-    // updates are disabled. To start an update programmatically, use
-    // \Drupal\automatic_updates\Updater::begin().
-    if ($this->getMode() === static::DISABLED) {
-      throw new \LogicException('Unattended updates are disabled.');
-    }
-    return parent::begin($project_versions, $timeout);
+  final public function begin(array $project_versions, ?int $timeout = 300): string {
+    // Unattended updates should never be started using this method. They should
+    // only be done by ::handleCron(), which has a strong opinion about which
+    // release to update to. Throwing an exception here is just to enforce this
+    // boundary. To update to a specific version of core, use
+    // \Drupal\automatic_updates\Updater::begin() (which is called in
+    // ::performUpdate() to start the update to the target version of core
+    // chosen by ::handleCron()).
+    throw new \BadMethodCallException(__METHOD__ . '() cannot be called directly.');
   }
 
   /**
@@ -148,7 +149,8 @@ class CronUpdater extends Updater {
     // handle any exceptions or validation errors consistently, and destroy the
     // stage regardless of whether the update succeeds.
     try {
-      $this->begin(['drupal' => $target_version], $timeout);
+      // @see ::begin()
+      parent::begin(['drupal' => $target_version], $timeout);
       $this->stage();
       $this->apply();
 

tests/src/Kernel/CronUpdaterTest.php

@@ -342,14 +342,10 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
   }
 
   /**
-   * Tests that the cron updater throws an exception if started while disabled.
+   * Tests that CronUpdater::begin() unconditionally throws an exception.
    */
-  public function testExceptionWhenDisabled(): void {
-    $this->config('automatic_updates.settings')
-      ->set('cron', CronUpdater::DISABLED)
-      ->save();
-
-    $this->expectExceptionMessage('Unattended updates are disabled.');
+  public function testBeginThrowsException(): void {
+    $this->expectExceptionMessage(CronUpdater::class . '::begin() cannot be called directly.');
     $this->container->get('automatic_updates.cron_updater')
       ->begin(['drupal' => '9.8.1']);
   }

tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php

@@ -2,9 +2,10 @@
 
 namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
 
-use Drupal\package_manager\Exception\StageValidationException;
-use Drupal\package_manager\ValidationResult;
+use Drupal\Core\Logger\RfcLogLevel;
+use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+use Psr\Log\Test\TestLogger;
 
 /**
  * @covers \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
@@ -25,18 +26,24 @@ class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
    */
   private const SUFFIXES = ['install', 'post_update.php'];
 
+  /**
+   * The test logger channel.
+   *
+   * @var \Psr\Log\Test\TestLogger
+   */
+  private $logger;
+
   /**
    * {@inheritdoc}
    */
   protected function setUp(): void {
     parent::setUp();
-
     $this->createTestProject();
 
-    /** @var \Drupal\Tests\automatic_updates\Kernel\TestCronUpdater $updater */
-    $updater = $this->container->get('automatic_updates.cron_updater');
-    $updater->begin(['drupal' => '9.8.1']);
-    $updater->stage();
+    $this->logger = new TestLogger();
+    $this->container->get('logger.factory')
+      ->get('automatic_updates')
+      ->addLogger($this->logger);
   }
 
   /**
@@ -80,21 +87,27 @@ class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
     $this->assertFalse($this->container->get('module_handler')->moduleExists('views'));
     $this->assertFalse($this->container->get('theme_handler')->themeExists('automatic_updates_theme_with_updates'));
 
-    // Create bogus staged versions of Views' and
-    // Automatic Updates Theme with Updates .install and .post_update.php files.
-    // Since these extensions are not installed, the changes should not raise
-    // any validation errors.
-    $updater = $this->container->get('automatic_updates.cron_updater');
-    $module_list = $this->container->get('extension.list.module')->getList();
-    $theme_list = $this->container->get('extension.list.theme')->getList();
-    $module_dir = $updater->getStageDirectory() . '/' . $module_list['views']->getPath();
-    $theme_dir = $updater->getStageDirectory() . '/' . $theme_list['automatic_updates_theme_with_updates']->getPath();
-    foreach (static::SUFFIXES as $suffix) {
-      file_put_contents("$module_dir/views.$suffix", $this->randomString());
-      file_put_contents("$theme_dir/automatic_updates_theme_with_updates.$suffix", $this->randomString());
-    }
+    $listener = function (PreApplyEvent $event): void {
+      // Create bogus staged versions of Views' and
+      // Automatic Updates Theme with Updates .install and .post_update.php
+      // files. Since these extensions are not installed, the changes should not
+      // raise any validation errors.
+      $dir = $event->getStage()->getStageDirectory();
+      $module_list = $this->container->get('extension.list.module')->getList();
+      $theme_list = $this->container->get('extension.list.theme')->getList();
+      $module_dir = $dir . '/' . $module_list['views']->getPath();
+      $theme_dir = $dir . '/' . $theme_list['automatic_updates_theme_with_updates']->getPath();
+      foreach (static::SUFFIXES as $suffix) {
+        file_put_contents("$module_dir/views.$suffix", $this->randomString());
+        file_put_contents("$theme_dir/automatic_updates_theme_with_updates.$suffix", $this->randomString());
+      }
+    };
+    $this->container->get('event_dispatcher')
+      ->addListener(PreApplyEvent::class, $listener, PHP_INT_MAX);
 
-    $updater->apply();
+    $this->container->get('cron')->run();
+    // There should not have been any errors.
+    $this->assertFalse($this->logger->hasRecords(RfcLogLevel::ERROR));
   }
 
   /**
@@ -124,73 +137,61 @@ class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
    * @dataProvider providerFileChanged
    */
   public function testFileChanged(string $suffix, bool $delete): void {
-    /** @var \Drupal\automatic_updates\CronUpdater $updater */
-    $updater = $this->container->get('automatic_updates.cron_updater');
-    $theme_installer = $this->container->get('theme_installer');
-    $theme_installer->install(['automatic_updates_theme_with_updates']);
-    $theme = $this->container->get('theme_handler')
-      ->getTheme('automatic_updates_theme_with_updates');
-    $module_file = $updater->getStageDirectory() . "/core/modules/system/system.$suffix";
-    $theme_file = $updater->getStageDirectory() . "/{$theme->getPath()}/automatic_updates_theme_with_updates.$suffix";
-    if ($delete) {
-      unlink($module_file);
-      unlink($theme_file);
-    }
-    else {
-      file_put_contents($module_file, $this->randomString());
-      file_put_contents($theme_file, $this->randomString());
-    }
-
-    $expected_results = [
-      ValidationResult::createError(['System', 'Automatic Updates Theme With Updates'], t('The update cannot proceed because possible database updates have been detected in the following extensions.')),
-    ];
+    $listener = function (PreApplyEvent $event) use ($suffix, $delete): void {
+      $dir = $event->getStage()->getStageDirectory();
+      $theme_installer = $this->container->get('theme_installer');
+      $theme_installer->install(['automatic_updates_theme_with_updates']);
+      $theme = $this->container->get('theme_handler')
+        ->getTheme('automatic_updates_theme_with_updates');
+      $module_file = "$dir/core/modules/system/system.$suffix";
+      $theme_file = "$dir/{$theme->getPath()}/{$theme->getName()}.$suffix";
+      if ($delete) {
+        unlink($module_file);
+        unlink($theme_file);
+      }
+      else {
+        file_put_contents($module_file, $this->randomString());
+        file_put_contents($theme_file, $this->randomString());
+      }
+    };
+    $this->container->get('event_dispatcher')
+      ->addListener(PreApplyEvent::class, $listener, PHP_INT_MAX);
 
-    try {
-      $updater->apply();
-      $this->fail('Expected a validation error.');
-    }
-    catch (StageValidationException $e) {
-      $this->assertValidationResultsEqual($expected_results, $e->getResults());
-    }
+    $this->container->get('cron')->run();
+    $this->assertTrue($this->logger->hasRecordThatContains("The update cannot proceed because possible database updates have been detected in the following extensions.\nSystem\nAutomatic Updates Theme With Updates", RfcLogLevel::ERROR));
   }
 
   /**
    * Tests that an error is raised if install or post-update files are added.
    */
   public function testUpdatesAddedInStage(): void {
-    $module = $this->container->get('module_handler')
-      ->getModule('package_manager_bypass');
-    $theme_installer = $this->container->get('theme_installer');
-    $theme_installer->install(['automatic_updates_theme']);
-    $theme = $this->container->get('theme_handler')
-      ->getTheme('automatic_updates_theme');
+    $listener = function (PreApplyEvent $event): void {
+      $module = $this->container->get('module_handler')
+        ->getModule('package_manager_bypass');
+      $theme_installer = $this->container->get('theme_installer');
+      $theme_installer->install(['automatic_updates_theme']);
+      $theme = $this->container->get('theme_handler')
+        ->getTheme('automatic_updates_theme');
 
-    /** @var \Drupal\automatic_updates\CronUpdater $updater */
-    $updater = $this->container->get('automatic_updates.cron_updater');
+      $dir = $event->getStage()->getStageDirectory();
 
-    foreach (static::SUFFIXES as $suffix) {
-      $module_file = sprintf('%s/%s/%s.%s', $updater->getStageDirectory(), $module->getPath(), $module->getName(), $suffix);
-      $theme_file = sprintf('%s/%s/%s.%s', $updater->getStageDirectory(), $theme->getPath(), $theme->getName(), $suffix);
-      // The files we're creating shouldn't already exist in the staging area
-      // unless it's a file we actually ship, which is a scenario covered by
-      // ::testFileChanged().
-      $this->assertFileDoesNotExist($module_file);
-      $this->assertFileDoesNotExist($theme_file);
-      file_put_contents($module_file, $this->randomString());
-      file_put_contents($theme_file, $this->randomString());
-    }
-
-    $expected_results = [
-      ValidationResult::createError(['Package Manager Bypass', 'Automatic Updates Theme'], t('The update cannot proceed because possible database updates have been detected in the following extensions.')),
-    ];
+      foreach (static::SUFFIXES as $suffix) {
+        $module_file = sprintf('%s/%s/%s.%s', $dir, $module->getPath(), $module->getName(), $suffix);
+        $theme_file = sprintf('%s/%s/%s.%s', $dir, $theme->getPath(), $theme->getName(), $suffix);
+        // The files we're creating shouldn't already exist in the staging area
+        // unless it's a file we actually ship, which is a scenario covered by
+        // ::testFileChanged().
+        $this->assertFileDoesNotExist($module_file);
+        $this->assertFileDoesNotExist($theme_file);
+        file_put_contents($module_file, $this->randomString());
+        file_put_contents($theme_file, $this->randomString());
+      }
+    };
+    $this->container->get('event_dispatcher')
+      ->addListener(PreApplyEvent::class, $listener, PHP_INT_MAX);
 
-    try {
-      $updater->apply();
-      $this->fail('Expected a validation error.');
-    }
-    catch (StageValidationException $e) {
-      $this->assertValidationResultsEqual($expected_results, $e->getResults());
-    }
+    $this->container->get('cron')->run();
+    $this->assertTrue($this->logger->hasRecordThatContains("The update cannot proceed because possible database updates have been detected in the following extensions.\nPackage Manager Bypass\nAutomatic Updates Theme", RfcLogLevel::ERROR));
   }
 
 }

tests/src/Kernel/ReadinessValidation/VersionPolicyValidatorTest.php

@@ -215,10 +215,8 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
 
     return [
       'valid target, dev snapshot installed' => [
-        ['automatic_updates.updater', 'automatic_updates.cron_updater'],
         '9.8.0-dev',
         "$metadata_dir/drupal.9.8.1-security.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.1'],
         [
           $this->createValidationResult('9.8.0-dev', '9.8.1', [
@@ -229,10 +227,8 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
       // The following cases can only happen by explicitly supplying the updater
       // with an invalid target version.
       'downgrade' => [
-        ['automatic_updates.updater', 'automatic_updates.cron_updater'],
         '9.8.1',
         "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.0'],
         [
           $this->createValidationResult('9.8.1', '9.8.0', [
@@ -241,10 +237,8 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
         ],
       ],
       'major version upgrade' => [
-        ['automatic_updates.updater', 'automatic_updates.cron_updater'],
         '8.9.1',
         "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.2'],
         [
           $this->createValidationResult('8.9.1', '9.8.2', [
@@ -253,10 +247,8 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
         ],
       ],
       'unsupported target version' => [
-        ['automatic_updates.updater', 'automatic_updates.cron_updater'],
         '9.8.0',
         "$metadata_dir/drupal.9.8.2-unsupported_unpublished.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.1'],
         [
           $this->createValidationResult('9.8.0', '9.8.1', [
@@ -265,58 +257,19 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
         ],
       ],
       // This case proves that an attended update to a normal non-security
-      // release is allowed regardless of how cron is configured...
+      // release is allowed regardless of how cron is configured.
       'attended update to normal release' => [
-        ['automatic_updates.updater'],
         '9.8.1',
         "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.2'],
         [],
       ],
-      // ...and these two cases prove that an unattended update to a normal
-      // non-security release is only allowed if cron is configured to allow
-      // all updates.
-      'unattended update to normal release, security only in cron' => [
-        ['automatic_updates.cron_updater'],
-        '9.8.1',
-        "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY],
-        ['drupal' => '9.8.2'],
-        [
-          $this->createValidationResult('9.8.1', '9.8.2', [
-            'Drupal cannot be automatically updated during cron from 9.8.1 to 9.8.2 because 9.8.2 is not a security release.',
-          ]),
-        ],
-      ],
-      'unattended update to normal release, all allowed in cron' => [
-        ['automatic_updates.cron_updater'],
-        '9.8.1',
-        "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::ALL],
-        ['drupal' => '9.8.2'],
-        [],
-      ],
-      // These three cases prove that updating across minor versions of Drupal
+      // These two cases prove that updating across minor versions of Drupal
       // core is only allowed for attended updates when a specific configuration
       // flag is set.
-      'unattended update to next minor' => [
-        ['automatic_updates.cron_updater'],
-        '9.7.9',
-        "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
-        ['drupal' => '9.8.2'],
-        [
-          $this->createValidationResult('9.7.9', '9.8.2', [
-            'Drupal cannot be automatically updated from 9.7.9 to 9.8.2 because automatic updates from one minor version to another are not supported during cron.',
-          ]),
-        ],
-      ],
       'attended update to next minor not allowed' => [
-        ['automatic_updates.updater'],
         '9.7.9',
         "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.2'],
         [
           $this->createValidationResult('9.7.9', '9.8.2', [
@@ -325,10 +278,8 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
         ],
       ],
       'attended update to next minor allowed' => [
-        ['automatic_updates.updater'],
         '9.7.9',
         "$metadata_dir/drupal.9.8.2.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.2'],
         [],
         TRUE,
@@ -336,74 +287,22 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
       // If attended updates across minor versions are allowed, it's okay to
       // update from an unsupported minor version.
       'attended update from unsupported minor allowed' => [
-        ['automatic_updates.updater'],
         '9.7.9',
         "$metadata_dir/drupal.9.8.1-security.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
         ['drupal' => '9.8.1'],
         [],
         TRUE,
       ],
-      // Unattended updates to unstable versions are not allowed.
-      'unattended update to unstable version' => [
-        ['automatic_updates.cron_updater'],
-        '9.8.0',
-        "$metadata_dir/drupal.9.8.2-older-sec-release.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
-        ['drupal' => '9.8.1-beta1'],
-        [
-          $this->createValidationResult('9.8.0', '9.8.1-beta1', [
-            'Drupal cannot be automatically updated during cron to the recommended version, 9.8.1-beta1, because it is not a stable version.',
-          ]),
-        ],
-      ],
-      // Unattended updates from an unsupported minor are never allowed, but
-      // the messaging will vary depending on whether attended updates across
-      // minor versions are allowed.
-      'unattended update from unsupported minor, minor updates forbidden' => [
-        ['automatic_updates.cron_updater'],
-        '9.7.9',
-        "$metadata_dir/drupal.9.8.1-security.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
-        ['drupal' => '9.8.1'],
-        [
-          $this->createValidationResult('9.7.9', '9.8.1', [
-            'The currently installed version of Drupal core, 9.7.9, is not in a supported minor version. Your site will not be automatically updated during cron until it is updated to a supported minor version.',
-            'See the <a href="/admin/reports/updates">available updates page</a> for available updates.',
-          ]),
-        ],
-        FALSE,
-      ],
-      'unattended update from unsupported minor, minor updates allowed' => [
-        ['automatic_updates.cron_updater'],
-        '9.7.9',
-        "$metadata_dir/drupal.9.8.1-security.xml",
-        [CronUpdater::SECURITY, CronUpdater::ALL],
-        ['drupal' => '9.8.1'],
-        [
-          $this->createValidationResult('9.7.9', '9.8.1', [
-            'The currently installed version of Drupal core, 9.7.9, is not in a supported minor version. Your site will not be automatically updated during cron until it is updated to a supported minor version.',
-            'Use the <a href="/admin/modules/automatic-update">update form</a> to update to a supported version.',
-          ]),
-        ],
-        TRUE,
-      ],
     ];
   }
 
   /**
    * Tests validation of explicitly specified target versions.
    *
-   * @param string[] $updaters
-   *   The IDs of the updater services to test.
    * @param string $installed_version
    *   The installed version of Drupal core.
    * @param string $release_metadata
    *   The path of the core release metadata to serve to the update system.
-   * @param string[] $cron_modes
-   *   The modes for unattended updates. Can contain
-   *   \Drupal\automatic_updates\CronUpdater::SECURITY or
-   *   \Drupal\automatic_updates\CronUpdater::ALL.
    * @param string[] $project_versions
    *   The desired project versions that should be passed to the updater.
    * @param \Drupal\package_manager\ValidationResult[] $expected_results
@@ -414,31 +313,26 @@ class VersionPolicyValidatorTest extends AutomaticUpdatesKernelTestBase {
    *
    * @dataProvider providerApi
    */
-  public function testApi(array $updaters, string $installed_version, string $release_metadata, array $cron_modes, array $project_versions, array $expected_results, bool $allow_minor_updates = FALSE): void {
+  public function testApi(string $installed_version, string $release_metadata, array $project_versions, array $expected_results, bool $allow_minor_updates = FALSE): void {
     $this->setCoreVersion($installed_version);
     $this->setReleaseMetadata(['drupal' => $release_metadata]);
 
-    foreach ($cron_modes as $cron_mode) {
-      $this->config('automatic_updates.settings')
-        ->set('cron', $cron_mode)
-        ->set('allow_core_minor_updates', $allow_minor_updates)
-        ->save();
+    $this->config('automatic_updates.settings')
+      ->set('allow_core_minor_updates', $allow_minor_updates)
+      ->save();
 
-      foreach ($updaters as $updater) {
-        /** @var \Drupal\automatic_updates\Updater $updater */
-        $updater = $this->container->get($updater);
+    /** @var \Drupal\automatic_updates\Updater $updater */
+    $updater = $this->container->get('automatic_updates.updater');
 
-        try {
-          $updater->begin($project_versions);
-          // Ensure that we did not, in fact, expect any errors.
-          $this->assertEmpty($expected_results);
-          // Reset the updater for the next iteration of the loop.
-          $updater->destroy();
-        }
-        catch (StageValidationException $e) {
-          $this->assertValidationResultsEqual($expected_results, $e->getResults());
-        }
-      }
+    try {
+      $updater->begin($project_versions);
+      // Ensure that we did not, in fact, expect any errors.
+      $this->assertEmpty($expected_results);
+      // Reset the updater for the next iteration of the loop.
+      $updater->destroy();
+    }
+    catch (StageValidationException $e) {
+      $this->assertValidationResultsEqual($expected_results, $e->getResults());
     }
   }