Contrib: Issue #3260698 by phenaproxima, tedbow: Make it simpler to disable...

Contrib: Issue #3260698 by phenaproxima, tedbow: Make it simpler to disable validators in tests - project/automatic_updates@d4fb6ede

core/modules/auto_updates/auto_updates.services.yml

@@ -9,6 +9,8 @@ services:
       - '@auto_updates.cron_updater'
       - '@config.factory'
       - 24
+    tags:
+      - { name: event_subscriber }
   auto_updates.updater:
     class: Drupal\auto_updates\Updater
     arguments:

core/modules/auto_updates/src/Validation/ReadinessValidationManager.php

@@ -9,12 +9,14 @@
 use Drupal\Component\Datetime\TimeInterface;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface;
+use Drupal\package_manager\Event\PostApplyEvent;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
  * Defines a manager to run readiness validation.
  */
-class ReadinessValidationManager {
+class ReadinessValidationManager implements EventSubscriberInterface {
 
   /**
    * The key/value expirable storage.
@@ -213,6 +215,13 @@ protected function getStoredValidResults(): ?array {
     return NULL;
   }
 
+  /**
+   * Deletes any stored readiness validation results.
+   */
+  public function clearStoredResults(): void {
+    $this->keyValueExpirable->delete('readiness_validation_last_run');
+  }
+
   /**
    * Gets the timestamp of the last run.
    *
@@ -224,4 +233,13 @@ public function getLastRunTime(): ?int {
     return $this->keyValueExpirable->get('readiness_check_timestamp');
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    return [
+      PostApplyEvent::class => 'clearStoredResults',
+    ];
+  }
+
 }

core/modules/auto_updates/tests/modules/auto_updates_test_disable_validators/auto_updates_test_disable_validators.info.yml

-name: 'Automatic Updates Test: Disable validators'
-description: Allows certain update validators to be disabled during testing.
-type: module
-package: Testing

core/modules/auto_updates/tests/modules/auto_updates_test_disable_validators/src/AutoUpdatesTestDisableValidatorsServiceProvider.php

-<?php
-
-namespace Drupal\auto_updates_test_disable_validators;
-
-use Drupal\Core\DependencyInjection\ContainerBuilder;
-use Drupal\Core\DependencyInjection\ServiceProviderBase;
-use Drupal\Core\Site\Settings;
-
-/**
- * Allows specific validators to be disabled by site settings.
- *
- * This should only really be used by functional tests. Kernel tests should
- * override their ::register() method to remove service definitions; build tests
- * should stay out of the API/services layer unless absolutely necessary.
- */
-class AutoUpdatesTestDisableValidatorsServiceProvider extends ServiceProviderBase {
-
-  /**
-   * {@inheritdoc}
-   */
-  public function alter(ContainerBuilder $container) {
-    $validators = Settings::get('auto_updates_disable_validators', []);
-    array_walk($validators, [$container, 'removeDefinition']);
-  }
-
-}

core/modules/auto_updates/tests/src/Functional/AutoUpdatesFunctionalTestBase.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\auto_updates\Functional;
 
+use Drupal\Component\Serialization\Yaml;
 use Drupal\Tests\BrowserTestBase;
 
 /**
@@ -12,41 +13,54 @@ abstract class AutoUpdatesFunctionalTestBase extends BrowserTestBase {
   /**
    * {@inheritdoc}
    */
-  protected static $modules = [
-    'auto_updates_test_disable_validators',
-    'update',
-    'update_test',
+  protected static $modules = ['update', 'update_test'];
+
+  /**
+   * The service IDs of any validators to disable.
+   *
+   * @var string[]
+   */
+  protected $disableValidators = [
+    // 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\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
+    'auto_updates.validator.file_system_permissions',
+    'package_manager.validator.file_system',
   ];
 
   /**
    * {@inheritdoc}
    */
-  protected function prepareSettings() {
-    parent::prepareSettings();
-
-    $settings['settings']['auto_updates_disable_validators'] = (object) [
-      'value' => $this->disableValidators(),
-      'required' => TRUE,
-    ];
-    $this->writeSettings($settings);
+  protected function setUp() {
+    parent::setUp();
+    $this->disableValidators($this->disableValidators);
   }
 
   /**
-   * Returns the service IDs of any validators to disable.
+   * Disables validators in the test site's services.yml.
+   *
+   * This modifies the service container such that the disabled validators are
+   * instances of stdClass, and not subscribed to any events.
    *
-   * @return string[]
+   * @param string[] $validators
    *   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\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
-    return [
-      'auto_updates.validator.file_system_permissions',
-      'package_manager.validator.file_system',
-    ];
+  protected function disableValidators(array $validators): void {
+    $services_file = $this->getDrupalRoot() . '/' . $this->siteDirectory . '/services.yml';
+    $this->assertFileIsWritable($services_file);
+    $services = file_get_contents($services_file);
+    $services = Yaml::decode($services);
+
+    foreach ($validators as $service_id) {
+      $services['services'][$service_id] = [
+        'class' => 'stdClass',
+      ];
+    }
+    file_put_contents($services_file, Yaml::encode($services));
+    // Ensure the container is rebuilt ASAP.
+    $this->kernel->invalidateContainer();
   }
 
   /**

core/modules/auto_updates/tests/src/Functional/ReadinessValidationTest.php

@@ -369,6 +369,58 @@ public function testReadinessCheckerUninstall(): void {
     $assert->pageTextNotContains($expected_results_1[0]->getMessages()[0]);
   }
 
+  /**
+   * Tests that stored validation results are deleted after an update.
+   */
+  public function testStoredResultsClearedAfterUpdate(): void {
+    // Because all actual staging operations are bypassed by
+    // package_manager_bypass, disable this validator because it will complain
+    // if there's no actual Composer data to inspect.
+    $this->disableValidators(['auto_updates.staged_projects_validator']);
+
+    $assert_session = $this->assertSession();
+    $page = $this->getSession()->getPage();
+    $this->drupalLogin($this->checkerRunnerUser);
+
+    // The current release is 9.8.1 (see ::setUp()), so ensure we're on an older
+    // version.
+    $this->setCoreVersion('9.8.0');
+
+    // Flag a validation warning, which will be displayed in the messages area,
+    // but not block or abort the update.
+    $results = $this->testResults['checker_1']['1 warning'];
+    TestChecker1::setTestResult($results, ReadinessCheckEvent::class);
+    $message = $results[0]->getMessages()[0];
+
+    $this->container->get('module_installer')->install([
+      'auto_updates',
+      'auto_updates_test',
+      'package_manager_bypass',
+    ]);
+
+    // The warning should be persistently visible, even after the checker stops
+    // flagging it.
+    $this->drupalGet('/admin/structure');
+    $assert_session->pageTextContains($message);
+    TestChecker1::setTestResult(NULL, ReadinessCheckEvent::class);
+    $this->getSession()->reload();
+    $assert_session->pageTextContains($message);
+
+    // Do the update; we don't expect any errors or special conditions to appear
+    // during it.
+    $this->drupalGet('/admin/modules/automatic-update');
+    $page->pressButton('Update');
+    $this->checkForMetaRefresh();
+    $this->assertUpdateReady();
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    $assert_session->pageTextContains('Update complete!');
+
+    // The warning should not be visible anymore.
+    $this->drupalGet('/admin/structure');
+    $assert_session->pageTextNotContains($message);
+  }
+
   /**
    * Asserts that the readiness requirement displays no errors or warnings.
    *

core/modules/auto_updates/tests/src/Functional/UpdaterFormTest.php

@@ -6,8 +6,8 @@
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\ValidationResult;
 use Drupal\auto_updates_test\ReadinessChecker\TestChecker1;
-use Drupal\Tests\package_manager\Traits\PackageManagerBypassTestTrait;
 use Drupal\Tests\auto_updates\Traits\ValidationTestTrait;
+use Drupal\Tests\package_manager\Traits\PackageManagerBypassTestTrait;
 
 /**
  * @covers \Drupal\auto_updates\Form\UpdaterForm
@@ -16,8 +16,8 @@
  */
 class UpdaterFormTest extends AutoUpdatesFunctionalTestBase {
 
-  use ValidationTestTrait;
   use PackageManagerBypassTestTrait;
+  use ValidationTestTrait;
 
   /**
    * {@inheritdoc}
@@ -38,6 +38,11 @@ class UpdaterFormTest extends AutoUpdatesFunctionalTestBase {
    * {@inheritdoc}
    */
   protected function setUp(): void {
+    // 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.
+    $this->disableValidators[] = 'auto_updates.staged_projects_validator';
+
     parent::setUp();
 
     $this->setReleaseMetadata(__DIR__ . '/../../fixtures/release-history/drupal.9.8.1-security.xml');
@@ -45,20 +50,6 @@ protected function setUp(): void {
     $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[] = 'auto_updates.staged_projects_validator';
-
-    return $disabled_validators;
-  }
-
   /**
    * Data provider for URLs to the update form.
    *

core/modules/auto_updates/tests/src/Kernel/AutoUpdatesKernelTestBase.php

@@ -25,6 +25,21 @@ abstract class AutoUpdatesKernelTestBase extends KernelTestBase {
    */
   protected static $modules = ['system', 'update', 'update_test'];
 
+  /**
+   * The service IDs of any validators to disable.
+   *
+   * @var string[]
+   */
+  protected $disableValidators = [
+    // 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 functionally in our build tests, since those do
+    // give us control over the filesystem permissions.
+    // @see \Drupal\Tests\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
+    'auto_updates.validator.file_system_permissions',
+    'package_manager.validator.file_system',
+  ];
+
   /**
    * The mocked HTTP client that returns metadata about available updates.
    *
@@ -93,20 +108,11 @@ public function register(ContainerBuilder $container) {
       $container->set('http_client', $this->client);
     }
 
-    $this->disableValidators($container);
-  }
-
-  /**
-   * Disables any validators that will interfere with this test.
-   */
-  protected function disableValidators(ContainerBuilder $container): void {
-    // 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 functionally in our build tests, since those do
-    // give us control over the filesystem permissions.
-    // @see \Drupal\Tests\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
-    $container->removeDefinition('auto_updates.validator.file_system_permissions');
-    $container->removeDefinition('package_manager.validator.file_system');
+    foreach ($this->disableValidators as $service_id) {
+      if ($container->hasDefinition($service_id)) {
+        $container->getDefinition($service_id)->clearTag('event_subscriber');
+      }
+    }
   }
 
   /**

core/modules/auto_updates/tests/src/Kernel/ReadinessValidation/PackageManagerReadinessChecksTest.php

@@ -3,7 +3,6 @@
 namespace Drupal\Tests\auto_updates\Kernel\ReadinessValidation;
 
 use Drupal\auto_updates\Event\ReadinessCheckEvent;
-use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\package_manager\Validator\PreOperationStageValidatorInterface;
 use Drupal\Tests\auto_updates\Kernel\AutoUpdatesKernelTestBase;
 use Prophecy\Argument;
@@ -33,9 +32,10 @@ class PackageManagerReadinessChecksTest extends AutoUpdatesKernelTestBase {
   /**
    * {@inheritdoc}
    */
-  protected function disableValidators(ContainerBuilder $container): void {
-    // No need to disable any validators in this test.
-  }
+  protected $disableValidators = [
+    // The parent class disables one of the validators that we're testing, so
+    // prevent that with an empty array.
+  ];
 
   /**
    * Data provider for ::testValidatorInvoked().

core/modules/auto_updates/tests/src/Kernel/ReadinessValidation/ReadinessValidationManagerTest.php

@@ -23,6 +23,7 @@ class ReadinessValidationManagerTest extends AutoUpdatesKernelTestBase {
   protected static $modules = [
     'auto_updates_test',
     'package_manager',
+    'package_manager_bypass',
     'user',
   ];
 
@@ -233,4 +234,45 @@ public function testCronSetting(): void {
     $this->assertSame(Updater::class, $stage_class);
   }
 
+  /**
+   * Tests that stored validation results are deleted after an update.
+   */
+  public function testStoredResultsDeletedPostApply(): void {
+    $this->container->get('module_installer')
+      ->install(['auto_updates']);
+
+    // Ensure there's a simulated core release to update to.
+    $this->setReleaseMetadata(__DIR__ . '/../../../fixtures/release-history/drupal.9.8.1.xml');
+
+    // The readiness checker should raise a warning, so that the update is not
+    // blocked or aborted.
+    $results = $this->testResults['checker_1']['1 warning'];
+    TestChecker1::setTestResult($results, ReadinessCheckEvent::class);
+
+    // Ensure that the validation manager collects the warning.
+    /** @var \Drupal\auto_updates\Validation\ReadinessValidationManager $manager */
+    $manager = $this->container->get('auto_updates.readiness_validation_manager')
+      ->run();
+    TestChecker1::setTestResult(NULL, ReadinessCheckEvent::class);
+    // Even though the checker no longer returns any results, the previous
+    // results should be stored.
+    $this->assertValidationResultsEqual($results, $manager->getResults());
+
+    // Don't validate staged projects because actual staging operations are
+    // bypassed by package_manager_bypass, which will make this validator
+    // complain that there is no actual Composer data for it to inspect.
+    $validator = $this->container->get('auto_updates.staged_projects_validator');
+    $this->container->get('event_dispatcher')->removeSubscriber($validator);
+
+    /** @var \Drupal\auto_updates\Updater $updater */
+    $updater = $this->container->get('auto_updates.updater');
+    $updater->begin(['drupal' => '9.8.1']);
+    $updater->stage();
+    $updater->apply();
+    $updater->destroy();
+
+    // The readiness validation manager shouldn't have any stored results.
+    $this->assertEmpty($manager->getResults());
+  }
+
 }

core/modules/auto_updates/tests/src/Kernel/ReadinessValidation/StagedProjectsValidatorTest.php

@@ -30,22 +30,21 @@ class StagedProjectsValidatorTest extends AutoUpdatesKernelTestBase {
   /**
    * {@inheritdoc}
    */
-  public function register(ContainerBuilder $container) {
-    parent::register($container);
-
-    $container->getDefinition('auto_updates.updater')
-      ->setClass(TestUpdater::class);
+  protected function setUp(): void {
+    // This test deals with fake sites that don't necessarily have lock files,
+    // so disable lock file validation.
+    $this->disableValidators[] = 'package_manager.validator.lock_file';
+    parent::setUp();
   }
 
   /**
    * {@inheritdoc}
    */
-  protected function disableValidators(ContainerBuilder $container): void {
-    parent::disableValidators($container);
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
 
-    // This test deals with fake sites that don't necessarily have lock files,
-    // so disable lock file validation.
-    $container->removeDefinition('package_manager.validator.lock_file');
+    $container->getDefinition('auto_updates.updater')
+      ->setClass(TestUpdater::class);
   }
 
   /**

core/modules/package_manager/src/Event/PreOperationStageEvent.php

@@ -10,6 +10,32 @@
  */
 abstract class PreOperationStageEvent extends StageEvent {
 
+  /**
+   * The validation results.
+   *
+   * @var \Drupal\package_manager\ValidationResult[]
+   */
+  protected $results = [];
+
+  /**
+   * Gets the validation results.
+   *
+   * @param int|null $severity
+   *   (optional) The severity for the results to return. Should be one of the
+   *   SystemManager::REQUIREMENT_* constants.
+   *
+   * @return \Drupal\package_manager\ValidationResult[]
+   *   The validation results.
+   */
+  public function getResults(?int $severity = NULL): array {
+    if ($severity !== NULL) {
+      return array_filter($this->results, function ($result) use ($severity) {
+        return $result->getSeverity() === $severity;
+      });
+    }
+    return $this->results;
+  }
+
   /**
    * Adds error information to the event.
    */

core/modules/package_manager/src/Event/StageEvent.php

@@ -10,13 +10,6 @@
  */
 abstract class StageEvent extends Event {
 
-  /**
-   * The validation results.
-   *
-   * @var \Drupal\package_manager\ValidationResult[]
-   */
-  protected $results = [];
-
   /**
    * The stage which fired this event.
    *
@@ -44,23 +37,4 @@ public function getStage(): Stage {
     return $this->stage;
   }
 
-  /**
-   * Gets the validation results.
-   *
-   * @param int|null $severity
-   *   (optional) The severity for the results to return. Should be one of the
-   *   SystemManager::REQUIREMENT_* constants.
-   *
-   * @return \Drupal\package_manager\ValidationResult[]
-   *   The validation results.
-   */
-  public function getResults(?int $severity = NULL): array {
-    if ($severity !== NULL) {
-      return array_filter($this->results, function ($result) use ($severity) {
-        return $result->getSeverity() === $severity;
-      });
-    }
-    return $this->results;
-  }
-
 }

core/modules/package_manager/src/Stage.php

@@ -14,6 +14,7 @@
 use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreDestroyEvent;
+use Drupal\package_manager\Event\PreOperationStageEvent;
 use Drupal\package_manager\Event\PreRequireEvent;
 use Drupal\package_manager\Event\StageEvent;
 use Drupal\package_manager\Exception\StageException;
@@ -341,9 +342,11 @@ protected function dispatch(StageEvent $event): void {
     try {
       $this->eventDispatcher->dispatch($event);
 
-      $results = $event->getResults();
-      if ($results) {
-        $error = new StageValidationException($results);
+      if ($event instanceof PreOperationStageEvent) {
+        $results = $event->getResults();
+        if ($results) {
+          $error = new StageValidationException($results);
+        }
       }
     }
     catch (\Throwable $error) {

core/modules/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php

@@ -28,6 +28,21 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
     'package_manager_bypass',
   ];
 
+  /**
+   * The service IDs of any validators to disable.
+   *
+   * @var string[]
+   */
+  protected $disableValidators = [
+    // 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 functionally in Automatic Updates' build tests,
+    // since those do give us control over the filesystem permissions.
+    // @see \Drupal\Tests\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
+    // @see \Drupal\Tests\package_manager\Kernel\WritableFileSystemValidatorTest
+    'package_manager.validator.file_system',
+  ];
+
   /**
    * {@inheritdoc}
    */
@@ -41,20 +56,12 @@ protected function setUp(): void {
    */
   public function register(ContainerBuilder $container) {
     parent::register($container);
-    $this->disableValidators($container);
-  }
 
-  /**
-   * Disables any validators that will interfere with this test.
-   */
-  protected function disableValidators(ContainerBuilder $container): void {
-    // 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 functionally in Automatic Updates' build tests,
-    // since those do give us control over the filesystem permissions.
-    // @see \Drupal\Tests\auto_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
-    // @see \Drupal\Tests\package_manager\Kernel\WritableFileSystemValidatorTest
-    $container->removeDefinition('package_manager.validator.file_system');
+    foreach ($this->disableValidators as $service_id) {
+      if ($container->hasDefinition($service_id)) {
+        $container->getDefinition($service_id)->clearTag('event_subscriber');
+      }
+    }
   }
 
   /**

core/modules/package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php

@@ -21,6 +21,14 @@
  */
 class WritableFileSystemValidatorTest extends PackageManagerKernelTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $disableValidators = [
+    // The parent class disables the validator we're testing, so prevent that
+    // here with an empty array.
+  ];
+
   /**
    * {@inheritdoc}
    */
@@ -33,14 +41,6 @@ public function register(ContainerBuilder $container) {
       ->setClass(TestWritableFileSystemValidator::class);
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function disableValidators(ContainerBuilder $container): void {
-    // The parent method disables the validator we're testing, so we don't want
-    // to do anything here.
-  }
-
   /**
    * Data provider for ::testWritable().
    *