diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml index cbb5b4164504ee607b9fd8ed308fd35cd1286243..0e50d2b22d0d06fbd7274e31908f91db84115d89 100644 --- a/automatic_updates.services.yml +++ b/automatic_updates.services.yml @@ -21,6 +21,7 @@ services: - '@file_system' - '@event_dispatcher' - '@tempstore.shared' + - '@datetime.time' automatic_updates.cron_updater: class: Drupal\automatic_updates\CronUpdater arguments: @@ -33,6 +34,7 @@ services: - '@file_system' - '@event_dispatcher' - '@tempstore.shared' + - '@datetime.time' automatic_updates.staged_projects_validator: class: Drupal\automatic_updates\Validator\StagedProjectsValidator arguments: diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php index 4c71210753039c20e25c62ccd079bb7b02258422..b93a3bcf7f6efadadb885a4fb7a62be3d95e552c 100644 --- a/package_manager/src/Stage.php +++ b/package_manager/src/Stage.php @@ -2,6 +2,7 @@ namespace Drupal\package_manager; +use Drupal\Component\Datetime\TimeInterface; use Drupal\Component\FileSystem\FileSystem; use Drupal\Component\Utility\Crypt; use Drupal\Core\File\Exception\FileException; @@ -55,6 +56,16 @@ class Stage { */ protected const TEMPSTORE_METADATA_KEY = 'metadata'; + /** + * The tempstore key under which to store the time that ::apply() was called. + * + * @var string + * + * @see ::apply() + * @see ::destroy() + */ + private const TEMPSTORE_APPLY_TIME_KEY = 'apply_time'; + /** * The path locator service. * @@ -104,6 +115,13 @@ class Stage { */ protected $tempStore; + /** + * The time service. + * + * @var \Drupal\Component\Datetime\TimeInterface + */ + protected $time; + /** * The lock info for the stage. * @@ -130,14 +148,17 @@ class Stage { * The event dispatcher service. * @param \Drupal\Core\TempStore\SharedTempStoreFactory $shared_tempstore * The shared tempstore factory. + * @param \Drupal\Component\Datetime\TimeInterface $time + * The time service. */ - public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, 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, TimeInterface $time) { $this->pathLocator = $path_locator; $this->beginner = $beginner; $this->stager = $stager; $this->committer = $committer; $this->fileSystem = $file_system; $this->eventDispatcher = $event_dispatcher; + $this->time = $time; $this->tempStore = $shared_tempstore->get('package_manager_stage'); } @@ -224,7 +245,9 @@ class Stage { $stage_dir = $this->getStageDirectory(); $event = new PreCreateEvent($this); - $this->dispatch($event); + // If an error occurs and we won't be able to create the stage, mark it as + // available. + $this->dispatch($event, [$this, 'markAsAvailable']); $this->beginner->begin($active_dir, $stage_dir, $event->getExcludedPaths()); $this->dispatch(new PostCreateEvent($this)); @@ -277,9 +300,16 @@ class Stage { $stage_dir = $this->getStageDirectory(); $event = new PreApplyEvent($this); - $this->dispatch($event); + $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime()); + // If an error occurs while dispatching the event, ensure that ::destroy() + // doesn't think we're in the middle of applying the staged changes to the + // active directory. + $this->dispatch($event, function (): void { + $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY); + }); $this->committer->commit($stage_dir, $active_dir, $event->getExcludedPaths()); + $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY); $this->dispatch(new PostApplyEvent($this)); } @@ -290,14 +320,22 @@ class Stage { * (optional) If TRUE, the staging area will be destroyed even if it is not * owned by the current user or session. Defaults to FALSE. * - * @todo Do not allow the stage to be destroyed while it's being applied to - * the active directory in https://www.drupal.org/i/3248909. + * @throws \Drupal\package_manager\Exception\StageException + * If the staged changes are being applied to the active directory. */ public function destroy(bool $force = FALSE): void { if (!$force) { $this->checkOwnership(); } + // If we started applying staged changes to the active directory less than + // an hour ago, prevent the stage from being destroyed. + // @see :apply() + $apply_time = $this->tempStore->get(self::TEMPSTORE_APPLY_TIME_KEY); + if (isset($apply_time) && $this->time->getRequestTime() - $apply_time < 3600) { + throw new StageException('Cannot destroy the staging area while it is being applied to the active directory.'); + } + $this->dispatch(new PreDestroyEvent($this)); // Delete all directories in parent staging directory. $parent_stage_dir = static::getStagingRoot(); @@ -332,13 +370,16 @@ class Stage { * * @param \Drupal\package_manager\Event\StageEvent $event * The event object. + * @param callable $on_error + * (optional) A callback function to call if an error occurs, before any + * exceptions are thrown. * * @throws \Drupal\package_manager\Exception\StageValidationException * If the event collects any validation errors. * @throws \Drupal\package_manager\Exception\StageException * If any other sort of error occurs. */ - protected function dispatch(StageEvent $event): void { + protected function dispatch(StageEvent $event, callable $on_error = NULL): void { try { $this->eventDispatcher->dispatch($event); @@ -354,10 +395,8 @@ class Stage { } if (isset($error)) { - // If we won't be able to create the staging area, mark it as available. - // @see ::create() - if ($event instanceof PreCreateEvent) { - $this->markAsAvailable(); + if ($on_error) { + $on_error(); } throw $error; } diff --git a/package_manager/tests/modules/package_manager_test_api/src/ApiController.php b/package_manager/tests/modules/package_manager_test_api/src/ApiController.php index a9f116ff2bc82ac62497304e31790a7c1953a208..9c237bc0006bec751e3304e6a67259e589a92544 100644 --- a/package_manager/tests/modules/package_manager_test_api/src/ApiController.php +++ b/package_manager/tests/modules/package_manager_test_api/src/ApiController.php @@ -42,6 +42,7 @@ class ApiController extends ControllerBase { $container->get('file_system'), $container->get('event_dispatcher'), $container->get('tempstore.shared'), + $container->get('datetime.time') ); return new static($stage); } diff --git a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php index e58267f1bb0dc50c550a1db621de9e82219c7ca3..0aeb5ede68ef6d92059aa8ef6e83552138521302 100644 --- a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php +++ b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php @@ -44,6 +44,19 @@ class TestSubscriber implements EventSubscriberInterface { $this->state = $state; } + /** + * Sets whether a specific event will call exit(). + * + * This is useful for simulating an unrecoverable (fatal) error when handling + * the given event. + * + * @param string $event + * The event class. + */ + public static function setExit(string $event): void { + \Drupal::state()->set(static::STATE_KEY . ".$event", 'exit'); + } + /** * Sets validation results for a specific event. * @@ -102,6 +115,9 @@ class TestSubscriber implements EventSubscriberInterface { if ($results instanceof \Throwable) { throw $results; } + elseif ($results === 'exit') { + exit(); + } /** @var \Drupal\package_manager\ValidationResult $result */ foreach ($results as $result) { if ($result->getSeverity() === SystemManager::REQUIREMENT_ERROR) { diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php index 647878fcae1a417468506edb8890a1d571daaec7..8bce0be1a87a2f0d46818c01a939e73b909a0300 100644 --- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php +++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php @@ -78,7 +78,8 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase { $this->container->get('package_manager.committer'), $this->container->get('file_system'), $this->container->get('event_dispatcher'), - $this->container->get('tempstore.shared') + $this->container->get('tempstore.shared'), + $this->container->get('datetime.time') ); } @@ -297,9 +298,9 @@ class TestStage extends Stage { /** * {@inheritdoc} */ - protected function dispatch(StageEvent $event): void { + protected function dispatch(StageEvent $event, callable $on_error = NULL): void { try { - parent::dispatch($event); + parent::dispatch($event, $on_error); } catch (StageException $e) { // Attach the event object to the exception so that test code can verify diff --git a/package_manager/tests/src/Kernel/StageTest.php b/package_manager/tests/src/Kernel/StageTest.php index 0ed5f56049ab936218670b10af52aa2ec41e2e3c..4d8e419cebd97727fbd4d9dd67800fcbf6e36755 100644 --- a/package_manager/tests/src/Kernel/StageTest.php +++ b/package_manager/tests/src/Kernel/StageTest.php @@ -2,6 +2,11 @@ namespace Drupal\Tests\package_manager\Kernel; +use Drupal\Component\Datetime\Time; +use Drupal\Core\DependencyInjection\ContainerBuilder; +use Drupal\package_manager\Event\PreApplyEvent; +use Drupal\package_manager\Exception\StageException; + /** * @coversDefaultClass \Drupal\package_manager\Stage * @@ -9,6 +14,16 @@ namespace Drupal\Tests\package_manager\Kernel; */ class StageTest extends PackageManagerKernelTestBase { + /** + * {@inheritdoc} + */ + public function register(ContainerBuilder $container) { + parent::register($container); + + $container->getDefinition('datetime.time') + ->setClass(TestTime::class); + } + /** * @covers ::getStageDirectory */ @@ -27,4 +42,78 @@ class StageTest extends PackageManagerKernelTestBase { $this->createStage()->getStageDirectory(); } + /** + * Data provider for ::testDestroyDuringApply(). + * + * @return array[] + * Sets of arguments to pass to the test method. + */ + public function providerDestroyDuringApply(): array { + return [ + 'force destroy, not stale' => [TRUE, 1, TRUE], + 'regular destroy, not stale' => [FALSE, 1, TRUE], + 'force destroy, stale' => [TRUE, 7200, FALSE], + 'regular destroy, stale' => [FALSE, 7200, FALSE], + ]; + } + + /** + * Tests destroying a stage while applying it. + * + * @param bool $force + * Whether or not the stage should be force destroyed. + * @param int $time_offset + * How many simulated seconds should have elapsed between the PreApplyEvent + * being dispatched and the attempt to destroy the stage. + * @param bool $expect_exception + * Whether or not destroying the stage will raise an exception. + * + * @dataProvider providerDestroyDuringApply + */ + public function testDestroyDuringApply(bool $force, int $time_offset, bool $expect_exception): void { + $listener = function (PreApplyEvent $event) use ($force, $time_offset): void { + // Simulate that a certain amount of time has passed since we started + // applying staged changes. After a point, it should be possible to + // destroy the stage even if it hasn't finished. + TestTime::$offset = $time_offset; + + // No real-life event subscriber should try to destroy the stage while + // handling another event. The only reason we're doing it here is to + // simulate an attempt to destroy the stage while it's being applied, for + // testing purposes. + $event->getStage()->destroy($force); + }; + $this->container->get('event_dispatcher') + ->addListener(PreApplyEvent::class, $listener); + + $stage = $this->createStage(); + $stage->create(); + if ($expect_exception) { + $this->expectException(StageException::class); + $this->expectExceptionMessage('Cannot destroy the staging area while it is being applied to the active directory.'); + } + $stage->apply(); + } + +} + +/** + * A test-only implementation of the time service. + */ +class TestTime extends Time { + + /** + * An offset to add to the request time. + * + * @var int + */ + public static $offset = 0; + + /** + * {@inheritdoc} + */ + public function getRequestTime() { + return parent::getRequestTime() + static::$offset; + } + } diff --git a/src/Form/UpdateReady.php b/src/Form/UpdateReady.php index 6f81bac387c51e8edbf92732982407111f13ddee..6fd63c3fabf8a81bf6a5873f03a898963d17e34e 100644 --- a/src/Form/UpdateReady.php +++ b/src/Form/UpdateReady.php @@ -11,6 +11,7 @@ use Drupal\Core\Form\FormBase; use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Messenger\MessengerInterface; use Drupal\Core\State\StateInterface; +use Drupal\package_manager\Exception\StageException; use Drupal\package_manager\Exception\StageOwnershipException; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -193,9 +194,14 @@ class UpdateReady extends FormBase { * Cancels the in-progress update. */ public function cancel(array &$form, FormStateInterface $form_state): void { - $this->updater->destroy(); - $this->messenger()->addStatus($this->t('The update was successfully cancelled.')); - $form_state->setRedirect('automatic_updates.report_update'); + try { + $this->updater->destroy(); + $this->messenger()->addStatus($this->t('The update was successfully cancelled.')); + $form_state->setRedirect('automatic_updates.report_update'); + } + catch (StageException $e) { + $this->messenger()->addError($e->getMessage()); + } } } diff --git a/src/Form/UpdaterForm.php b/src/Form/UpdaterForm.php index d6e2daa4f069d98e4415eef83d315612eb4268a0..e1ee4e2e9a53eb7497bd96117d4aae4c293d37ef 100644 --- a/src/Form/UpdaterForm.php +++ b/src/Form/UpdaterForm.php @@ -14,6 +14,7 @@ use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Link; use Drupal\Core\State\StateInterface; use Drupal\Core\Url; +use Drupal\package_manager\Exception\StageException; use Drupal\package_manager\Exception\StageOwnershipException; use Drupal\system\SystemManager; use Drupal\update\UpdateManagerInterface; @@ -276,8 +277,13 @@ class UpdaterForm extends FormBase { * Submit function to delete an existing in-progress update. */ public function deleteExistingUpdate(): void { - $this->updater->destroy(TRUE); - $this->messenger()->addMessage($this->t("Staged update deleted")); + try { + $this->updater->destroy(TRUE); + $this->messenger()->addMessage($this->t("Staged update deleted")); + } + catch (StageException $e) { + $this->messenger()->addError($e->getMessage()); + } } /** diff --git a/src/Updater.php b/src/Updater.php index f8c3ca05e7ba598cb9cad68d25ed69e631cb6948..c333f6c376549f501914af7bd84d9bf3d5558893 100644 --- a/src/Updater.php +++ b/src/Updater.php @@ -91,9 +91,9 @@ class Updater extends Stage { /** * {@inheritdoc} */ - protected function dispatch(StageEvent $event): void { + protected function dispatch(StageEvent $event, callable $on_error = NULL): void { try { - parent::dispatch($event); + parent::dispatch($event, $on_error); } catch (StageValidationException $e) { throw new UpdateException($e->getResults(), $e->getMessage() ?: "Unable to complete the update because of errors.", $e->getCode(), $e); diff --git a/tests/src/Functional/UpdaterFormTest.php b/tests/src/Functional/UpdaterFormTest.php index c68f84dcba8d19bb446711031b8876a58ab1016e..2e4f6ed52577e79db983c74d626330b75bd211b1 100644 --- a/tests/src/Functional/UpdaterFormTest.php +++ b/tests/src/Functional/UpdaterFormTest.php @@ -3,6 +3,8 @@ namespace Drupal\Tests\automatic_updates\Functional; use Drupal\automatic_updates\Event\ReadinessCheckEvent; +use Drupal\automatic_updates_test\Datetime\TestTime; +use Drupal\package_manager\Event\PreApplyEvent; use Drupal\package_manager\Event\PreCreateEvent; use Drupal\package_manager\ValidationResult; use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1; @@ -240,6 +242,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase { */ public function testDeleteExistingUpdate(): void { $conflict_message = 'Cannot begin an update because another Composer operation is currently in progress.'; + $cancelled_message = 'The update was successfully cancelled.'; $assert_session = $this->assertSession(); $page = $this->getSession()->getPage(); @@ -263,7 +266,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase { // Delete the existing update. $page->pressButton('Cancel update'); $assert_session->addressEquals('/admin/reports/updates/automatic-update'); - $assert_session->pageTextContains('The update was successfully cancelled.'); + $assert_session->pageTextContains($cancelled_message); $assert_session->pageTextNotContains($conflict_message); // Ensure we can start another update after deleting the existing one. $page->pressButton('Update'); @@ -281,12 +284,60 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase { $this->drupalGet('/admin/reports/updates/automatic-update'); $assert_session->pageTextContains($conflict_message); $assert_session->buttonNotExists('Update'); - // We should be able to delete the previous update, and then we should be - // able to start a new one. + // We should be able to delete the previous update, then start a new one. $page->pressButton('Delete existing update'); $assert_session->pageTextContains('Staged update deleted'); $assert_session->pageTextNotContains($conflict_message); - $assert_session->buttonExists('Update'); + $page->pressButton('Update'); + $this->checkForMetaRefresh(); + $this->assertUpdateReady(); + + // Stop execution during pre-apply. This should make Package Manager think + // the staged changes are being applied and raise an error if we try to + // cancel the update. + TestSubscriber1::setExit(PreApplyEvent::class); + $page->pressButton('Continue'); + $this->checkForMetaRefresh(); + $page->clickLink('the error page'); + $page->pressButton('Cancel update'); + // The exception should have been caught and displayed in the messages area. + $assert_session->statusCodeEquals(200); + $destroy_error = 'Cannot destroy the staging area while it is being applied to the active directory.'; + $assert_session->pageTextContains($destroy_error); + $assert_session->pageTextNotContains($cancelled_message); + + // We should get the same error if we log in as another user and try to + // delete the staged update. + $this->drupalLogin($this->rootUser); + $this->drupalGet('/admin/reports/updates/automatic-update'); + $assert_session->pageTextContains($conflict_message); + $page->pressButton('Delete existing update'); + $assert_session->statusCodeEquals(200); + $assert_session->pageTextContains($destroy_error); + $assert_session->pageTextNotContains('Staged update deleted'); + + // Two hours later, Package Manager should consider the stage to be stale, + // allowing the staged update to be deleted. + TestTime::setFakeTimeByOffset('+2 hours'); + $this->getSession()->reload(); + $assert_session->pageTextContains($conflict_message); + $page->pressButton('Delete existing update'); + $assert_session->statusCodeEquals(200); + $assert_session->pageTextContains('Staged update deleted'); + + // If a legitimate error is raised during pre-apply, we should be able to + // delete the staged update right away. + $this->createTestValidationResults(); + $results = $this->testResults['checker_1']['1 error']; + TestSubscriber1::setTestResult($results, PreApplyEvent::class); + $page->pressButton('Update'); + $this->checkForMetaRefresh(); + $this->assertUpdateReady(); + $page->pressButton('Continue'); + $this->checkForMetaRefresh(); + $page->clickLink('the error page'); + $page->pressButton('Cancel update'); + $assert_session->pageTextContains($cancelled_message); } /**