Issue #3260368 by phenaproxima: Improve the UX for updates that require database changes

automatic_updates.routing.yml

@@ -13,6 +13,12 @@ automatic_updates.confirmation_page:
   requirements:
     _permission: 'administer software updates'
     _access_update_manager: 'TRUE'
+automatic_updates.finish:
+  path: '/automatic-update/finish'
+  defaults:
+    _controller: '\Drupal\automatic_updates\Controller\UpdateController::onFinish'
+  requirements:
+    _permission: 'administer software updates'
 # Links to our updater form appear in three different sets of local tasks. To ensure the breadcrumbs and paths are
 # consistent with the other local tasks in each set, we need two separate routes to the same form.
 automatic_updates.report_update:

src/BatchProcessor.php

@@ -162,12 +162,11 @@ class BatchProcessor {
    *   A list of the operations that had not been completed by the batch API.
    */
   public static function finishCommit(bool $success, array $results, array $operations): ?RedirectResponse {
-
     if ($success) {
-      \Drupal::messenger()->addMessage('Update complete!');
-      // @todo redirect to update.php?
-      return new RedirectResponse(Url::fromRoute('update.status', [],
-        ['absolute' => TRUE])->toString());
+      $url = Url::fromRoute('automatic_updates.finish')
+        ->setAbsolute()
+        ->toString();
+      return new RedirectResponse($url);
     }
     static::handleBatchError($results);
     return NULL;

src/Controller/UpdateController.php

+<?php
+
+namespace Drupal\automatic_updates\Controller;
+
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\Update\UpdateHookRegistry;
+use Drupal\Core\Update\UpdateRegistry;
+use Drupal\Core\Url;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+
+/**
+ * Defines a controller to handle various stages of an automatic update.
+ *
+ * @internal
+ *   Controller classes are internal.
+ */
+class UpdateController extends ControllerBase {
+
+  /**
+   * The update hook registry service.
+   *
+   * @var \Drupal\Core\Update\UpdateHookRegistry
+   */
+  protected $updateHookRegistry;
+
+  /**
+   * The post-update registry service.
+   *
+   * @var \Drupal\Core\Update\UpdateRegistry
+   */
+  protected $postUpdateRegistry;
+
+  /**
+   * Constructs an UpdateController object.
+   *
+   * @param \Drupal\Core\Update\UpdateHookRegistry $update_hook_registry
+   *   The update hook registry service.
+   * @param \Drupal\Core\Update\UpdateRegistry $post_update_registry
+   *   The post-update registry service.
+   */
+  public function __construct(UpdateHookRegistry $update_hook_registry, UpdateRegistry $post_update_registry) {
+    $this->updateHookRegistry = $update_hook_registry;
+    $this->postUpdateRegistry = $post_update_registry;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('update.update_hook_registry'),
+      $container->get('update.post_update_registry')
+    );
+  }
+
+  /**
+   * Redirects after staged changes are applied to the active directory.
+   *
+   * If there are any pending update hooks or post-updates, the user is sent to
+   * update.php to run those. Otherwise, they are redirected to the status
+   * report.
+   *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   A redirect to the appropriate destination.
+   */
+  public function onFinish(): RedirectResponse {
+    if ($this->pendingUpdatesExist()) {
+      $message = $this->t('Please apply database updates to complete the update process.');
+      $url = Url::fromRoute('system.db_update');
+    }
+    else {
+      $message = $this->t('Update complete!');
+      $url = Url::fromRoute('update.status');
+    }
+    $this->messenger()->addStatus($message);
+    return new RedirectResponse($url->setAbsolute()->toString());
+  }
+
+  /**
+   * Checks if there are any pending database updates.
+   *
+   * @return bool
+   *   TRUE if there are any pending update hooks or post-updates, otherwise
+   *   FALSE.
+   */
+  protected function pendingUpdatesExist(): bool {
+    if ($this->postUpdateRegistry->getPendingUpdateFunctions()) {
+      return TRUE;
+    }
+
+    $modules = array_keys($this->moduleHandler()->getModuleList());
+    foreach ($modules as $module) {
+      if ($this->updateHookRegistry->getAvailableUpdates($module)) {
+        return TRUE;
+      }
+    }
+    return FALSE;
+  }
+
+}

src/Form/UpdateReady.php

@@ -4,7 +4,9 @@ namespace Drupal\automatic_updates\Form;
 
 use Drupal\automatic_updates\BatchProcessor;
 use Drupal\automatic_updates\Updater;
+use Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator;
 use Drupal\Core\Batch\BatchBuilder;
+use Drupal\Core\Extension\ModuleExtensionList;
 use Drupal\Core\Form\FormBase;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Messenger\MessengerInterface;
@@ -34,6 +36,20 @@ class UpdateReady extends FormBase {
    */
   protected $state;
 
+  /**
+   * The module list service.
+   *
+   * @var \Drupal\Core\Extension\ModuleExtensionList
+   */
+  protected $moduleList;
+
+  /**
+   * The staged database update validator service.
+   *
+   * @var \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
+   */
+  protected $stagedDatabaseUpdateValidator;
+
   /**
    * Constructs a new UpdateReady object.
    *
@@ -43,11 +59,17 @@ class UpdateReady extends FormBase {
    *   The messenger service.
    * @param \Drupal\Core\State\StateInterface $state
    *   The state service.
+   * @param \Drupal\Core\Extension\ModuleExtensionList $module_list
+   *   The module list service.
+   * @param \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator $staged_database_update_validator
+   *   The staged database update validator service.
    */
-  public function __construct(Updater $updater, MessengerInterface $messenger, StateInterface $state) {
+  public function __construct(Updater $updater, MessengerInterface $messenger, StateInterface $state, ModuleExtensionList $module_list, StagedDatabaseUpdateValidator $staged_database_update_validator) {
     $this->updater = $updater;
     $this->setMessenger($messenger);
     $this->state = $state;
+    $this->moduleList = $module_list;
+    $this->stagedDatabaseUpdateValidator = $staged_database_update_validator;
   }
 
   /**
@@ -64,7 +86,9 @@ class UpdateReady extends FormBase {
     return new static(
       $container->get('automatic_updates.updater'),
       $container->get('messenger'),
-      $container->get('state')
+      $container->get('state'),
+      $container->get('extension.list.module'),
+      $container->get('automatic_updates.validator.staged_database_updates')
     );
   }
 
@@ -80,6 +104,23 @@ class UpdateReady extends FormBase {
       return $form;
     }
 
+    // Don't check for pending database updates if the form has been submitted,
+    // because we don't want to store the warning in the messenger during form
+    // submit.
+    if (!$form_state->getUserInput()) {
+      // If there are any installed modules with database updates in the staging
+      // area, warn the user that they might be sent to update.php once the
+      // staged changes have been applied.
+      $pending_updates = $this->getModulesWithStagedDatabaseUpdates();
+
+      if ($pending_updates) {
+        $this->messenger()->addWarning($this->t('Possible database updates were detected in the following modules; you may be redirected to the database update page in order to complete the update process.'));
+        foreach ($pending_updates as $info) {
+          $this->messenger()->addWarning($info['name']);
+        }
+      }
+    }
+
     $form['stage_id'] = [
       '#type' => 'value',
       '#value' => $stage_id,
@@ -106,6 +147,20 @@ class UpdateReady extends FormBase {
     return $form;
   }
 
+  /**
+   * Returns info for all installed modules that have staged database updates.
+   *
+   * @return array[]
+   *   The info arrays for the modules which have staged database updates, keyed
+   *   by module machine name.
+   */
+  protected function getModulesWithStagedDatabaseUpdates(): array {
+    $filter = function (string $name): bool {
+      return $this->stagedDatabaseUpdateValidator->hasStagedUpdates($this->updater, $this->moduleList->get($name));
+    };
+    return array_filter($this->moduleList->getAllInstalledInfo(), $filter, ARRAY_FILTER_USE_KEY);
+  }
+
   /**
    * {@inheritdoc}
    */

src/Validator/StagedDatabaseUpdateValidator.php

@@ -3,6 +3,7 @@
 namespace Drupal\automatic_updates\Validator;
 
 use Drupal\automatic_updates\CronUpdater;
+use Drupal\automatic_updates\Updater;
 use Drupal\Core\Extension\Extension;
 use Drupal\Core\Extension\ModuleExtensionList;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
@@ -60,22 +61,13 @@ class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
       return;
     }
 
-    $active_dir = $this->pathLocator->getProjectRoot();
-    $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))) {
+      if ($this->hasStagedUpdates($stage, $this->moduleList->get($name))) {
         $invalid_modules[] = $info['name'];
       }
     }
