Issue #3258048 by phenaproxima: Ensure the stage is marked as available even...

Issue #3258048 by phenaproxima: Ensure the stage is marked as available even if all stage directories can't be deleted

package_manager/src/Stage.php

@@ -4,6 +4,7 @@ namespace Drupal\package_manager;
 
 use Drupal\Component\FileSystem\FileSystem;
 use Drupal\Component\Utility\Crypt;
+use Drupal\Core\File\Exception\FileException;
 use Drupal\Core\File\FileSystemInterface;
 use Drupal\Core\TempStore\SharedTempStoreFactory;
 use Drupal\package_manager\Event\PostApplyEvent;
@@ -290,12 +291,17 @@ class Stage {
     // Delete all directories in parent staging directory.
     $parent_stage_dir = static::getStagingRoot();
     if (is_dir($parent_stage_dir)) {
-      // @todo Ensure that even if attempting to delete this directory throws an
-      //   exception the stage is still marked as available in
-      //   https://www.drupal.org/i/3258048.
-      $this->fileSystem->deleteRecursive($parent_stage_dir, function (string $path): void {
-        $this->fileSystem->chmod($path, 0777);
-      });
+      try {
+        $this->fileSystem->deleteRecursive($parent_stage_dir, function (string $path): void {
+          $this->fileSystem->chmod($path, 0777);
+        });
+      }
+      catch (FileException $e) {
+        // Deliberately swallow the exception so that the stage will be marked
+        // as available and the post-destroy event will be fired, even if the
+        // staging area can't actually be deleted. The file system service logs
+        // the exception, so we don't need to do anything else here.
+      }
     }
     $this->markAsAvailable();
     $this->dispatch(new PostDestroyEvent($this));

package_manager/tests/modules/package_manager_bypass/src/PackageManagerBypassServiceProvider.php

@@ -4,7 +4,6 @@ namespace Drupal\package_manager_bypass;
 
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Drupal\Core\DependencyInjection\ServiceProviderBase;
-use Drupal\Core\Site\Settings;
 use Symfony\Component\DependencyInjection\Reference;
 
 /**
@@ -18,30 +17,19 @@ class PackageManagerBypassServiceProvider extends ServiceProviderBase {
   public function alter(ContainerBuilder $container) {
     parent::alter($container);
 
-    // By default, bypass the Composer Stager library. This can be disabled for
-    // tests that want to use the real library, but only need to disable
-    // validators.
-    if (Settings::get('package_manager_bypass_stager', TRUE)) {
-      $container->getDefinition('package_manager.beginner')
-        ->setClass(Beginner::class);
-      $container->getDefinition('package_manager.stager')
-        ->setClass(Stager::class);
-
-      $container->register('package_manager_bypass.committer')
-        ->setClass(Committer::class)
-        ->setPublic(FALSE)
-        ->setDecoratedService('package_manager.committer')
-        ->setArguments([
-          new Reference('package_manager_bypass.committer.inner'),
-        ])
-        ->setProperty('_serviceId', 'package_manager.committer');
-    }
-
-    // Allow functional tests to disable specific validators as necessary.
-    // Kernel tests can override the ::register() method and modify the
-    // container directly.
-    $validators = Settings::get('package_manager_bypass_validators', []);
-    array_walk($validators, [$container, 'removeDefinition']);
+    $container->getDefinition('package_manager.beginner')
+      ->setClass(Beginner::class);
+    $container->getDefinition('package_manager.stager')
+      ->setClass(Stager::class);
+
+    $container->register('package_manager_bypass.committer')
+      ->setClass(Committer::class)
+      ->setPublic(FALSE)
+      ->setDecoratedService('package_manager.committer')
+      ->setArguments([
+        new Reference('package_manager_bypass.committer.inner'),
+      ])
+      ->setProperty('_serviceId', 'package_manager.committer');
   }
 
 }

package_manager/tests/src/Kernel/ExcludedPathsTest.php

@@ -26,18 +26,17 @@ class ExcludedPathsTest extends PackageManagerKernelTestBase {
   protected function setUp(): void {
     parent::setUp();
 
-    // Normally, package_manager_bypass will disable all the actual staging
-    // operations. In this case, we want to perform them so that we can be sure
-    // that files are staged as expected.
-    $this->setSetting('package_manager_bypass_stager', FALSE);
     // The private stream wrapper is only registered if this setting is set.
     // @see \Drupal\Core\CoreServiceProvider::register()
     $this->setSetting('file_private_path', 'private');
-
-    // Rebuild the container to make the new settings take effect.
-    $kernel = $this->container->get('kernel');
-    $kernel->rebuildContainer();
-    $this->container = $kernel->getContainer();
+    // In this test, we want to perform the actual staging operations so that we
+    // can be sure that files are staged as expected. This will also rebuild
+    // the container, enabling the private stream wrapper.
+    $this->container->get('module_installer')->uninstall([
+      'package_manager_bypass',
+    ]);
+    // Ensure we have an up-to-date container.
+    $this->container = $this->container->get('kernel')->getContainer();
 
     // Mock a SQLite database connection so we can test that the subscriber will
     // exclude the database files.

package_manager/tests/src/Kernel/StageOwnershipTest.php

@@ -2,11 +2,16 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\Core\File\FileSystem;
+use Drupal\Core\Logger\RfcLogLevel;
+use Drupal\package_manager\Event\PostDestroyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Exception\StageException;
 use Drupal\package_manager\Exception\StageOwnershipException;
 use Drupal\package_manager_test_validation\TestSubscriber;
 use Drupal\Tests\user\Traits\UserCreationTrait;
+use Prophecy\Argument;
+use Psr\Log\LoggerInterface;
 
 /**
  * Tests that ownership of the stage is enforced.
@@ -32,6 +37,7 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
   protected function setUp(): void {
     parent::setUp();
     $this->installSchema('system', ['sequences']);
+    $this->installSchema('user', ['users_data']);
     $this->installEntitySchema('user');
     $this->registerPostUpdateFunctions();
   }
@@ -221,4 +227,70 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
     $not_owned->destroy(TRUE);
   }
 
+  /**
+   * Tests that the stage is available if ::destroy() has a file system error.
+   */
+  public function testStageDestroyedWithFileSystemError(): void {
+    // Enable the Composer Stager library, since we will actually want to create
+    // the stage directory.
+    $this->container->get('module_installer')->uninstall([
+      'package_manager_bypass',
+    ]);
+    // Ensure we have an up-to-date container.
+    $this->container = $this->container->get('kernel')->getContainer();
+    $this->createTestProject();
+
+    $logger_channel = $this->container->get('logger.channel.file');
+    $arguments = [
+      $this->container->get('stream_wrapper_manager'),
+      $this->container->get('settings'),
+      $logger_channel,
+    ];
+    $this->container->set('file_system', new class (...$arguments) extends FileSystem {
+
+      /**
+       * {@inheritdoc}
+       */
+      public function chmod($uri, $mode = NULL) {
+        // Normally, the stage will call this method as it tries to make
+        // everything in the staging area writable so it can be deleted. We
+        // don't wan't to do that in this test, since we're specifically testing
+        // what happens when we try to delete a staging area with unwritable
+        // files.
+      }
+
+    });
+
+    $stage = $this->createStage();
+    $this->assertTrue($stage->isAvailable());
+    $stage->create();
+    $this->assertFalse($stage->isAvailable());
+
+    // Make the stage directory unwritable, which should prevent files in it
+    // from being deleted.
+    $dir = $stage->getStageDirectory();
+    chmod($dir, 0400);
+    $this->assertDirectoryIsNotWritable($dir);
+
+    // Mock a logger to prove that a file system error was raised while trying
+    // to delete the stage directory.
+    $logger = $this->prophesize(LoggerInterface::class);
+    $logger->log(
+      RfcLogLevel::ERROR,
+      "Failed to unlink file '%path'.",
+      Argument::withEntry('%path', "$dir/composer.json")
+    )->shouldBeCalled();
+    $logger_channel->addLogger($logger->reveal());
+
+    // Listen to the post-destroy event so we can confirm that it was fired, and
+    // the stage was made available, despite the file system error.
+    $stage_available = NULL;
+    $this->container->get('event_dispatcher')
+      ->addListener(PostDestroyEvent::class, function (PostDestroyEvent $event) use (&$stage_available): void {
+        $stage_available = $event->getStage()->isAvailable();
+      });
+    $stage->destroy();
+    $this->assertTrue($stage_available);
+  }
+
 }