Issue #3321206 by omkar.podey, tedbow, Wim Leers: The same xdebug warning...

Issue #3321206 by omkar.podey, tedbow, Wim Leers: The same xdebug warning shows on the status report 2 times

automatic_updates.services.yml

@@ -90,12 +90,6 @@ services:
       - '@string_translation'
     tags:
       - { name: event_subscriber }
-  automatic_updates.validator.xdebug:
-    class: Drupal\automatic_updates\Validator\XdebugValidator
-    arguments:
-      - '@package_manager.validator.xdebug'
-    tags:
-      - { name: event_subscriber }
   automatic_updates.validator.version_policy:
     class: Drupal\automatic_updates\Validator\VersionPolicyValidator
     arguments:

package_manager/src/Validator/XdebugValidator.php

@@ -16,7 +16,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
  *   at any time without warning. External code should not interact with this
  *   class.
  */
-final class XdebugValidator implements EventSubscriberInterface {
+class XdebugValidator implements EventSubscriberInterface {
 
   use StringTranslationTrait;
 
@@ -26,13 +26,26 @@ final class XdebugValidator implements EventSubscriberInterface {
    * @param \Drupal\package_manager\Event\StatusCheckEvent $event
    *   The event object.
    */
-  public function checkForXdebug(StatusCheckEvent $event): void {
+  public function validateXdebugOff(StatusCheckEvent $event): void {
+    $warning = $this->checkForXdebug();
+    if ($warning) {
+      $event->addWarning($warning);
+    }
+  }
+
+  /**
+   * Checks if Xdebug is enabled and returns a warning if it is.
+   *
+   * @return array|null
+   *   Returns an array of warnings or null if Xdebug isn't detected.
+   */
+  protected function checkForXdebug(): ?array {
     if (function_exists('xdebug_break')) {
-      $messages = [
+      return [
         $this->t('Xdebug is enabled, which may have a negative performance impact on Package Manager and any modules that use it.'),
       ];
-      $event->addWarning($messages);
     }
+    return NULL;
   }
 
   /**
@@ -40,7 +53,7 @@ final class XdebugValidator implements EventSubscriberInterface {
    */
   public static function getSubscribedEvents(): array {
     return [
-      StatusCheckEvent::class => 'checkForXdebug',
+      StatusCheckEvent::class => 'validateXdebugOff',
     ];
   }
 

src/AutomaticUpdatesServiceProvider.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\automatic_updates;
+
+use Drupal\automatic_updates\Validator\XdebugValidator;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\DependencyInjection\ServiceProviderBase;
+
+/**
+ * Modifies container services for Automatic Updates.
+ */
+class AutomaticUpdatesServiceProvider extends ServiceProviderBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function alter(ContainerBuilder $container) {
+    $service_id = 'package_manager.validator.xdebug';
+    if ($container->hasDefinition($service_id)) {
+      $container->getDefinition($service_id)
+        ->setClass(XdebugValidator::class);
+    }
+  }
+
+}

src/Validator/XdebugValidator.php

@@ -6,7 +6,6 @@ namespace Drupal\automatic_updates\Validator;
 
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 use Drupal\automatic_updates\CronUpdater;
-use Drupal\automatic_updates\Updater;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreOperationStageEvent;
 use Drupal\package_manager\Event\StatusCheckEvent;
@@ -20,24 +19,7 @@ use Drupal\package_manager\Validator\XdebugValidator as PackageManagerXdebugVali
  *   at any time without warning. External code should not interact with this
  *   class.
  */
-final class XdebugValidator implements EventSubscriberInterface {
-
-  /**
-   * The Package Manager validator we're wrapping.
-   *
-   * @var \Drupal\package_manager\Validator\XdebugValidator
-   */
-  private $packageManagerValidator;
-
-  /**
-   * Constructs an XdebugValidator object.
-   *
-   * @param \Drupal\package_manager\Validator\XdebugValidator $package_manager_validator
-   *   The Package Manager validator we're wrapping.
-   */
-  public function __construct(PackageManagerXdebugValidator $package_manager_validator) {
-    $this->packageManagerValidator = $package_manager_validator;
-  }
+final class XdebugValidator extends PackageManagerXdebugValidator implements EventSubscriberInterface {
 
   /**
    * Performs validation if Xdebug is enabled.
@@ -45,31 +27,17 @@ final class XdebugValidator implements EventSubscriberInterface {
    * @param \Drupal\package_manager\Event\PreOperationStageEvent $event
    *   The event object.
    */
-  public function checkForXdebug(PreOperationStageEvent $event): void {
+  public function validateXdebugOff(PreOperationStageEvent $event): void {
     $stage = $event->getStage();
+    $warning = $this->checkForXdebug();
 
-    // We only want to do this check if the stage belongs to Automatic Updates.
-    if (!($stage instanceof Updater)) {
-      return;
-    }
-
-    $status_check = new StatusCheckEvent($stage, []);
-    $this->packageManagerValidator->checkForXdebug($status_check);
-    $results = $status_check->getResults();
-    if (empty($results)) {
-      return;
-    }
-    elseif ($stage instanceof CronUpdater) {
-      // Cron updates are not allowed if Xdebug is enabled.
-      foreach ($results as $result) {
-        $event->addError($result->getMessages(), $result->getSummary());
+    if ($warning) {
+      if ($stage instanceof CronUpdater) {
+        // Cron updates are not allowed if Xdebug is enabled.
+        $event->addError([$this->t("Xdebug is enabled, currently Cron Updates are not allowed while it is enabled. If Xdebug is not disabled you will not receive security and other updates during cron.")]);
       }
-    }
-    elseif ($event instanceof StatusCheckEvent) {
-      // For non-cron updates provide a warning but do not stop updates from
-      // executing.
-      foreach ($results as $result) {
-        $event->addWarning($result->getMessages(), $result->getSummary());
+      elseif ($event instanceof StatusCheckEvent) {
+        $event->addWarning($warning);
       }
     }
   }
@@ -79,8 +47,8 @@ final class XdebugValidator implements EventSubscriberInterface {
    */
   public static function getSubscribedEvents(): array {
     return [
-      PreCreateEvent::class => 'checkForXdebug',
-      StatusCheckEvent::class => 'checkForXdebug',
+      PreCreateEvent::class => 'validateXdebugOff',
+      StatusCheckEvent::class => 'validateXdebugOff',
     ];
   }
 

tests/src/Functional/AutomaticUpdatesFunctionalTestBase.php

@@ -60,7 +60,6 @@ abstract class AutomaticUpdatesFunctionalTestBase extends BrowserTestBase {
     'automatic_updates.composer_executable_validator',
     'package_manager.validator.composer_executable',
     // Always allow tests to run with Xdebug on.
-    'automatic_updates.validator.xdebug',
     'package_manager.validator.xdebug',
   ];
 

tests/src/Kernel/AutomaticUpdatesKernelTestBase.php

@@ -51,7 +51,6 @@ abstract class AutomaticUpdatesKernelTestBase extends PackageManagerKernelTestBa
       $this->disableValidators[] = 'automatic_updates.validator.symlink';
     }
     // Always disable the Xdebug validator to allow test to run with Xdebug on.
-    $this->disableValidators[] = 'automatic_updates.validator.xdebug';
     $this->disableValidators[] = 'package_manager.validator.xdebug';
     parent::setUp();
     // Enable cron updates, which will eventually be the default.

tests/src/Kernel/StatusCheck/XdebugValidatorTest.php

@@ -7,6 +7,7 @@ namespace Drupal\Tests\automatic_updates\Kernel\StatusCheck;
 use Drupal\automatic_updates\CronUpdater;
 use Drupal\Core\Logger\RfcLogLevel;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\package_manager\StatusCheckTrait;
 use Drupal\package_manager\ValidationResult;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
 use Drupal\Tests\package_manager\Traits\PackageManagerBypassTestTrait;
@@ -21,6 +22,7 @@ class XdebugValidatorTest extends AutomaticUpdatesKernelTestBase {
 
   use PackageManagerBypassTestTrait;
   use StringTranslationTrait;
+  use StatusCheckTrait;
 
   /**
    * {@inheritdoc}
@@ -40,7 +42,7 @@ class XdebugValidatorTest extends AutomaticUpdatesKernelTestBase {
 
     // The parent class unconditionally disables the Xdebug validator we're
     // testing, so undo that here.
-    $validator = $this->container->get('automatic_updates.validator.xdebug');
+    $validator = $this->container->get('package_manager.validator.xdebug');
     $this->container->get('event_dispatcher')->addSubscriber($validator);
   }
 
@@ -48,12 +50,26 @@ class XdebugValidatorTest extends AutomaticUpdatesKernelTestBase {
    * Tests warnings and/or errors if Xdebug is enabled.
    */
   public function testXdebugValidation(): void {
-    $message = 'Xdebug is enabled, which may have a negative performance impact on Package Manager and any modules that use it.';
+    $message = $this->t('Xdebug is enabled, which may have a negative performance impact on Package Manager and any modules that use it.');
+    $error = $this->t("Xdebug is enabled, currently Cron Updates are not allowed while it is enabled. If Xdebug is not disabled you will not receive security and other updates during cron.");
 
     $config = $this->config('automatic_updates.settings');
     // If cron updates are disabled, the status check message should only be
     // a warning.
     $config->set('cron', CronUpdater::DISABLED)->save();
+
+    // Tests that other projects which depend on Package manager also get the
+    // warning.
+    $stage = $this->createStage();
+    $this->assertUpdateStagedTimes(0);
+    $stage->create();
+    $stage->require(['drupal/random']);
+    $this->assertUpdateStagedTimes(1);
+    $event_dispatcher = \Drupal::service('event_dispatcher');
+    $result = $this->runStatusCheck($stage, $event_dispatcher, TRUE);
+    $this->assertSame($message->getUntranslatedString(), $result[0]->getMessages()[0]->getUntranslatedString());
+    $stage->destroy(TRUE);
+
     $result = ValidationResult::createWarning([$message]);
     $this->assertCheckerResultsFromManager([$result], TRUE);
 
@@ -63,7 +79,7 @@ class XdebugValidatorTest extends AutomaticUpdatesKernelTestBase {
 
     // If cron updates are enabled the status check message should be an
     // error.
-    $result = ValidationResult::createError([$message]);
+    $result = ValidationResult::createError([$error]);
     $this->assertCheckerResultsFromManager([$result], TRUE);
 
     // Trying to do the update during cron should fail with an error.
@@ -73,8 +89,9 @@ class XdebugValidatorTest extends AutomaticUpdatesKernelTestBase {
       ->addLogger($logger);
 
     $this->container->get('cron')->run();
-    $this->assertUpdateStagedTimes(0);
-    $this->assertTrue($logger->hasRecordThatMatches("/$message/", (string) RfcLogLevel::ERROR));
+    // Assert there was not another update staged during cron.
+    $this->assertUpdateStagedTimes(1);
+    $this->assertTrue($logger->hasRecordThatMatches("/$error/", (string) RfcLogLevel::ERROR));
   }
 
 }