@@ -88,10 +80,8 @@ class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
   /**
    * 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\automatic_updates\Updater $updater
+   *   The updater which is controlling the update process.
    * @param \Drupal\Core\Extension\Extension $extension
    *   The extension to check.
    *
@@ -111,7 +101,16 @@ class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
    *
    * @see https://www.drupal.org/project/automatic_updates/issues/3253828
    */
-  protected function hasStagedUpdates(string $active_dir, string $stage_dir, Extension $extension): bool {
+  public function hasStagedUpdates(Updater $updater, Extension $extension): bool {
+    $active_dir = $this->pathLocator->getProjectRoot();
+    $stage_dir = $updater->getStageDirectory();
+
+    $web_root = $this->pathLocator->getWebRoot();
+    if ($web_root) {
+      $active_dir .= DIRECTORY_SEPARATOR . $web_root;
+      $stage_dir .= DIRECTORY_SEPARATOR . $web_root;
+    }
+
     $active_hashes = $this->getHashes($active_dir, $extension);
     $staged_hashes = $this->getHashes($stage_dir, $extension);
 

tests/modules/automatic_updates_test/automatic_updates_test.services.yml

 services:
+  automatic_updates_test.route_subscriber:
+    class: \Drupal\automatic_updates_test\Routing\RouteSubscriber
+    tags:
+      - { name: event_subscriber }
   automatic_updates_test.checker:
     class: Drupal\automatic_updates_test\ReadinessChecker\TestChecker1
     tags:

tests/modules/automatic_updates_test/src/Controller/TestUpdateController.php

+<?php
+
+namespace Drupal\automatic_updates_test\Controller;
+
+use Drupal\automatic_updates\Controller\UpdateController;
+
+/**
+ * A test-only version of the update controller.
+ */
+class TestUpdateController extends UpdateController {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function pendingUpdatesExist(): bool {
+    $pending_updates = $this->state()
+      ->get('automatic_updates_test.staged_database_updates', []);
+
+    // If the System module should expose a pending update, create one that will
+    // be detected by the update hook registry. We only do this for System so
+    // that there is NO way we could possibly evaluate any user input (i.e.,
+    // if malicious code were somehow injected into state).
+    if (array_key_exists('system', $pending_updates)) {
+      // @codingStandardsIgnoreLine
+      eval('function system_update_4294967294() {}');
+    }
+    return parent::pendingUpdatesExist();
+  }
+
+}

tests/modules/automatic_updates_test/src/Form/TestUpdateReady.php

+<?php
+
+namespace Drupal\automatic_updates_test\Form;
+
+use Drupal\automatic_updates\Form\UpdateReady;
+
+/**
+ * A test-only version of the form displayed before applying an update.
+ */
+class TestUpdateReady extends UpdateReady {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function getModulesWithStagedDatabaseUpdates(): array {
+    return $this->state->get('automatic_updates_test.staged_database_updates', parent::getModulesWithStagedDatabaseUpdates());
+  }
+
+}

tests/modules/automatic_updates_test/src/Routing/RouteSubscriber.php

+<?php
+
+namespace Drupal\automatic_updates_test\Routing;
+
+use Drupal\automatic_updates_test\Controller\TestUpdateController;
+use Drupal\automatic_updates_test\Form\TestUpdateReady;
+use Drupal\Core\Routing\RouteSubscriberBase;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Alters route definitions for testing purposes.
+ */
+class RouteSubscriber extends RouteSubscriberBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function alterRoutes(RouteCollection $collection) {
+    $collection->get('automatic_updates.confirmation_page')
+      ->setDefault('_form', TestUpdateReady::class);
+    $collection->get('automatic_updates.finish')
+      ->setDefault('_controller', TestUpdateController::class . '::onFinish');
+  }
+
+}

tests/src/Functional/AutomaticUpdatesFunctionalTestBase.php

@@ -24,21 +24,31 @@ abstract class AutomaticUpdatesFunctionalTestBase extends BrowserTestBase {
   protected function prepareSettings() {
     parent::prepareSettings();
 
-    // Disable the filesystem permissions validator, since we cannot guarantee
-    // that the current code base will be writable in all testing situations. We
-    // test this validator in our build tests, since those do give us control
-    // over the filesystem permissions.
-    // @see \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
     $settings['settings']['automatic_updates_disable_validators'] = (object) [
-      'value' => [
-        'automatic_updates.validator.file_system_permissions',
-        'package_manager.validator.file_system',
-      ],
+      'value' => $this->disableValidators(),
       'required' => TRUE,
     ];
     $this->writeSettings($settings);
   }
 
+  /**
+   * Returns the service IDs of any validators to disable.
+   *
+   * @return string[]
+   *   The service IDs of the validators to disable.
+   */
+  protected function disableValidators(): array {
+    // Disable the filesystem permissions validators, since we cannot guarantee
+    // that the current code base will be writable in all testing situations. We
+    // test these validators in our build tests, since those do give us control
+    // over the filesystem permissions.
+    // @see \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
+    return [
+      'automatic_updates.validator.file_system_permissions',
+      'package_manager.validator.file_system',
+    ];
+  }
+
   /**
    * Sets the current (running) version of core, as known to the Update module.
    *

tests/src/Functional/UpdaterFormTest.php

@@ -45,6 +45,20 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     $this->checkForUpdates();
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function disableValidators(): array {
+    $disabled_validators = parent::disableValidators();
+
+    // In this test class, all actual staging operations are bypassed by
+    // package_manager_bypass, which means this validator will complain because
+    // there is no actual Composer data for it to inspect.
+    $disabled_validators[] = 'automatic_updates.staged_projects_validator';
+
+    return $disabled_validators;
+  }
+
   /**
    * Data provider for URLs to the update form.
    *
@@ -270,4 +284,63 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     $assert_session->buttonExists('Continue');
   }
 
+  /**
+   * Tests the update form when staged modules have database updates.
+   */
+  public function testStagedDatabaseUpdates(): void {
+    $this->setCoreVersion('9.8.0');
+    $this->checkForUpdates();
+
+    // Simulate a staged database update in the System module.
+    $this->container->get('state')
+      ->set('automatic_updates_test.staged_database_updates', [
+        'system' => [
+          'name' => 'System',
+        ],
+      ]);
+
+    $page = $this->getSession()->getPage();
+    $this->drupalGet('/admin/modules/automatic-update');
+    $page->pressButton('Update');
+    $this->checkForMetaRefresh();
+    $this->assertUpdateStagedTimes(1);
+    $this->assertUpdateReady();
+    // We should see a warning about pending database updates, and once the
+    // staged changes have been applied, we should be redirected to update.php.
+    $assert_session = $this->assertSession();
+    $possible_update_message = 'Possible database updates were detected in the following modules; you may be redirected to the database update page in order to complete the update process.';
+    $assert_session->pageTextContains($possible_update_message);
+    $assert_session->pageTextContains('System');
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    $assert_session->addressEquals('/update.php');
+    $assert_session->pageTextNotContains($possible_update_message);
+    $assert_session->pageTextContainsOnce('Please apply database updates to complete the update process.');
+  }
+
+  /**
+   * Tests an update that has no errors or special conditions.
+   *
+   * @param string $update_form_url
+   *   The URL of the update form to visit.
+   *
+   * @dataProvider providerUpdateFormReferringUrl
+   */
+  public function testSuccessfulUpdate(string $update_form_url): void {
+    $this->setCoreVersion('9.8.0');
+    $this->checkForUpdates();
+
+    $page = $this->getSession()->getPage();
+    $this->drupalGet($update_form_url);
+    $page->pressButton('Update');
+    $this->checkForMetaRefresh();
+    $this->assertUpdateStagedTimes(1);
+    $this->assertUpdateReady();
+    $assert_session = $this->assertSession();
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    $assert_session->addressEquals('/admin/reports/updates');
+    $assert_session->pageTextContainsOnce('Update complete!');
+  }
+
 }