Issue #3252126 by tedbow, phenaproxima: Do not apply updates during cron if...

Issue #3252126 by tedbow, phenaproxima: Do not apply updates during cron if there are DB updates in the staging area

automatic_updates.module

@@ -73,9 +73,7 @@ function automatic_updates_module_implements_alter(&$implementations, $hook) {
  * Implements hook_cron().
  */
 function automatic_updates_cron() {
-  /** @var \Drupal\automatic_updates\CronUpdater $cron_updater */
-  $cron_updater = \Drupal::classResolver(CronUpdater::class);
-  $cron_updater->handleCron();
+  \Drupal::service('automatic_updates.cron_updater')->handleCron();
 
   /** @var \Drupal\automatic_updates\Validation\ReadinessValidationManager $checker_manager */
   $checker_manager = \Drupal::service('automatic_updates.readiness_validation_manager');

automatic_updates.services.yml

@@ -18,6 +18,18 @@ services:
       - '@file_system'
       - '@event_dispatcher'
       - '@tempstore.shared'
+  automatic_updates.cron_updater:
+    class: Drupal\automatic_updates\CronUpdater
+    arguments:
+      - '@config.factory'
+      - '@logger.factory'
+      - '@package_manager.path_locator'
+      - '@package_manager.beginner'
+      - '@package_manager.stager'
+      - '@package_manager.committer'
+      - '@file_system'
+      - '@event_dispatcher'
+      - '@tempstore.shared'
   automatic_updates.excluded_paths_subscriber:
     class: Drupal\automatic_updates\Event\ExcludedPathsSubscriber
     arguments:
@@ -86,3 +98,11 @@ services:
       - '@string_translation'
     tags:
       - { name: event_subscriber }
+  automatic_updates.validator.staged_database_updates:
+    class: Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
+    arguments:
+      - '@package_manager.path_locator'
+      - '@extension.list.module'
+      - '@string_translation'
+    tags:
+      - { name: event_subscriber }

src/CronUpdater.php

@@ -3,9 +3,7 @@
 namespace Drupal\automatic_updates;
 
 use Drupal\Core\Config\ConfigFactoryInterface;
-use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Logger\LoggerChannelFactoryInterface;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
  * Defines a service that updates via cron.
@@ -14,7 +12,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
  *   This class implements logic specific to Automatic Updates' cron hook
  *   implementation. It should not be called directly.
  */
-class CronUpdater implements ContainerInjectionInterface {
+class CronUpdater extends Updater {
 
   /**
    * All automatic updates are disabled.
@@ -37,13 +35,6 @@ class CronUpdater implements ContainerInjectionInterface {
    */
   public const ALL = 'patch';
 
-  /**
-   * The updater service.
-   *
-   * @var \Drupal\automatic_updates\Updater
-   */
-  protected $updater;
-
   /**
    * The config factory service.
    *
@@ -61,30 +52,19 @@ class CronUpdater implements ContainerInjectionInterface {
   /**
    * Constructs a CronUpdater object.
    *
-   * @param \Drupal\automatic_updates\Updater $updater
-   *   The updater service.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory service.
    * @param \Drupal\Core\Logger\LoggerChannelFactoryInterface $logger_factory
    *   The logger channel factory.
+   * @param mixed ...$arguments
+   *   Additional arguments to pass to the parent constructor.
    */
-  public function __construct(Updater $updater, ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory) {
-    $this->updater = $updater;
+  public function __construct(ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory, ...$arguments) {
+    parent::__construct(...$arguments);
     $this->configFactory = $config_factory;
     $this->logger = $logger_factory->get('automatic_updates');
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public static function create(ContainerInterface $container) {
-    return new static(
-      $container->get('automatic_updates.updater'),
-      $container->get('config.factory'),
-      $container->get('logger.factory')
-    );
-  }
-
   /**
    * Handles updates during cron.
    */
@@ -127,12 +107,12 @@ class CronUpdater implements ContainerInjectionInterface {
     //   cron runs.
     $recommended_version = $recommended_release->getVersion();
     try {
-      $this->updater->begin([
+      $this->begin([
         'drupal' => $recommended_version,
       ]);
-      $this->updater->stage();
-      $this->updater->apply();
-      $this->updater->destroy();
+      $this->stage();
+      $this->apply();
+      $this->destroy();
     }
     catch (\Throwable $e) {
       $this->logger->error($e->getMessage());

src/Validator/StagedDatabaseUpdateValidator.php

+<?php
+
+namespace Drupal\automatic_updates\Validator;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\Core\Extension\Extension;
+use Drupal\Core\Extension\ModuleExtensionList;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\StringTranslation\TranslationInterface;
+use Drupal\package_manager\Event\PreApplyEvent;
+use Drupal\package_manager\PathLocator;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Validates that there are no database updates in a staged update.
+ */
+class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
+
+  use StringTranslationTrait;
+
+  /**
+   * The path locator service.
+   *
+   * @var \Drupal\package_manager\PathLocator
+   */
+  protected $pathLocator;
+
+  /**
+   * The module list service.
+   *
+   * @var \Drupal\Core\Extension\ModuleExtensionList
+   */
+  protected $moduleList;
+
+  /**
+   * Constructs a StagedDatabaseUpdateValidator object.
+   *
+   * @param \Drupal\package_manager\PathLocator $path_locator
+   *   The path locator service.
+   * @param \Drupal\Core\Extension\ModuleExtensionList $module_list
+   *   The module list service.
+   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation
+   *   The string translation service.
+   */
+  public function __construct(PathLocator $path_locator, ModuleExtensionList $module_list, TranslationInterface $translation) {
+    $this->pathLocator = $path_locator;
+    $this->moduleList = $module_list;
+    $this->setStringTranslation($translation);
+  }
+
+  /**
+   * Checks that the staged update does not have changes to its install files.
+   *
+   * @param \Drupal\package_manager\Event\PreApplyEvent $event
+   *   The event object.
+   */
+  public function checkUpdateHooks(PreApplyEvent $event): void {
+    $stage = $event->getStage();
+    if (!$stage instanceof CronUpdater) {
+      return;
+    }
+
+    $active_dir = $this->pathLocator->getActiveDirectory();
+    $stage_dir = $stage->getStageDirectory();
+
+    $web_root = $this->pathLocator->getWebRoot();
+    if ($web_root) {
+      $active_dir .= DIRECTORY_SEPARATOR . $web_root;
+      $stage_dir .= DIRECTORY_SEPARATOR . $web_root;
+    }
+
+    $invalid_modules = [];
+    // Although \Drupal\automatic_updates\Validator\StagedProjectsValidator
+    // should prevent non-core modules from being added, updated, or removed in
+    // the staging area, we check all installed modules so as not to rely on the
+    // presence of StagedProjectsValidator.
+    foreach ($this->moduleList->getAllInstalledInfo() as $name => $info) {
+      if ($this->hasStagedUpdates($active_dir, $stage_dir, $this->moduleList->get($name))) {
+        $invalid_modules[] = $info['name'];
+      }
+    }
+
+    if ($invalid_modules) {
+      $event->addError($invalid_modules, $this->t('The update cannot proceed because possible database updates have been detected in the following modules.'));
+    }
+  }
+
+  /**
+   * Determines if a staged extension has changed update functions.
+   *
+   * @param string $active_dir
+   *   The path of the running Drupal code base.
+   * @param string $stage_dir
+   *   The path of the staging area.
+   * @param \Drupal\Core\Extension\Extension $extension
+   *   The extension to check.
+   *
+   * @return bool
+   *   TRUE if the staged copy of the extension has changed update functions
+   *   compared to the active copy, FALSE otherwise.
+   *
+   * @todo Use a more sophisticated method to detect changes in the staged
+   *   extension. Right now, we just compare hashes of the .install and
+   *   .post_update.php files in both copies of the given extension, but this
+   *   will cause false positives for changes to comments, whitespace, or
+   *   runtime code like requirements checks. It would be preferable to use a
+   *   static analyzer to detect new or changed functions that are actually
+   *   executed during an update. No matter what, this method must NEVER cause
+   *   false negatives, since that could result in code which is incompatible
+   *   with the current database schema being copied to the active directory.
+   *
+   * @see https://www.drupal.org/project/automatic_updates/issues/3253828
+   */
+  protected function hasStagedUpdates(string $active_dir, string $stage_dir, Extension $extension): bool {
+    $active_hashes = $this->getHashes($active_dir, $extension);
+    $staged_hashes = $this->getHashes($stage_dir, $extension);
+
+    return $active_hashes !== $staged_hashes;
+  }
+
+  /**
+   * Returns hashes of the .install and .post-update.php files for a module.
+   *
+   * @param string $root_dir
+   *   The root directory of the Drupal code base.
+   * @param \Drupal\Core\Extension\Extension $module
+   *   The module to check.
+   *
+   * @return string[]
+   *   The hashes of the module's .install and .post_update.php files, in that
+   *   order, if they exist. The array will be keyed by file extension.
+   */
+  protected function getHashes(string $root_dir, Extension $module): array {
+    $path = implode(DIRECTORY_SEPARATOR, [
+      $root_dir,
+      $module->getPath(),
+      $module->getName(),
+    ]);
+    $hashes = [];
+
+    foreach (['.install', '.post_update.php'] as $suffix) {
+      $file = $path . $suffix;
+
+      if (file_exists($file)) {
+        $hashes[$suffix] = hash_file('sha256', $file);
+      }
+    }
+    return $hashes;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    return [
+      PreApplyEvent::class => 'checkUpdateHooks',
+    ];
+  }
+
+}

tests/src/Kernel/CronUpdaterTest.php

@@ -4,8 +4,8 @@ namespace Drupal\Tests\automatic_updates\Kernel;
 
 use Drupal\automatic_updates\CronUpdater;
 use Drupal\Core\Form\FormState;
-use Drupal\package_manager\ComposerUtility;
 use Drupal\update\UpdateSettingsForm;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 
 /**
  * @covers \Drupal\automatic_updates\CronUpdater
@@ -21,6 +21,7 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
   protected static $modules = [
     'automatic_updates',
     'package_manager',
+    'package_manager_bypass',
   ];
 
   /**
@@ -97,21 +98,20 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
     $form_state->setValue('automatic_updates_cron', $setting);
     $form_builder->submitForm(UpdateSettingsForm::class, $form_state);
 
-    // Mock the updater so we can assert that its methods are called or bypassed
-    // depending on configuration.
-    $will_update = (int) $will_update;
-    $updater = $this->prophesize('\Drupal\automatic_updates\Updater');
-
-    $composer = ComposerUtility::createForDirectory(__DIR__ . '/../../fixtures/fake-site');
-    $updater->getActiveComposer()->willReturn($composer);
-
-    $updater->begin(['drupal' => '9.8.1'])->shouldBeCalledTimes($will_update);
-    $updater->stage()->shouldBeCalledTimes($will_update);
-    $updater->apply()->shouldBeCalledTimes($will_update);
-    $updater->destroy()->shouldBeCalledTimes($will_update);
-    $this->container->set('automatic_updates.updater', $updater->reveal());
+    // Since we're just trying to ensure that all of Package Manager's services
+    // are called as expected, disable validation by replacing the event
+    // dispatcher with a dummy version.
+    $event_dispatcher = $this->prophesize(EventDispatcherInterface::class);
+    $this->container->set('event_dispatcher', $event_dispatcher->reveal());
 
+    // Run cron and ensure that Package Manager's services were called or
+    // bypassed depending on configuration.
     $this->container->get('cron')->run();
+
+    $will_update = (int) $will_update;
+    $this->assertCount($will_update, $this->container->get('package_manager.beginner')->getInvocationArguments());
+    $this->assertCount($will_update, $this->container->get('package_manager.stager')->getInvocationArguments());
+    $this->assertCount($will_update, $this->container->get('package_manager.committer')->getInvocationArguments());
   }
 
 }

tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php

+<?php
+
+namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
+
+use Drupal\automatic_updates\CronUpdater;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\package_manager\Exception\StageValidationException;
+use Drupal\package_manager\ValidationResult;
+use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+
+/**
+ * @covers \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
+ *
+ * @group automatic_updates
+ */
+class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = [
+    'automatic_updates',
+    'package_manager',
+    'package_manager_bypass',
+  ];
+
+  /**
+   * The suffixes of the files that can contain database updates.
+   *
+   * @var string[]
+   */
+  private const SUFFIXES = ['install', 'post_update.php'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+
+    TestCronUpdater::$stagingRoot = $this->vfsRoot->url();
+
+    /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+    $updater = $this->container->get('automatic_updates.cron_updater');
+    $updater->begin(['drupal' => '9.8.1']);
+    $updater->stage();
+
+    $stage_dir = $updater->getStageDirectory();
+    mkdir($stage_dir);
+
+    // To satisfy StagedProjectsValidator, copy the active Composer files into
+    // the staging area.
+    $active_dir = $this->getDrupalRoot();
+    @copy("$active_dir/composer.json", "$stage_dir/composer.json");
+    @copy("$active_dir/composer.lock", "$stage_dir/composer.lock");
+
+    // Copy the .install and .post_update.php files from every installed module
+    // into the staging directory.
+    $module_list = $this->container->get('module_handler')->getModuleList();
+    foreach ($module_list as $name => $module) {
+      $path = $module->getPath();
+      mkdir("$stage_dir/$path", 0777, TRUE);
+
+      foreach (static::SUFFIXES as $suffix) {
+        // If the source file doesn't exist, silence the warning it raises.
+        @copy("$active_dir/$path/$name.$suffix", "$stage_dir/$path/$name.$suffix");
+      }
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
+
+    $container->getDefinition('automatic_updates.cron_updater')
+      ->setClass(TestCronUpdater::class);
+  }
+
+  /**
+   * Tests that no errors are raised if staged files have no DB updates.
+   */
+  public function testNoUpdates(): void {
+    // Since we're testing with a modified version of Views, it should not be
+    // installed.
+    $this->assertFalse($this->container->get('module_handler')->moduleExists('views'));
+
+    // Create bogus staged versions of Views' .install and .post_update.php
+    // files. Since it's not installed, the changes should not raise any
+    // validation errors.
+    $updater = $this->container->get('automatic_updates.cron_updater');
+    $module_dir = $updater->getStageDirectory() . '/core/modules/views';
+    mkdir($module_dir, 0777, TRUE);
+    foreach (static::SUFFIXES as $suffix) {
+      file_put_contents("$module_dir/views.$suffix", $this->randomString());
+    }
+
+    $updater->apply();
+  }
+
+  /**
+   * Data provider for ::testFileChanged().
+   *
+   * @return array[]
+   *   Sets of arguments to pass to the test method.
+   */
+  public function providerFileChanged(): array {
+    $scenarios = [];
+    foreach (static::SUFFIXES as $suffix) {
+      $scenarios[] = [$suffix, FALSE];
+      $scenarios[] = [$suffix, TRUE];
+    }
+    return $scenarios;
+  }
+
+  /**
+   * Tests that an error is raised if install or post-update files are changed.
+   *
+   * @param string $suffix
+   *   The suffix of the file to change. Can be either 'install' or
+   *   'post_update.php'.
+   * @param bool $delete
+   *   Whether or not to delete the file.
+   *
+   * @dataProvider providerFileChanged
+   */
+  public function testFileChanged(string $suffix, bool $delete): void {
+    /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+    $updater = $this->container->get('automatic_updates.cron_updater');
+
+    $file = $updater->getStageDirectory() . "/core/modules/system/system.$suffix";
+    if ($delete) {
+      unlink($file);
+    }
+    else {
+      file_put_contents($file, $this->randomString());
+    }
+
+    $expected_results = [
+      ValidationResult::createError(['System'], t('The update cannot proceed because possible database updates have been detected in the following modules.')),
+    ];
+
+    try {
+      $updater->apply();
+      $this->fail('Expected a validation error.');
+    }
+    catch (StageValidationException $e) {
+      $this->assertValidationResultsEqual($expected_results, $e->getResults());
+    }
+  }
+
+  /**
+   * 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');
+
+    /** @var \Drupal\Tests\automatic_updates\Kernel\ReadinessValidation\TestCronUpdater $updater */
+    $updater = $this->container->get('automatic_updates.cron_updater');
+
+    foreach (static::SUFFIXES as $suffix) {
+      $file = sprintf('%s/%s/%s.%s', $updater->getStageDirectory(), $module->getPath(), $module->getName(), $suffix);
+      // The file 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->assertFileNotExists($file);
+      file_put_contents($file, $this->randomString());
+    }
+
+    $expected_results = [
+      ValidationResult::createError(['Package Manager Bypass'], t('The update cannot proceed because possible database updates have been detected in the following modules.')),
+    ];
+
+    try {
+      $updater->apply();
+      $this->fail('Expected a validation error.');
+    }
+    catch (StageValidationException $e) {
+      $this->assertValidationResultsEqual($expected_results, $e->getResults());
+    }
+  }
+
+}
+
+/**
+ * A test-only version of the cron updater.
+ */
+class TestCronUpdater extends CronUpdater {
+
+  /**
+   * The directory where staging areas will be created.
+   *
+   * @var string
+   */
+  public static $stagingRoot;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static function getStagingRoot(): string {
+    return static::$stagingRoot ?: parent::getStagingRoot();
+  }
+
+}