From 8c3fe9d58170f3eca3aea7b89c513b01d052bded Mon Sep 17 00:00:00 2001 From: phenaproxima <phenaproxima@205645.no-reply.drupal.org> Date: Thu, 22 Sep 2022 16:57:32 +0000 Subject: [PATCH] Issue #3293417 by yash.rode, phenaproxima, tedbow, TravisCarden, rahul_, bnjmnm: If an update failed to apply don't allow more use of the module --- automatic_updates.services.yml | 2 + .../automatic_updates_extensions.services.yml | 1 + ...tomaticUpdatesExtensionsKernelTestBase.php | 3 +- package_manager/package_manager.services.yml | 4 + package_manager/src/FailureMarker.php | 87 +++++++++++++++++++ .../src/PackageManagerUninstallValidator.php | 3 +- package_manager/src/Stage.php | 57 ++++++++++-- .../Kernel/PackageManagerKernelTestBase.php | 3 +- .../tests/src/Kernel/StageTest.php | 86 ++++++++++++++++-- src/Form/UpdateReady.php | 5 ++ src/Form/UpdaterForm.php | 24 ++++- src/Updater.php | 8 ++ tests/src/Functional/UpdaterFormTest.php | 38 ++++++++ 13 files changed, 300 insertions(+), 21 deletions(-) create mode 100644 package_manager/src/FailureMarker.php diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml index 6eb57aa7a3..2afad073f3 100644 --- a/automatic_updates.services.yml +++ b/automatic_updates.services.yml @@ -27,6 +27,7 @@ services: - '@tempstore.shared' - '@datetime.time' - '@PhpTuf\ComposerStager\Infrastructure\Factory\Path\PathFactoryInterface' + - '@package_manager.failure_marker' automatic_updates.cron_updater: class: Drupal\automatic_updates\CronUpdater arguments: @@ -45,6 +46,7 @@ services: - '@tempstore.shared' - '@datetime.time' - '@PhpTuf\ComposerStager\Infrastructure\Factory\Path\PathFactoryInterface' + - '@package_manager.failure_marker' automatic_updates.staged_projects_validator: class: Drupal\automatic_updates\Validator\StagedProjectsValidator arguments: diff --git a/automatic_updates_extensions/automatic_updates_extensions.services.yml b/automatic_updates_extensions/automatic_updates_extensions.services.yml index 8e22f97c56..4e38d2f244 100644 --- a/automatic_updates_extensions/automatic_updates_extensions.services.yml +++ b/automatic_updates_extensions/automatic_updates_extensions.services.yml @@ -12,6 +12,7 @@ services: - '@tempstore.shared' - '@datetime.time' - '@PhpTuf\ComposerStager\Infrastructure\Factory\Path\PathFactoryInterface' + - '@package_manager.failure_marker' automatic_updates_extensions.validator.packages_installed_with_composer: class: Drupal\automatic_updates_extensions\Validator\PackagesInstalledWithComposerValidator arguments: diff --git a/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php b/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php index 04bc6d7342..c610ed1123 100644 --- a/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php +++ b/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php @@ -114,7 +114,8 @@ abstract class AutomaticUpdatesExtensionsKernelTestBase extends AutomaticUpdates $this->container->get('event_dispatcher'), $this->container->get('tempstore.shared'), $this->container->get('datetime.time'), - new TestPathFactory() + new TestPathFactory(), + $this->container->get('package_manager.failure_marker') ); } diff --git a/package_manager/package_manager.services.yml b/package_manager/package_manager.services.yml index f9ab5a1b59..50edf6afdb 100644 --- a/package_manager/package_manager.services.yml +++ b/package_manager/package_manager.services.yml @@ -45,6 +45,10 @@ services: - '%app.root%' - '@config.factory' - '@file_system' + package_manager.failure_marker: + class: Drupal\package_manager\FailureMarker + arguments: + - '@package_manager.path_locator' # Validators. package_manager.validator.composer_executable: diff --git a/package_manager/src/FailureMarker.php b/package_manager/src/FailureMarker.php new file mode 100644 index 0000000000..70dc588ce1 --- /dev/null +++ b/package_manager/src/FailureMarker.php @@ -0,0 +1,87 @@ +<?php + +namespace Drupal\package_manager; + +use Drupal\Component\Serialization\Json; +use Drupal\package_manager\Exception\ApplyFailedException; + +/** + * Handles failure marker file operation. + * + * The failure marker is a file placed in the active directory while staged + * code is copied back into it, and then removed afterwards. This allows us to + * know if a commit operation failed midway through, which could leave the site + * code base in an indeterminate state -- which, in the worst case scenario, + * might render Drupal unbootable. + */ +final class FailureMarker { + + /** + * The path locator service. + * + * @var \Drupal\package_manager\PathLocator + */ + protected $pathLocator; + + /** + * Constructs a FailureMarker object. + * + * @param \Drupal\package_manager\PathLocator $pathLocator + * The path locator service. + */ + public function __construct(PathLocator $pathLocator) { + $this->pathLocator = $pathLocator; + } + + /** + * Gets the marker file path. + * + * @return string + * The absolute path of the marker file. + */ + public function getPath(): string { + return $this->pathLocator->getProjectRoot() . '/PACKAGE_MANAGER_FAILURE.json'; + } + + /** + * Deletes the marker file. + */ + public function clear(): void { + unlink($this->getPath()); + } + + /** + * Writes data to marker file. + * + * @param \Drupal\package_manager\Stage $stage + * The stage. + * @param string $message + * Failure message to be added. + */ + public function write(Stage $stage, string $message): void { + $data = [ + 'stage_class' => get_class($stage), + 'stage_file' => (new \ReflectionObject($stage))->getFileName(), + 'message' => $message, + ]; + file_put_contents($this->getPath(), Json::encode($data)); + } + + /** + * Asserts the failure file doesn't exist. + * + * @throws \Drupal\package_manager\Exception\ApplyFailedException + * Thrown if the marker file exists. + */ + public function assertNotExists(): void { + $path = $this->getPath(); + + if (file_exists($path)) { + $data = file_get_contents($path); + $data = Json::decode($data); + + throw new ApplyFailedException($data['message']); + } + } + +} diff --git a/package_manager/src/PackageManagerUninstallValidator.php b/package_manager/src/PackageManagerUninstallValidator.php index 9785121163..c2de44ff4c 100644 --- a/package_manager/src/PackageManagerUninstallValidator.php +++ b/package_manager/src/PackageManagerUninstallValidator.php @@ -35,7 +35,8 @@ final class PackageManagerUninstallValidator implements ModuleUninstallValidator $this->container->get('event_dispatcher'), $this->container->get('tempstore.shared'), $this->container->get('datetime.time'), - $this->container->get(PathFactoryInterface::class) + $this->container->get(PathFactoryInterface::class), + $this->container->get('package_manager.failure_marker') ); if ($stage->isAvailable() || !$stage->isApplying()) { return []; diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php index 653976b607..7c16dcdfb8 100644 --- a/package_manager/src/Stage.php +++ b/package_manager/src/Stage.php @@ -7,6 +7,8 @@ use Drupal\Component\Utility\Crypt; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\File\Exception\FileException; use Drupal\Core\File\FileSystemInterface; +use Drupal\Core\StringTranslation\StringTranslationTrait; +use Drupal\Core\StringTranslation\TranslatableMarkup; use Drupal\Core\TempStore\SharedTempStoreFactory; use Drupal\package_manager\Event\PostApplyEvent; use Drupal\package_manager\Event\PostCreateEvent; @@ -58,6 +60,8 @@ use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; */ class Stage { + use StringTranslationTrait; + /** * The tempstore key under which to store the locking info for this stage. * @@ -170,6 +174,13 @@ class Stage { */ private $lock; + /** + * The failure marker service. + * + * @var \Drupal\package_manager\FailureMarker + */ + protected $failureMarker; + /** * Constructs a new Stage object. * @@ -193,8 +204,10 @@ class Stage { * The time service. * @param \PhpTuf\ComposerStager\Infrastructure\Factory\Path\PathFactoryInterface $path_factory * The path factory service. + * @param \Drupal\package_manager\FailureMarker $failure_marker + * The failure marker service. */ - public function __construct(ConfigFactoryInterface $config_factory, PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore, TimeInterface $time, PathFactoryInterface $path_factory = NULL) { + public function __construct(ConfigFactoryInterface $config_factory, PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore, TimeInterface $time, PathFactoryInterface $path_factory = NULL, FailureMarker $failure_marker = NULL) { $this->configFactory = $config_factory; $this->pathLocator = $path_locator; $this->beginner = $beginner; @@ -209,6 +222,11 @@ class Stage { $path_factory = new PathFactory(); } $this->pathFactory = $path_factory; + if (empty($failure_marker)) { + @trigger_error('Calling ' . __METHOD__ . '() without the $failure_marker argument is deprecated in automatic_updates:8.x-2.3 and will be required before automatic_updates:3.0.0. See https://www.drupal.org/node/3311257.', E_USER_DEPRECATED); + $failure_marker = \Drupal::service('package_manager.failure_marker'); + } + $this->failureMarker = $failure_marker; } /** @@ -282,6 +300,8 @@ class Stage { * @see ::claim() */ public function create(?int $timeout = 300): string { + $this->failureMarker->assertNotExists(); + if (!$this->isAvailable()) { throw new StageException('Cannot create a new stage because one already exists.'); } @@ -379,22 +399,29 @@ class Stage { $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime()); $this->dispatch($event, $this->setNotApplying()); + // Create a marker file so that we can tell later on if the commit failed. + $this->failureMarker->write($this, $this->getFailureMarkerMessage()); + // Exclude the failure file from the commit operation. + $event->excludePath($this->failureMarker->getPath()); + try { $this->committer->commit($stage_dir, $active_dir, new PathList($event->getExcludedPaths()), NULL, $timeout); } - // @todo When this module requires PHP 8 consolidate the next 2 catch blocks - // into 1. - catch (InvalidArgumentException $e) { - throw new StageException($e->getMessage(), $e->getCode(), $e); - } - catch (PreconditionException $e) { + catch (InvalidArgumentException | PreconditionException $e) { + // The commit operation has not started yet, so we can clear the failure + // marker. + $this->failureMarker->clear(); throw new StageException($e->getMessage(), $e->getCode(), $e); } catch (\Throwable $throwable) { - // If the throwable is any other type the commit operation may have - // failed. + // The commit operation may have failed midway through, and the site code + // is in an indeterminate state. Release the flag which says we're still + // applying, because in this situation, the site owner should probably + // restore everything from a backup. + $this->setNotApplying()(); throw new ApplyFailedException($throwable->getMessage(), $throwable->getCode(), $throwable); } + $this->failureMarker->clear(); } /** @@ -564,6 +591,8 @@ class Stage { * @see ::create() */ final public function claim(string $unique_id): self { + $this->failureMarker->assertNotExists(); + if ($this->isAvailable()) { throw new StageException('Cannot claim the stage because no stage has been created.'); } @@ -655,4 +684,14 @@ class Stage { return isset($apply_time) && $this->time->getRequestTime() - $apply_time < 3600; } + /** + * Returns the failure marker message. + * + * @return \Drupal\Core\StringTranslation\TranslatableMarkup + * The translated failure marker message. + */ + protected function getFailureMarkerMessage(): TranslatableMarkup { + return $this->t('Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.'); + } + } diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php index e9490304df..d6d06a90e5 100644 --- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php +++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php @@ -135,7 +135,8 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase { $this->container->get('event_dispatcher'), $this->container->get('tempstore.shared'), $this->container->get('datetime.time'), - new TestPathFactory() + new TestPathFactory(), + $this->container->get('package_manager.failure_marker') ); } diff --git a/package_manager/tests/src/Kernel/StageTest.php b/package_manager/tests/src/Kernel/StageTest.php index 8cf2d036e1..42c0fddd00 100644 --- a/package_manager/tests/src/Kernel/StageTest.php +++ b/package_manager/tests/src/Kernel/StageTest.php @@ -285,9 +285,11 @@ class StageTest extends PackageManagerKernelTestBase { * * @dataProvider providerCommitException */ - public function testCommitException(string $thrown_class, string $expected_class = NULL): void { + public function testCommitException(string $thrown_class, string $expected_class): void { $stage = $this->createStage(); $stage->create(); + $stage->require(['drupal/core' => '9.8.1']); + $thrown_message = 'A very bad thing happened'; // PreconditionException requires a preconditions object. if ($thrown_class === PreconditionException::class) { @@ -297,20 +299,90 @@ class StageTest extends PackageManagerKernelTestBase { $throwable = new $thrown_class($thrown_message, 123); } Committer::setException($throwable); - $this->expectException($expected_class); - $this->expectExceptionMessage($thrown_message); - $this->expectExceptionCode(123); - $stage->require(['drupal/core' => '9.8.1']); + + try { + $stage->apply(); + $this->fail('Expected an exception.'); + } + catch (\Throwable $exception) { + $this->assertInstanceOf($expected_class, $exception); + $this->assertSame($thrown_message, $exception->getMessage()); + $this->assertSame(123, $exception->getCode()); + + $failure_marker = $this->container->get('package_manager.failure_marker'); + if ($exception instanceof ApplyFailedException) { + $this->assertFileExists($failure_marker->getPath()); + $this->assertFalse($stage->isApplying()); + } + else { + $failure_marker->assertNotExists(); + } + } + } + + /** + * Tests that if a stage fails to apply, another stage cannot be created. + */ + public function testFailureMarkerPreventsCreate(): void { + $stage = $this->createStage(); + $stage->create(); + $stage->require(['ext-json:*']); + + // Make the committer throw an exception, which should cause the failure + // marker to be present. + $thrown = new \Exception('Disastrous catastrophe!'); + Committer::setException($thrown); + try { + $stage->apply(); + $this->fail('Expected an exception.'); + } + catch (ApplyFailedException $e) { + $this->assertSame($thrown->getMessage(), $e->getMessage()); + $this->assertFalse($stage->isApplying()); + } + $stage->destroy(); + + // Even through the previous stage was destroyed, we cannot create a new one + // bceause the failure marker is still there. + $stage = $this->createStage(); + try { + $stage->create(); + $this->fail('Expected an exception.'); + } + catch (ApplyFailedException $e) { + $this->assertSame('Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.', $e->getMessage()); + $this->assertFalse($stage->isApplying()); + } + + // If the failure marker is cleared, we should be able to create the stage + // without issue. + $this->container->get('package_manager.failure_marker')->clear(); + $stage->create(); + } + + /** + * Tests that the failure marker file doesn't exist if apply succeeds. + * + * @see ::testCommitException + */ + public function testNoFailureFileOnSuccess(): void { + $stage = $this->createStage(); + $stage->create(); + $stage->require(['ext-json:*']); $stage->apply(); + + $this->container->get('package_manager.failure_marker') + ->assertNotExists(); } /** - * Tests enforcing that the path factory must be passed to the constructor. + * Tests enforcing that certain services must be passed to the constructor. * * @group legacy */ - public function testPathFactoryConstructorDeprecation(): void { + public function testConstructorDeprecations(): void { $this->expectDeprecation('Calling Drupal\package_manager\Stage::__construct() without the $path_factory argument is deprecated in automatic_updates:8.x-2.3 and will be required before automatic_updates:3.0.0. See https://www.drupal.org/node/3310706.'); + $this->expectDeprecation('Calling Drupal\package_manager\Stage::__construct() without the $failure_marker argument is deprecated in automatic_updates:8.x-2.3 and will be required before automatic_updates:3.0.0. See https://www.drupal.org/node/3311257.'); new Stage( $this->container->get('config.factory'), $this->container->get('package_manager.path_locator'), diff --git a/src/Form/UpdateReady.php b/src/Form/UpdateReady.php index 4b07f27778..7a05f0de10 100644 --- a/src/Form/UpdateReady.php +++ b/src/Form/UpdateReady.php @@ -12,6 +12,7 @@ use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Messenger\MessengerInterface; use Drupal\Core\Render\RendererInterface; use Drupal\Core\State\StateInterface; +use Drupal\package_manager\Exception\ApplyFailedException; use Drupal\package_manager\Exception\StageException; use Drupal\package_manager\Exception\StageOwnershipException; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -116,6 +117,10 @@ final class UpdateReady extends FormBase { $this->messenger()->addError($this->t('Cannot continue the update because another Composer operation is currently in progress.')); return $form; } + catch (ApplyFailedException $e) { + $this->messenger()->addError($e->getMessage()); + return $form; + } $messages = []; diff --git a/src/Form/UpdaterForm.php b/src/Form/UpdaterForm.php index 1a14a641b7..1a07169d11 100644 --- a/src/Form/UpdaterForm.php +++ b/src/Form/UpdaterForm.php @@ -4,10 +4,12 @@ namespace Drupal\automatic_updates\Form; use Drupal\automatic_updates\BatchProcessor; use Drupal\automatic_updates\Event\ReadinessCheckEvent; +use Drupal\package_manager\FailureMarker; use Drupal\package_manager\ProjectInfo; use Drupal\automatic_updates\ReleaseChooser; use Drupal\automatic_updates\Updater; use Drupal\automatic_updates\Validation\ReadinessTrait; +use Drupal\package_manager\Exception\ApplyFailedException; use Drupal\update\ProjectRelease; use Drupal\Core\Batch\BatchBuilder; use Drupal\Core\Extension\ExtensionVersion; @@ -73,6 +75,13 @@ final class UpdaterForm extends FormBase { */ protected $renderer; + /** + * Failure marker service. + * + * @var \Drupal\package_manager\FailureMarker + */ + protected $failureMarker; + /** * Constructs a new UpdaterForm object. * @@ -86,13 +95,16 @@ final class UpdaterForm extends FormBase { * The release chooser service. * @param \Drupal\Core\Render\RendererInterface $renderer * The renderer service. + * @param \Drupal\package_manager\FailureMarker $failure_marker + * The failure marker service. */ - public function __construct(StateInterface $state, Updater $updater, EventDispatcherInterface $event_dispatcher, ReleaseChooser $release_chooser, RendererInterface $renderer) { + public function __construct(StateInterface $state, Updater $updater, EventDispatcherInterface $event_dispatcher, ReleaseChooser $release_chooser, RendererInterface $renderer, FailureMarker $failure_marker) { $this->updater = $updater; $this->state = $state; $this->eventDispatcher = $event_dispatcher; $this->releaseChooser = $release_chooser; $this->renderer = $renderer; + $this->failureMarker = $failure_marker; } /** @@ -111,7 +123,8 @@ final class UpdaterForm extends FormBase { $container->get('automatic_updates.updater'), $container->get('event_dispatcher'), $container->get('automatic_updates.release_chooser'), - $container->get('renderer') + $container->get('renderer'), + $container->get('package_manager.failure_marker') ); } @@ -119,6 +132,13 @@ final class UpdaterForm extends FormBase { * {@inheritdoc} */ public function buildForm(array $form, FormStateInterface $form_state) { + try { + $this->failureMarker->assertNotExists(); + } + catch (ApplyFailedException $e) { + $this->messenger()->addError($e->getMessage()); + return $form; + } if ($this->updater->isAvailable()) { $stage_exists = FALSE; } diff --git a/src/Updater.php b/src/Updater.php index 0f9fd2a505..d54d3f99b7 100644 --- a/src/Updater.php +++ b/src/Updater.php @@ -3,6 +3,7 @@ namespace Drupal\automatic_updates; use Drupal\automatic_updates\Exception\UpdateException; +use Drupal\Core\StringTranslation\TranslatableMarkup; use Drupal\package_manager\Event\StageEvent; use Drupal\package_manager\Exception\ApplyFailedException; use Drupal\package_manager\Exception\StageValidationException; @@ -117,4 +118,11 @@ class Updater extends Stage { } } + /** + * {@inheritdoc} + */ + protected function getFailureMarkerMessage(): TranslatableMarkup { + return $this->t('Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup.'); + } + } diff --git a/tests/src/Functional/UpdaterFormTest.php b/tests/src/Functional/UpdaterFormTest.php index 4460a6f448..7fcba223c8 100644 --- a/tests/src/Functional/UpdaterFormTest.php +++ b/tests/src/Functional/UpdaterFormTest.php @@ -11,6 +11,7 @@ use Drupal\package_manager\Event\PostApplyEvent; use Drupal\package_manager\Event\PreCreateEvent; use Drupal\package_manager\ValidationResult; use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1; +use Drupal\package_manager_bypass\Committer; use Drupal\package_manager_bypass\Stager; use Drupal\system\SystemManager; use Drupal\Tests\automatic_updates\Traits\ValidationTestTrait; @@ -312,6 +313,43 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase { $assert_session->pageTextNotContains($cached_message); } + /** + * Tests that an exception is thrown if a previous apply failed. + */ + public function testMarkerFileFailure(): void { + $session = $this->getSession(); + $assert_session = $this->assertSession(); + $page = $session->getPage(); + $this->setCoreVersion('9.8.0'); + $this->checkForUpdates(); + + $this->drupalGet('/admin/modules/automatic-update'); + $assert_session->pageTextNotContains(static::$errorsExplanation); + $assert_session->pageTextNotContains(static::$warningsExplanation); + $page->pressButton('Update to 9.8.1'); + $this->checkForMetaRefresh(); + $this->assertUpdateStagedTimes(1); + + Committer::setException(new \Exception('failed at commiter')); + $page->pressButton('Continue'); + $this->checkForMetaRefresh(); + $assert_session->pageTextContainsOnce('An error has occurred.'); + $assert_session->pageTextContains('The update operation failed to apply. The update may have been partially applied. It is recommended that the site be restored from a code backup.'); + $page->clickLink('the error page'); + + $failure_message = 'Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup.'; + // We should be on the form (i.e., 200 response code), but unable to + // continue the update. + $assert_session->statusCodeEquals(200); + $assert_session->pageTextContains($failure_message); + $assert_session->buttonNotExists('Continue'); + // The same thing should be true if we try to start from the beginning. + $this->drupalGet('/admin/modules/automatic-update'); + $assert_session->statusCodeEquals(200); + $assert_session->pageTextContains($failure_message); + $assert_session->buttonNotExists('Update'); + } + /** * Tests that updating to a different minor version isn't supported. * -- GitLab