Issue #3246695 by phenaproxima, tedbow: Move WritableFilesystemValidator into Package Manager

automatic_updates.services.yml

@@ -65,11 +65,9 @@ services:
     tags:
       - { name: event_subscriber }
   automatic_updates.validator.file_system_permissions:
-    class: Drupal\automatic_updates\Validator\WritableFileSystemValidator
+    class: Drupal\automatic_updates\Validator\PackageManagerReadinessCheck
     arguments:
-      - '@package_manager.path_locator'
-      - '%app.root%'
-      - '@string_translation'
+      - '@package_manager.validator.file_system'
     tags:
       - { name: event_subscriber }
   automatic_updates.validator.core_composer:

package_manager/package_manager.services.yml

@@ -108,3 +108,11 @@ services:
       - '@string_translation'
     tags:
       - { name: event_subscriber }
+  package_manager.validator.file_system:
+    class: Drupal\package_manager\EventSubscriber\WritableFileSystemValidator
+    arguments:
+      - '@package_manager.path_locator'
+      - '%app.root%'
+      - '@string_translation'
+    tags:
+      - { name: event_subscriber }

src/Validator/WritableFileSystemValidator.php → package_manager/src/EventSubscriber/WritableFileSystemValidator.php

 <?php
 
-namespace Drupal\automatic_updates\Validator;
+namespace Drupal\package_manager\EventSubscriber;
 
-use Drupal\automatic_updates\Event\ReadinessCheckEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\StageEvent;
 use Drupal\package_manager\ValidationResult;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\package_manager\PathLocator;
-use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
  * Checks that the file system is writable.
  */
