From 27bee216d558b35de4de84c3e198c54e132d7a42 Mon Sep 17 00:00:00 2001
From: phenaproxima <phenaproxima@205645.no-reply.drupal.org>
Date: Wed, 12 Jan 2022 19:25:56 +0000
Subject: [PATCH] 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 | 18 +++--
.../PackageManagerBypassServiceProvider.php | 38 ++++------
.../tests/src/Kernel/ExcludedPathsTest.php | 17 +++--
.../tests/src/Kernel/StageOwnershipTest.php | 72 +++++++++++++++++++
4 files changed, 105 insertions(+), 40 deletions(-)
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index 6d46783830..711da3a019 100644
--- a/package_manager/src/Stage.php
+++ b/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));
diff --git a/package_manager/tests/modules/package_manager_bypass/src/PackageManagerBypassServiceProvider.php b/package_manager/tests/modules/package_manager_bypass/src/PackageManagerBypassServiceProvider.php
index 3394cf4e15..3f7bf47d38 100644
--- a/package_manager/tests/modules/package_manager_bypass/src/PackageManagerBypassServiceProvider.php
+++ b/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');
}
}
diff --git a/package_manager/tests/src/Kernel/ExcludedPathsTest.php b/package_manager/tests/src/Kernel/ExcludedPathsTest.php
index 776c0ff9a0..33dd8f5e16 100644
--- a/package_manager/tests/src/Kernel/ExcludedPathsTest.php
+++ b/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.
diff --git a/package_manager/tests/src/Kernel/StageOwnershipTest.php b/package_manager/tests/src/Kernel/StageOwnershipTest.php
index 99339a9696..a5c76c7b7a 100644
--- a/package_manager/tests/src/Kernel/StageOwnershipTest.php
+++ b/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);
+ }
+
}
--
GitLab