From a212e6e3f2ead77a53add9367de765501434e0be Mon Sep 17 00:00:00 2001
From: tedbow <tedbow@240860.no-reply.drupal.org>
Date: Wed, 1 Dec 2021 16:03:49 +0000
Subject: [PATCH] Issue #3250696 by tedbow, phenaproxima: Each stage should
have its own staging directory, to avoid filesystem and locking conflicts
---
automatic_updates.services.yml | 2 +-
package_manager/package_manager.services.yml | 6 --
package_manager/src/Cleaner.php | 78 -------------------
package_manager/src/PathLocator.php | 3 +-
package_manager/src/Stage.php | 51 ++++++++----
.../src/Functional/ExcludedPathsTest.php | 8 +-
.../Kernel/PackageManagerKernelTestBase.php | 2 +-
.../tests/src/Kernel/ServicesTest.php | 1 -
.../tests/src/Kernel/StageEventsTest.php | 2 +-
.../Functional/FileSystemOperationsTest.php | 36 ++++-----
.../StagedProjectsValidatorTest.php | 29 ++++++-
11 files changed, 85 insertions(+), 133 deletions(-)
delete mode 100644 package_manager/src/Cleaner.php
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index 4ad9a1001c..b54402216e 100644
--- a/automatic_updates.services.yml
+++ b/automatic_updates.services.yml
@@ -15,7 +15,7 @@ services:
- '@package_manager.beginner'
- '@package_manager.stager'
- '@package_manager.committer'
- - '@package_manager.cleaner'
+ - '@file_system'
- '@event_dispatcher'
- '@tempstore.shared'
automatic_updates.excluded_paths_subscriber:
diff --git a/package_manager/package_manager.services.yml b/package_manager/package_manager.services.yml
index 0ffa5aa01a..cc7e58d625 100644
--- a/package_manager/package_manager.services.yml
+++ b/package_manager/package_manager.services.yml
@@ -74,12 +74,6 @@ services:
arguments:
- '@package_manager.file_syncer'
- '@package_manager.file_system'
- package_manager.cleaner:
- class: Drupal\package_manager\Cleaner
- arguments:
- - '@package_manager.file_system'
- - '%site.path%'
- - '@package_manager.path_locator'
package_manager.path_locator:
class: Drupal\package_manager\PathLocator
arguments:
diff --git a/package_manager/src/Cleaner.php b/package_manager/src/Cleaner.php
deleted file mode 100644
index e920c259ae..0000000000
--- a/package_manager/src/Cleaner.php
+++ /dev/null
@@ -1,78 +0,0 @@
-<?php
-
-namespace Drupal\package_manager;
-
-use PhpTuf\ComposerStager\Domain\Cleaner as StagerCleaner;
-use PhpTuf\ComposerStager\Domain\CleanerInterface;
-use PhpTuf\ComposerStager\Domain\Output\ProcessOutputCallbackInterface;
-use PhpTuf\ComposerStager\Infrastructure\Filesystem\FilesystemInterface;
-use Symfony\Component\Filesystem\Filesystem;
-
-/**
- * Defines a cleaner service that makes the staged site directory writable.
- */
-class Cleaner implements CleanerInterface {
-
- /**
- * The decorated cleaner service.
- *
- * @var \PhpTuf\ComposerStager\Domain\CleanerInterface
- */
- protected $decorated;
-
- /**
- * The current site path, without leading or trailing slashes.
- *
- * @var string
- */
- protected $sitePath;
-
- /**
- * The path locator service.
- *
- * @var \Drupal\package_manager\PathLocator
- */
- protected $pathLocator;
-
- /**
- * Constructs a Cleaner object.
- *
- * @param \PhpTuf\ComposerStager\Infrastructure\Filesystem\FilesystemInterface $file_system
- * The file system service from Composer Stager.
- * @param string $site_path
- * The current site path (e.g., 'sites/default'), without leading or
- * trailing slashes.
- * @param \Drupal\package_manager\PathLocator $path_locator
- * The path locator service.
- */
- public function __construct(FilesystemInterface $file_system, string $site_path, PathLocator $path_locator) {
- $this->decorated = new StagerCleaner($file_system);
- $this->sitePath = $site_path;
- $this->pathLocator = $path_locator;
- }
-
- /**
- * {@inheritdoc}
- */
- public function clean(string $stagingDir, ?ProcessOutputCallbackInterface $callback = NULL, ?int $timeout = 120): void {
- // Ensure that the staged site directory is writable so we can delete it.
- $site_dir = implode(DIRECTORY_SEPARATOR, [
- $stagingDir,
- $this->pathLocator->getWebRoot() ?: '.',
- $this->sitePath,
- ]);
-
- if ($this->directoryExists($site_dir)) {
- (new Filesystem())->chmod($site_dir, 0777);
- }
- $this->decorated->clean($stagingDir, $callback, $timeout);
- }
-
- /**
- * {@inheritdoc}
- */
- public function directoryExists(string $stagingDir): bool {
- return $this->decorated->directoryExists($stagingDir);
- }
-
-}
diff --git a/package_manager/src/PathLocator.php b/package_manager/src/PathLocator.php
index ae28ee005c..982022e3e4 100644
--- a/package_manager/src/PathLocator.php
+++ b/package_manager/src/PathLocator.php
@@ -52,7 +52,8 @@ class PathLocator {
* Returns the path of the directory where changes should be staged.
*
* This directory may be made world-writeable for clean-up, so it should be
- * somewhere that doesn't put the Drupal installation at risk.
+ * somewhere that doesn't put the Drupal installation at risk. Each staging
+ * area will use a sub-directory with a random name.
*
* @return string
* The absolute path of the directory where changes should be staged.
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index 6f238597e9..5024089da6 100644
--- a/package_manager/src/Stage.php
+++ b/package_manager/src/Stage.php
@@ -3,6 +3,7 @@
namespace Drupal\package_manager;
use Drupal\Component\Utility\Crypt;
+use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\TempStore\SharedTempStoreFactory;
use Drupal\package_manager\Event\PostApplyEvent;
use Drupal\package_manager\Event\PostCreateEvent;
@@ -17,7 +18,6 @@ use Drupal\package_manager\Exception\StageException;
use Drupal\package_manager\Exception\StageOwnershipException;
use Drupal\package_manager\Exception\StageValidationException;
use PhpTuf\ComposerStager\Domain\BeginnerInterface;
-use PhpTuf\ComposerStager\Domain\CleanerInterface;
use PhpTuf\ComposerStager\Domain\CommitterInterface;
use PhpTuf\ComposerStager\Domain\StagerInterface;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
@@ -81,11 +81,11 @@ class Stage {
protected $committer;
/**
- * The cleaner service from Composer Stager.
+ * The file system service.
*
- * @var \PhpTuf\ComposerStager\Domain\CleanerInterface
+ * @var \Drupal\Core\File\FileSystemInterface
*/
- protected $cleaner;
+ protected $fileSystem;
/**
* The event dispatcher service.
@@ -121,19 +121,19 @@ class Stage {
* The stager service from Composer Stager.
* @param \PhpTuf\ComposerStager\Domain\CommitterInterface $committer
* The committer service from Composer Stager.
- * @param \PhpTuf\ComposerStager\Domain\CleanerInterface $cleaner
- * The cleaner service from Composer Stager.
+ * @param \Drupal\Core\File\FileSystemInterface $file_system
+ * The file system service.
* @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
* The event dispatcher service.
* @param \Drupal\Core\TempStore\SharedTempStoreFactory $shared_tempstore
* The shared tempstore factory.
*/
- public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, CleanerInterface $cleaner, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore) {
+ public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore) {
$this->pathLocator = $path_locator;
$this->beginner = $beginner;
$this->stager = $stager;
$this->committer = $committer;
- $this->cleaner = $cleaner;
+ $this->fileSystem = $file_system;
$this->eventDispatcher = $event_dispatcher;
$this->tempStore = $shared_tempstore->get('package_manager_stage');
}
@@ -215,7 +215,7 @@ class Stage {
$this->claim($id);
$active_dir = $this->pathLocator->getActiveDirectory();
- $stage_dir = $this->pathLocator->getStageDirectory();
+ $stage_dir = $this->getStageDirectory();
$event = new PreCreateEvent($this);
$this->dispatch($event);
@@ -244,7 +244,7 @@ class Stage {
}
$this->dispatch(new PreRequireEvent($this));
- $this->stager->stage($command, $this->pathLocator->getStageDirectory());
+ $this->stager->stage($command, $this->getStageDirectory());
$this->dispatch(new PostRequireEvent($this));
}
@@ -255,7 +255,7 @@ class Stage {
$this->checkOwnership();
$active_dir = $this->pathLocator->getActiveDirectory();
- $stage_dir = $this->pathLocator->getStageDirectory();
+ $stage_dir = $this->getStageDirectory();
$event = new PreApplyEvent($this);
$this->dispatch($event);
@@ -280,9 +280,12 @@ class Stage {
}
$this->dispatch(new PreDestroyEvent($this));
- $stage_dir = $this->pathLocator->getStageDirectory();
- if (is_dir($stage_dir)) {
- $this->cleaner->clean($stage_dir);
+ // Delete all directories in parent staging directory.
+ $parent_stage_dir = $this->pathLocator->getStageDirectory();
+ if (is_dir($parent_stage_dir)) {
+ $this->fileSystem->deleteRecursive($parent_stage_dir, function (string $path): void {
+ $this->fileSystem->chmod($path, 0777);
+ });
}
$this->markAsAvailable();
$this->dispatch(new PostDestroyEvent($this));
@@ -354,7 +357,7 @@ class Stage {
* The Composer utility object.
*/
public function getStageComposer(): ComposerUtility {
- $dir = $this->pathLocator->getStageDirectory();
+ $dir = $this->getStageDirectory();
return ComposerUtility::createForDirectory($dir);
}
@@ -420,4 +423,22 @@ class Stage {
}
}
+ /**
+ * Returns the path of the directory where changes should be staged.
+ *
+ * @return string
+ * The absolute path of the directory where changes should be staged.
+ *
+ * @throws \LogicException
+ * If this method is called before the stage has been created or claimed.
+ *
+ * @todo Make this method public in https://www.drupal.org/i/3251972.
+ */
+ protected function getStageDirectory(): string {
+ if (!$this->lock) {
+ throw new \LogicException(__METHOD__ . '() cannot be called because the stage has not been created or claimed.');
+ }
+ return $this->pathLocator->getStageDirectory() . DIRECTORY_SEPARATOR . $this->lock[0];
+ }
+
}
diff --git a/package_manager/tests/src/Functional/ExcludedPathsTest.php b/package_manager/tests/src/Functional/ExcludedPathsTest.php
index 9de0de61ef..f3ccffdc64 100644
--- a/package_manager/tests/src/Functional/ExcludedPathsTest.php
+++ b/package_manager/tests/src/Functional/ExcludedPathsTest.php
@@ -59,11 +59,11 @@ class ExcludedPathsTest extends BrowserTestBase {
*/
public function testExcludedPaths(): void {
$active_dir = __DIR__ . '/../../fixtures/fake_site';
- $stage_dir = $this->siteDirectory . '/stage';
+ $parent_stage_dir = $this->siteDirectory . '/stage';
$path_locator = $this->prophesize(PathLocator::class);
$path_locator->getActiveDirectory()->willReturn($active_dir);
- $path_locator->getStageDirectory()->willReturn($stage_dir);
+ $path_locator->getStageDirectory()->willReturn($parent_stage_dir);
$site_path = 'sites/example.com';
@@ -97,11 +97,11 @@ class ExcludedPathsTest extends BrowserTestBase {
$this->container->get('package_manager.beginner'),
$this->container->get('package_manager.stager'),
$this->container->get('package_manager.committer'),
- $this->container->get('package_manager.cleaner'),
+ $this->container->get('file_system'),
$this->container->get('event_dispatcher'),
$this->container->get('tempstore.shared'),
);
- $stage->create();
+ $stage_dir = $parent_stage_dir . DIRECTORY_SEPARATOR . $stage->create();
$this->assertDirectoryExists($stage_dir);
$this->assertDirectoryNotExists("$stage_dir/sites/simpletest");
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index a259fcc185..6a36af152f 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -58,7 +58,7 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
$this->container->get('package_manager.beginner'),
$this->container->get('package_manager.stager'),
$this->container->get('package_manager.committer'),
- $this->container->get('package_manager.cleaner'),
+ $this->container->get('file_system'),
$this->container->get('event_dispatcher'),
$this->container->get('tempstore.shared')
);
diff --git a/package_manager/tests/src/Kernel/ServicesTest.php b/package_manager/tests/src/Kernel/ServicesTest.php
index bb0088e225..ffa23a8a9c 100644
--- a/package_manager/tests/src/Kernel/ServicesTest.php
+++ b/package_manager/tests/src/Kernel/ServicesTest.php
@@ -24,7 +24,6 @@ class ServicesTest extends KernelTestBase {
'package_manager.beginner',
'package_manager.stager',
'package_manager.committer',
- 'package_manager.cleaner',
];
foreach ($services as $service) {
$this->assertIsObject($this->container->get($service));
diff --git a/package_manager/tests/src/Kernel/StageEventsTest.php b/package_manager/tests/src/Kernel/StageEventsTest.php
index fc8a47d6f1..c5f92892fe 100644
--- a/package_manager/tests/src/Kernel/StageEventsTest.php
+++ b/package_manager/tests/src/Kernel/StageEventsTest.php
@@ -51,7 +51,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
$this->container->get('package_manager.beginner'),
$this->container->get('package_manager.stager'),
$this->container->get('package_manager.committer'),
- $this->container->get('package_manager.cleaner'),
+ $this->container->get('file_system'),
$this->container->get('event_dispatcher'),
$this->container->get('tempstore.shared')
);
diff --git a/tests/src/Functional/FileSystemOperationsTest.php b/tests/src/Functional/FileSystemOperationsTest.php
index 315d59f769..1c29e1fda8 100644
--- a/tests/src/Functional/FileSystemOperationsTest.php
+++ b/tests/src/Functional/FileSystemOperationsTest.php
@@ -2,7 +2,6 @@
namespace Drupal\Tests\automatic_updates\Functional;
-use Drupal\package_manager\Cleaner;
use Drupal\automatic_updates\Updater;
use Drupal\Core\Site\Settings;
use Drupal\package_manager\PathLocator;
@@ -60,21 +59,12 @@ class FileSystemOperationsTest extends AutomaticUpdatesFunctionalTestBase {
$locator->getProjectRoot()->willReturn($drupal_root);
$locator->getWebRoot()->willReturn('');
- // Create a cleaner that uses 'sites/default' as its site path, since it
- // will otherwise default to the site path being used for the test site,
- // which doesn't exist in the fake site fixture.
- $cleaner = new Cleaner(
- $this->container->get('package_manager.file_system'),
- 'sites/default',
- $locator->reveal()
- );
-
$this->updater = new Updater(
$locator->reveal(),
$this->container->get('package_manager.beginner'),
$this->container->get('package_manager.stager'),
$this->container->get('package_manager.committer'),
- $cleaner,
+ $this->container->get('file_system'),
$this->container->get('event_dispatcher'),
$this->container->get('tempstore.shared')
);
@@ -103,19 +93,19 @@ class FileSystemOperationsTest extends AutomaticUpdatesFunctionalTestBase {
* @covers \Drupal\automatic_updates\Updater::getExclusions
*/
public function testExclusions(): void {
- $this->updater->begin(['drupal' => '9.8.1']);
- $this->assertFileDoesNotExist("$this->stageDir/sites/default/settings.php");
- $this->assertFileDoesNotExist("$this->stageDir/sites/default/settings.local.php");
- $this->assertFileDoesNotExist("$this->stageDir/sites/default/services.yml");
+ $stage_id = $this->updater->begin(['drupal' => '9.8.1']);
+ $this->assertFileDoesNotExist("$this->stageDir/$stage_id/sites/default/settings.php");
+ $this->assertFileDoesNotExist("$this->stageDir/$stage_id/sites/default/settings.local.php");
+ $this->assertFileDoesNotExist("$this->stageDir/$stage_id/sites/default/services.yml");
// A file in sites/default, that isn't one of the site-specific settings
// files, should be staged.
- $this->assertFileExists("$this->stageDir/sites/default/staged.txt");
- $this->assertDirectoryDoesNotExist("$this->stageDir/sites/simpletest");
- $this->assertDirectoryDoesNotExist("$this->stageDir/files/public");
- $this->assertDirectoryDoesNotExist("$this->stageDir/files/private");
+ $this->assertFileExists("$this->stageDir/$stage_id/sites/default/staged.txt");
+ $this->assertDirectoryDoesNotExist("$this->stageDir/$stage_id/sites/simpletest");
+ $this->assertDirectoryDoesNotExist("$this->stageDir/$stage_id/files/public");
+ $this->assertDirectoryDoesNotExist("$this->stageDir/$stage_id/files/private");
// A file that's in the general files directory, but not in the public or
// private directories, should be staged.
- $this->assertFileExists("$this->stageDir/files/staged.txt");
+ $this->assertFileExists("$this->stageDir/$stage_id/files/staged.txt");
}
/**
@@ -124,11 +114,11 @@ class FileSystemOperationsTest extends AutomaticUpdatesFunctionalTestBase {
* @covers \Drupal\automatic_updates\Cleaner
*/
public function testClean(): void {
- $this->updater->begin(['drupal' => '9.8.1']);
+ $stage_id = $this->updater->begin(['drupal' => '9.8.1']);
// Make the staged site directory read-only, so we can test that it will be
// made writable on clean-up.
- $this->assertTrue(chmod("$this->stageDir/sites/default", 0400));
- $this->assertNotIsWritable("$this->stageDir/sites/default/staged.txt");
+ $this->assertTrue(chmod("$this->stageDir/$stage_id/sites/default", 0400));
+ $this->assertNotIsWritable("$this->stageDir/$stage_id/sites/default/staged.txt");
// If the site directory is not writable, this will throw an exception.
$this->updater->destroy();
$this->assertDirectoryDoesNotExist($this->stageDir);
diff --git a/tests/src/Kernel/ReadinessValidation/StagedProjectsValidatorTest.php b/tests/src/Kernel/ReadinessValidation/StagedProjectsValidatorTest.php
index 1d28571507..aa9a25e809 100644
--- a/tests/src/Kernel/ReadinessValidation/StagedProjectsValidatorTest.php
+++ b/tests/src/Kernel/ReadinessValidation/StagedProjectsValidatorTest.php
@@ -7,6 +7,8 @@ use Drupal\package_manager\Event\PreApplyEvent;
use Drupal\package_manager\Exception\StageValidationException;
use Drupal\package_manager\PathLocator;
use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+use org\bovigo\vfs\vfsStream;
+use Symfony\Component\Filesystem\Filesystem;
/**
* @covers \Drupal\automatic_updates\Validator\StagedProjectsValidator
@@ -48,14 +50,37 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase {
*/
private function validate(string $active_dir, string $stage_dir): array {
$locator = $this->prophesize(PathLocator::class);
+
$locator->getActiveDirectory()->willReturn($active_dir);
$locator->getProjectRoot()->willReturn($active_dir);
$locator->getVendorDirectory()->willReturn($active_dir);
- $locator->getStageDirectory()->willReturn($stage_dir);
+
+ $stage_dir_exists = is_dir($stage_dir);
+ if ($stage_dir_exists) {
+ // If we are testing a fixture with existing stage directory then we
+ // need to use a virtual file system directory, so we can create a
+ // subdirectory using the stage ID after it is created below.
+ $vendor = vfsStream::newDirectory('au_stage');
+ $this->vfsRoot->addChild($vendor);
+ $locator->getStageDirectory()->willReturn($this->vfsRoot->url() . DIRECTORY_SEPARATOR . 'au_stage');
+ }
+ else {
+ // If we are testing non-existent staging directory we can use the path
+ // directly.
+ $locator->getStageDirectory()->willReturn($stage_dir);
+ }
+
$this->container->set('package_manager.path_locator', $locator->reveal());
$updater = $this->container->get('automatic_updates.updater');
- $updater->begin(['drupal' => '9.8.1']);
+ $stage_id = $updater->begin(['drupal' => '9.8.1']);
+ if ($stage_dir_exists) {
+ // Copy the fixture's staging directory into a subdirectory using the
+ // stage ID as the directory name.
+ $sub_directory = vfsStream::newDirectory($stage_id);
+ $vendor->addChild($sub_directory);
+ (new Filesystem())->mirror($stage_dir, $sub_directory->url());
+ }
// The staged projects validator only runs before staged updates are
// applied. Since the active and stage directories may not exist, we don't
--
GitLab