Issue #3321684: Should most validators that subscribe to PreCreate check readiness to update also subscribe to PreApply?

tests/src/Kernel/StatusCheck/CronServerValidatorTest.php

@@ -13,6 +13,7 @@ use Drupal\package_manager\Exception\StageValidationException;
 use Drupal\package_manager\ValidationResult;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
 use ColinODell\PsrTestLogger\TestLogger;
+use Drupal\Tests\package_manager\Traits\PackageManagerBypassTestTrait;
 
 /**
  * @covers \Drupal\automatic_updates\Validator\CronServerValidator
@@ -21,6 +22,8 @@ use ColinODell\PsrTestLogger\TestLogger;
  */
 class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
 
+  use PackageManagerBypassTestTrait;
+
   /**
    * {@inheritdoc}
    */
@@ -72,7 +75,7 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
   }
 
   /**
-   * Tests server validation for unattended updates.
+   * Tests server validation during pre-create for unattended updates.
    *
    * @param bool $alternate_port
    *   Whether or not an alternate port should be set.
@@ -88,7 +91,7 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
    *
    * @dataProvider providerCronServerValidation
    */
-  public function testCronServerValidation(bool $alternate_port, string $server_api, array $cron_modes, array $expected_results): void {
+  public function testCronServerValidationDuringPreCreate(bool $alternate_port, string $server_api, array $cron_modes, array $expected_results): void {
     $request = $this->container->get('request_stack')->getCurrentRequest();
     $this->assertNotEmpty($request);
     $this->assertSame(80, $request->getPort());
@@ -113,6 +116,9 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
       // If errors were expected, cron should not have run.
       $this->container->get('cron')->run();
       if ($expected_results) {
+        // Assert the update was not staged to ensure the error was flagged in
+        // PreCreateEvent and not PreApplyEvent.
+        $this->assertUpdateStagedTimes(0);
         $error = new StageValidationException($expected_results);
         $this->assertTrue($logger->hasRecord($error->getMessage(), (string) RfcLogLevel::ERROR));
       }
@@ -148,30 +154,45 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
     $this->container->get('logger.factory')
       ->get('automatic_updates')
       ->addLogger($logger);
+    $original_port = $this->config('automatic_updates.settings')
+      ->get('cron_port');
+    $property = new \ReflectionProperty(CronServerValidator::class, 'serverApi');
+    $property->setAccessible(TRUE);
+    $original_server_api = $property->getValue();
+
+    $this->container->get('event_dispatcher')->addListener(
+      PreApplyEvent::class,
+      function () use ($alternate_port, $server_api): void {
+        $property = new \ReflectionProperty(CronServerValidator::class, 'serverApi');
+        $property->setAccessible(TRUE);
+        $property->setValue(NULL, $server_api);
+        $this->config('automatic_updates.settings')
+          ->set('cron_port', $alternate_port ? 2501 : 0)
+          ->save();
+      },
+      PHP_INT_MAX
+    );
+    $cron_staged_count = 0;
 
     foreach ($cron_modes as $mode) {
-      // As the listener listens to pre-apply event we need to set the cron mode
-      // outside the listener because that time cron is running, and it cannot
-      // override cron mode.
+      // Because we are running cron multiple times before each cron run we need
+      // to reset the server API and cron port settings to their original
+      // settings, otherwise the settings change in the previous cron run's
+      // PreApplyEvent would still be set.
+      $property->setValue(NULL, $original_server_api);
+      $this->config('automatic_updates.settings')
+        ->set('cron_port', $original_port)
+        ->save();
       $this->config('automatic_updates.settings')
         ->set('cron', $mode)
         ->save();
-      $this->container->get('event_dispatcher')->addListener(
-        PreApplyEvent::class,
-        function () use ($mode, $alternate_port, $server_api): void {
-          $property = new \ReflectionProperty(CronServerValidator::class, 'serverApi');
-          $property->setAccessible(TRUE);
-          $property->setValue(NULL, $server_api);
-          $this->config('automatic_updates.settings')
-            ->set('cron_port', $alternate_port ? 2501 : 0)
-            ->save();
-        },
-        PHP_INT_MAX
-      );
-
       // If errors were expected, cron should not have run.
       $this->container->get('cron')->run();
       if ($expected_results) {
+        $cron_staged_count++;
+        // Assert that update was staged to ensure the error was not flagged
+        // in PreCreateEvent.
+        $this->assertUpdateStagedTimes($cron_staged_count);
         $error = new StageValidationException($expected_results);
         $this->assertTrue($logger->hasRecord($error->getMessage(), (string) RfcLogLevel::ERROR));
       }
@@ -213,7 +234,7 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
       }
       $expected_results[$i] = ValidationResult::createError($messages);
     }
-    $this->testCronServerValidation($alternate_port, $server_api, $cron_modes, $expected_results);
+    $this->testCronServerValidationDuringPreApply($alternate_port, $server_api, $cron_modes, $expected_results);
   }
 
 }