-class WritableFileSystemValidator implements EventSubscriberInterface {
+class WritableFileSystemValidator implements StageValidatorInterface {
 
   use StringTranslationTrait;
 
@@ -49,17 +47,14 @@ class WritableFileSystemValidator implements EventSubscriberInterface {
   }
 
   /**
-   * Checks that the file system is writable.
-   *
-   * @param \Drupal\package_manager\Event\StageEvent $event
-   *   The event object.
+   * {@inheritdoc}
    *
    * @todo It might make sense to use a more sophisticated method of testing
    *   writability than is_writable(), since it's not clear if that can return
    *   false negatives/positives due to things like SELinux, exotic file
    *   systems, and so forth.
    */
-  public function checkPermissions(StageEvent $event): void {
+  public function validateStage(StageEvent $event): void {
     $messages = [];
 
     if (!is_writable($this->appRoot)) {
@@ -84,8 +79,7 @@ class WritableFileSystemValidator implements EventSubscriberInterface {
    */
   public static function getSubscribedEvents() {
     return [
-      ReadinessCheckEvent::class => 'checkPermissions',
-      PreCreateEvent::class => 'checkPermissions',
+      PreCreateEvent::class => 'validateStage',
     ];
   }
 

package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\KernelTests\KernelTestBase;
 use Drupal\package_manager\Event\StageEvent;
 use Drupal\package_manager\Stage;
@@ -23,6 +24,27 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
     'package_manager_bypass',
   ];
 
+  /**
+   * {@inheritdoc}
+   */
+  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\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
+    // @see \Drupal\Tests\package_manager\Kernel\WritableFileSystemValidatorTest
+    $container->removeDefinition('package_manager.validator.file_system');
+  }
+
   /**
    * Asserts validation results are returned from a stage life cycle event.
    *

package_manager/tests/src/Kernel/StageEventsTest.php

@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
-use Drupal\KernelTests\KernelTestBase;
 use Drupal\package_manager\Event\PostApplyEvent;
 use Drupal\package_manager\Event\PostCreateEvent;
 use Drupal\package_manager\Event\PostDestroyEvent;
@@ -24,15 +23,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
  *
  * @group package_manager
  */
-class StageEventsTest extends KernelTestBase implements EventSubscriberInterface {
-
-  /**
-   * {@inheritdoc}
-   */
-  protected static $modules = [
-    'package_manager',
-    'package_manager_bypass',
-  ];
+class StageEventsTest extends PackageManagerKernelTestBase implements EventSubscriberInterface {
 
   /**
    * The events that were fired, in the order they were fired.

tests/src/Kernel/ReadinessValidation/WritableFileSystemValidatorTest.php → package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php

 <?php
 
-namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
+namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\package_manager\Event\PreCreateEvent;
+use Drupal\package_manager\EventSubscriber\WritableFileSystemValidator;
 use Drupal\package_manager\ValidationResult;
-use Drupal\automatic_updates\Validator\WritableFileSystemValidator;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\package_manager\PathLocator;
-use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
 use org\bovigo\vfs\vfsStream;
 
 /**
  * Unit tests the file system permissions validator.
  *
- * This validator is tested functionally in our build tests, since those give
- * us control over the file system permissions.
+ * This validator is tested functionally in Automatic Updates' build tests,
+ * since those give us control over the file system permissions.
  *
  * @see \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
  *
- * @covers \Drupal\automatic_updates\Validator\WritableFileSystemValidator
+ * @covers \Drupal\package_manager\EventSubscriber\WritableFileSystemValidator
  *
- * @group automatic_updates
+ * @group package_manager
  */
-class WritableFileSystemValidatorTest extends AutomaticUpdatesKernelTestBase {
+class WritableFileSystemValidatorTest extends PackageManagerKernelTestBase {
 
   /**
    * {@inheritdoc}
    */
-  protected static $modules = [
-    'automatic_updates',
-    'package_manager',
-  ];
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
+
+    // Replace the file system permissions validator with our test-only
+    // implementation.
+    $container->getDefinition('package_manager.validator.file_system')
+      ->setClass(TestValidator::class);
+  }
 
   /**
    * {@inheritdoc}
    */
   protected function disableValidators(ContainerBuilder $container): void {
-    // No need to disable any validators in this test.
+    // Disable the disk space validator, since it tries to inspect the file
+    // system in ways that vfsStream doesn't support, like calling stat() and
+    // disk_free_space().
+    $container->removeDefinition('package_manager.validator.disk_space');
   }
 
   /**
@@ -94,19 +101,34 @@ class WritableFileSystemValidatorTest extends AutomaticUpdatesKernelTestBase {
    * @dataProvider providerWritable
    */
   public function testWritable(int $root_permissions, int $vendor_permissions, array $expected_results): void {
-    $files = vfsStream::setup('root', $root_permissions);
-    $files->addChild(vfsStream::newDirectory('vendor', $vendor_permissions));
+    $root = vfsStream::setup('root', $root_permissions);
+    $vendor = vfsStream::newDirectory('vendor', $vendor_permissions);
+    $root->addChild($vendor);
 
     $path_locator = $this->prophesize(PathLocator::class);
-    $path_locator->getVendorDirectory()->willReturn(vfsStream::url('root/vendor'));
-
-    $validator = new WritableFileSystemValidator(
-      $path_locator->reveal(),
-      vfsStream::url('root'),
-      $this->container->get('string_translation')
-    );
-    $this->container->set('automatic_updates.validator.file_system_permissions', $validator);
-    $this->assertCheckerResultsFromManager($expected_results, TRUE);
+    $path_locator->getActiveDirectory()->willReturn($root->url());
+    $path_locator->getStageDirectory()->willReturn('/fake/stage/dir');
+    $path_locator->getWebRoot()->willReturn('');
+    $path_locator->getVendorDirectory()->willReturn($vendor->url());
+    $this->container->set('package_manager.path_locator', $path_locator->reveal());
+
+    /** @var \Drupal\Tests\package_manager\Kernel\TestValidator $validator */
+    $validator = $this->container->get('package_manager.validator.file_system');
+    $validator->appRoot = $root->url();
+
+    $this->assertResults($expected_results, PreCreateEvent::class);
   }
 
 }
+
+/**
+ * A test version of the file system permissions validator.
+ */
+class TestValidator extends WritableFileSystemValidator {
+
+  /**
+   * {@inheritdoc}
+   */
+  public $appRoot;
+
+}

tests/src/Functional/AutomaticUpdatesFunctionalTestBase.php

@@ -30,7 +30,10 @@ abstract class AutomaticUpdatesFunctionalTestBase extends BrowserTestBase {
     // 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'],
+      'value' => [
+        'automatic_updates.validator.file_system_permissions',
+        'package_manager.validator.file_system',
+      ],
       'required' => TRUE,
     ];
     $this->writeSettings($settings);

tests/src/Kernel/AutomaticUpdatesKernelTestBase.php

@@ -99,6 +99,7 @@ abstract class AutomaticUpdatesKernelTestBase extends KernelTestBase {
     // give us control over the filesystem permissions.
     // @see \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::assertReadOnlyFileSystemError()
     $container->removeDefinition('automatic_updates.validator.file_system_permissions');
+    $container->removeDefinition('package_manager.validator.file_system');
   }
 
   /**

tests/src/Kernel/ReadinessValidation/PackageManagerReadinessChecksTest.php

@@ -3,6 +3,7 @@
 namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\package_manager\EventSubscriber\StageValidatorInterface;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
 use Prophecy\Argument;
@@ -17,6 +18,7 @@ use Prophecy\Argument;
  * @see \Drupal\Tests\package_manager\Kernel\ComposerExecutableValidatorTest
  * @see \Drupal\Tests\package_manager\Kernel\DiskSpaceValidatorTest
  * @see \Drupal\Tests\package_manager\Kernel\PendingUpdatesValidatorTest
+ * @see \Drupal\Tests\package_manager\Kernel\WritableFileSystemValidatorTest
  */
 class PackageManagerReadinessChecksTest extends AutomaticUpdatesKernelTestBase {
 
@@ -28,6 +30,13 @@ class PackageManagerReadinessChecksTest extends AutomaticUpdatesKernelTestBase {
     'package_manager',
   ];
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function disableValidators(ContainerBuilder $container): void {
+    // No need to disable any validators in this test.
+  }
+
   /**
    * Data provider for ::testValidatorInvoked().
    *
@@ -39,6 +48,7 @@ class PackageManagerReadinessChecksTest extends AutomaticUpdatesKernelTestBase {
       ['package_manager.validator.composer_executable'],
       ['package_manager.validator.disk_space'],
       ['package_manager.validator.pending_updates'],
+      ['package_manager.validator.file_system'],
     ];
   }