From 0886b28a8b4939c379d89cc7aac6bd677f010164 Mon Sep 17 00:00:00 2001
From: phenaproxima <phenaproxima@205645.no-reply.drupal.org>
Date: Tue, 28 Jun 2022 17:26:08 +0000
Subject: [PATCH] Issue #3291147 by phenaproxima, tedbow, effulgentsia, heddn:
Successfully applying an update puts the remainder of the request into an
unreliable state
---
automatic_updates.module | 6 +
automatic_updates.routing.yml | 6 +
automatic_updates.services.yml | 1 +
.../src/BatchProcessor.php | 19 +++
.../src/Form/UpdateReady.php | 1 +
.../src/ApiController.php | 1 +
...tomaticUpdatesExtensionsKernelTestBase.php | 1 +
config/schema/automatic_updates.schema.yml | 3 +
package_manager/src/Stage.php | 36 +++++-
.../package_manager_test_api.routing.yml | 6 +
.../src/ApiController.php | 47 ++++++--
.../tests/src/Build/PackageUpdateTest.php | 28 ++---
.../Kernel/PackageManagerKernelTestBase.php | 1 +
.../tests/src/Kernel/StageEventsTest.php | 2 +
.../tests/src/Kernel/StageOwnershipTest.php | 2 +
.../tests/src/Kernel/StageTest.php | 9 ++
src/BatchProcessor.php | 28 +++++
src/CronUpdater.php | 109 ++++++++++++++++--
src/Form/UpdateReady.php | 1 +
.../src/TestController.php | 1 +
tests/src/Build/UpdateTestBase.php | 30 +++--
.../Kernel/AutomaticUpdatesKernelTestBase.php | 16 +++
tests/src/Kernel/CronUpdaterTest.php | 3 +
.../ReadinessValidationManagerTest.php | 1 +
24 files changed, 306 insertions(+), 52 deletions(-)
diff --git a/automatic_updates.module b/automatic_updates.module
index bfb47fef89..9b5d8534ae 100644
--- a/automatic_updates.module
+++ b/automatic_updates.module
@@ -88,6 +88,12 @@ function automatic_updates_module_implements_alter(&$implementations, $hook) {
// own routes to avoid these messages while an update is in progress.
unset($implementations['update']);
}
+ if ($hook === 'cron') {
+ // Whatever mofo.
+ $hook = $implementations['automatic_updates'];
+ unset($implementations['automatic_updates']);
+ $implementations['automatic_updates'] = $hook;
+ }
}
/**
diff --git a/automatic_updates.routing.yml b/automatic_updates.routing.yml
index 5fa9b92850..dd408c1809 100644
--- a/automatic_updates.routing.yml
+++ b/automatic_updates.routing.yml
@@ -27,6 +27,12 @@ automatic_updates.finish:
options:
_maintenance_access: TRUE
_automatic_updates_readiness_messages: skip
+automatic_updates.cron.post_apply:
+ path: '/automatic-update/cron/post-apply/{stage_id}/{installed_version}/{target_version}/{key}'
+ defaults:
+ _controller: 'automatic_updates.cron_updater:handlePostApply'
+ requirements:
+ _access_system_cron: 'TRUE'
# Links to our updater form appear in three different sets of local tasks. To ensure the breadcrumbs and paths are
# consistent with the other local tasks in each set, we need two separate routes to the same form.
automatic_updates.report_update:
diff --git a/automatic_updates.services.yml b/automatic_updates.services.yml
index 00e0493964..3e79a4a3fe 100644
--- a/automatic_updates.services.yml
+++ b/automatic_updates.services.yml
@@ -33,6 +33,7 @@ services:
- '@logger.factory'
- '@plugin.manager.mail'
- '@language_manager'
+ - '@state'
- '@config.factory'
- '@package_manager.path_locator'
- '@package_manager.beginner'
diff --git a/automatic_updates_extensions/src/BatchProcessor.php b/automatic_updates_extensions/src/BatchProcessor.php
index 49c5e3ae72..2430cc0c5c 100644
--- a/automatic_updates_extensions/src/BatchProcessor.php
+++ b/automatic_updates_extensions/src/BatchProcessor.php
@@ -123,6 +123,25 @@ final class BatchProcessor {
}
}
+ /**
+ * Calls the updater's postApply() method.
+ *
+ * @param string $stage_id
+ * The stage ID.
+ * @param array $context
+ * The current context of the batch job.
+ *
+ * @see \Drupal\automatic_updates\Updater::postApply()
+ */
+ public static function postApply(string $stage_id, array &$context): void {
+ try {
+ static::getUpdater()->claim($stage_id)->postApply();
+ }
+ catch (\Throwable $e) {
+ static::handleException($e, $context);
+ }
+ }
+
/**
* Calls the updater's clean() method.
*
diff --git a/automatic_updates_extensions/src/Form/UpdateReady.php b/automatic_updates_extensions/src/Form/UpdateReady.php
index 6fe7c737ac..76af9c14c6 100644
--- a/automatic_updates_extensions/src/Form/UpdateReady.php
+++ b/automatic_updates_extensions/src/Form/UpdateReady.php
@@ -162,6 +162,7 @@ final class UpdateReady extends FormBase {
->setTitle($this->t('Apply updates'))
->setInitMessage($this->t('Preparing to apply updates'))
->addOperation([BatchProcessor::class, 'commit'], [$stage_id])
+ ->addOperation([BatchProcessor::class, 'postApply'], [$stage_id])
->addOperation([BatchProcessor::class, 'clean'], [$stage_id])
->setFinishCallback([BatchProcessor::class, 'finishCommit'])
->toArray();
diff --git a/automatic_updates_extensions/tests/modules/automatic_updates_extensions_test_api/src/ApiController.php b/automatic_updates_extensions/tests/modules/automatic_updates_extensions_test_api/src/ApiController.php
index fc1c2207c8..9cd99da9a9 100644
--- a/automatic_updates_extensions/tests/modules/automatic_updates_extensions_test_api/src/ApiController.php
+++ b/automatic_updates_extensions/tests/modules/automatic_updates_extensions_test_api/src/ApiController.php
@@ -74,6 +74,7 @@ class ApiController extends ControllerBase {
$this->extensionUpdater->begin($request->get('projects', []));
$this->extensionUpdater->stage();
$this->extensionUpdater->apply();
+ $this->extensionUpdater->postApply();
$this->extensionUpdater->destroy();
$dir = $this->pathLocator->getProjectRoot();
diff --git a/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php b/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php
index 208095f9bc..01467ce09a 100644
--- a/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php
+++ b/automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php
@@ -103,6 +103,7 @@ abstract class AutomaticUpdatesExtensionsKernelTestBase extends AutomaticUpdates
$updater->begin($project_versions);
$updater->stage();
$updater->apply();
+ $updater->postApply();
$updater->destroy();
// If we did not get an exception, ensure we didn't expect any results.
diff --git a/config/schema/automatic_updates.schema.yml b/config/schema/automatic_updates.schema.yml
index c9fe674982..08f5326330 100644
--- a/config/schema/automatic_updates.schema.yml
+++ b/config/schema/automatic_updates.schema.yml
@@ -5,6 +5,9 @@ automatic_updates.settings:
cron:
type: string
label: 'Enable automatic updates during cron'
+ cron_port:
+ type: integer
+ label: 'Port to use for finalization sub-request'
allow_core_minor_updates:
type: boolean
label: 'Allow minor level Drupal core updates'
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index 9c04f552d3..0f8f1c3652 100644
--- a/package_manager/src/Stage.php
+++ b/package_manager/src/Stage.php
@@ -338,6 +338,15 @@ class Stage {
/**
* Applies staged changes to the active directory.
*
+ * After the staged changes are applied, the current request should be
+ * terminated as soon as possible. This is because the code loaded into the
+ * PHP runtime may no longer match the code that is physically present in the
+ * active code base, which means that the current request is running in an
+ * unreliable, inconsistent environment. In the next request,
+ * ::postApply() should be called as early as possible after Drupal is
+ * fully bootstrapped, to rebuild the service container, flush caches, and
+ * dispatch the post-apply event.
+ *
* @param int|null $timeout
* (optional) How long to allow the file copying operation to run before
* timing out, in seconds, or NULL to never time out. Defaults to 600
@@ -352,15 +361,31 @@ class Stage {
// If an error occurs while dispatching the events, ensure that ::destroy()
// doesn't think we're in the middle of applying the staged changes to the
// active directory.
- $release_apply = function (): void {
- $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
- };
-
$event = new PreApplyEvent($this);
$this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
- $this->dispatch($event, $release_apply);
+ $this->dispatch($event, $this->setNotApplying());
$this->committer->commit($stage_dir, $active_dir, new PathList($event->getExcludedPaths()), NULL, $timeout);
+ }
+
+ /**
+ * Returns a closure that marks this stage as no longer being applied.
+ *
+ * @return \Closure
+ * A closure that, when called, marks this stage as no longer in the process
+ * of being applied to the active directory.
+ */
+ private function setNotApplying(): \Closure {
+ return function (): void {
+ $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
+ };
+ }
+
+ /**
+ * Performs post-apply tasks.
+ */
+ public function postApply(): void {
+ $this->checkOwnership();
// Rebuild the container and clear all caches, to ensure that new services
// are picked up.
@@ -370,6 +395,7 @@ class Stage {
// unlikely to call newly added code during the current request.
$this->eventDispatcher = \Drupal::service('event_dispatcher');
+ $release_apply = $this->setNotApplying();
$this->dispatch(new PostApplyEvent($this), $release_apply);
$release_apply();
}
diff --git a/package_manager/tests/modules/package_manager_test_api/package_manager_test_api.routing.yml b/package_manager/tests/modules/package_manager_test_api/package_manager_test_api.routing.yml
index f400c0f4fc..ec3e15596e 100644
--- a/package_manager/tests/modules/package_manager_test_api/package_manager_test_api.routing.yml
+++ b/package_manager/tests/modules/package_manager_test_api/package_manager_test_api.routing.yml
@@ -4,3 +4,9 @@ package_manager_test_api:
_controller: 'Drupal\package_manager_test_api\ApiController::run'
requirements:
_access: 'TRUE'
+package_manager_test_api.finish:
+ path: '/package-manager-test-api/finish/{id}'
+ defaults:
+ _controller: 'Drupal\package_manager_test_api\ApiController::finish'
+ requirements:
+ _access: 'TRUE'
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 45eb27f0f8..35269cc57e 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
@@ -3,10 +3,12 @@
namespace Drupal\package_manager_test_api;
use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\Url;
use Drupal\package_manager\PathLocator;
use Drupal\package_manager\Stage;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
+use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\Request;
/**
@@ -63,10 +65,10 @@ class ApiController extends ControllerBase {
}
/**
- * Runs a complete stage life cycle.
+ * Begins a stage life cycle.
*
* Creates a staging area, requires packages into it, applies changes to the
- * active directory, and destroys the stage.
+ * active directory.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request. The runtime and dev dependencies are expected to be in
@@ -75,18 +77,47 @@ class ApiController extends ControllerBase {
* contains an array of file paths, relative to the project root, whose
* contents should be returned in the response.
*
- * @return \Symfony\Component\HttpFoundation\JsonResponse
- * A JSON response containing an associative array of the contents of the
- * files listed in the 'files_to_return' request key. The array will be
- * keyed by path, relative to the project root.
+ * @return \Symfony\Component\HttpFoundation\RedirectResponse
+ * A response that directs to the ::finish() method.
+ *
+ * @see ::finish()
*/
- public function run(Request $request): JsonResponse {
- $this->stage->create();
+ public function run(Request $request): RedirectResponse {
+ $id = $this->stage->create();
$this->stage->require(
$request->get('runtime', []),
$request->get('dev', [])
);
$this->stage->apply();
+
+ $redirect_url = Url::fromRoute('package_manager_test_api.finish')
+ ->setRouteParameter('id', $id)
+ ->setOption('query', [
+ 'files_to_return' => $request->get('files_to_return', []),
+ ])
+ ->setAbsolute()
+ ->toString();
+
+ return new RedirectResponse($redirect_url);
+ }
+
+ /**
+ * Performs post-apply tasks and destroys the stage.
+ *
+ * @param string $id
+ * The stage ID.
+ * @param \Symfony\Component\HttpFoundation\Request $request
+ * The request. There may be a 'files_to_return' key in either the query
+ * string or request body which contains an array of file paths, relative to
+ * the project root, whose contents should be returned in the response.
+ *
+ * @return \Symfony\Component\HttpFoundation\JsonResponse
+ * A JSON response containing an associative array of the contents of the
+ * files listed in the 'files_to_return' request key. The array will be
+ * keyed by path, relative to the project root.
+ */
+ public function finish(string $id, Request $request): JsonResponse {
+ $this->stage->claim($id)->postApply();
$this->stage->destroy();
$dir = $this->pathLocator->getProjectRoot();
diff --git a/package_manager/tests/src/Build/PackageUpdateTest.php b/package_manager/tests/src/Build/PackageUpdateTest.php
index f0016d4440..120f59b6a1 100644
--- a/package_manager/tests/src/Build/PackageUpdateTest.php
+++ b/package_manager/tests/src/Build/PackageUpdateTest.php
@@ -100,10 +100,11 @@ class PackageUpdateTest extends TemplateProjectTestBase {
$this->assertSame($expected_pre_apply_results, $results['pre']);
$expected_post_apply_results = [
- // Existing functions will still use the pre-update version.
- 'return value of existing global function' => 'pre-update-value',
- // New functions that were added in .module files will not be available.
- 'new global function exists' => 'not exists',
+ // An existing functions will now return a new value.
+ 'return value of existing global function' => 'post-update-value',
+ // New functions that were added in .module files will be available
+ // because the post-apply event is dispatched in a new request.
+ 'new global function exists' => 'exists',
// Definitions for existing routes should be updated.
'path of changed route' => '/updated-module/changed/post',
// Routes deleted from the updated module should not be available.
@@ -127,11 +128,11 @@ class PackageUpdateTest extends TemplateProjectTestBase {
'value of updated_module.added_service' => 'New service, should not exist before update',
// A block removed from the updated module should not be defined anymore.
'updated_module_deleted_block block exists' => 'not exists',
- // A block that was updated should have a changed definition, but an
- // unchanged implementation.
+ // A block that was updated should have a changed definition and
+ // implementation.
'updated_module_updated_block block exists' => 'exists',
'updated_module_updated_block block label' => '1.1.0',
- 'updated_module_updated_block block output' => '1.0.0',
+ 'updated_module_updated_block block output' => '1.1.0',
// A block added to the module should be defined.
'updated_module_added_block block exists' => 'exists',
'updated_module_added_block block label' => 'Added block',
@@ -142,14 +143,13 @@ class PackageUpdateTest extends TemplateProjectTestBase {
'updated_module_ignored_block block exists' => 'exists',
'updated_module_ignored_block block label' => '1.1.0',
'updated_module_ignored_block block output' => 'I was ignored before the update.',
- // Existing class should be available.
+ // Existing class should still be available, and will be outputting its
+ // new value.
'ChangedClass exists' => 'exists',
- // Existing class will still use the pre-update version.
- 'value of ChangedClass' => 'Before Update',
- // Classes loaded in pre-apply or before and deleted from the updated module should
- // be available.
- 'LoadedAndDeletedClass exists' => 'exists',
- 'value of LoadedAndDeletedClass' => 'This class will be loaded and then deleted',
+ 'value of ChangedClass' => 'After Update',
+ // Classes loaded in pre-apply, but deleted from the updated module,
+ // should be unavailable.
+ 'LoadedAndDeletedClass exists' => 'not exists',
// Classes not loaded before the apply operation and deleted from the updated module
// should not be available.
'DeletedClass exists' => 'not exists',
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index bd79149923..617d068d17 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -141,6 +141,7 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
$stage->create();
$stage->require(['drupal/core:9.8.1']);
$stage->apply();
+ $stage->postApply();
$stage->destroy();
// If we did not get an exception, ensure we didn't expect any results.
diff --git a/package_manager/tests/src/Kernel/StageEventsTest.php b/package_manager/tests/src/Kernel/StageEventsTest.php
index 6ce862a881..877dfa71ff 100644
--- a/package_manager/tests/src/Kernel/StageEventsTest.php
+++ b/package_manager/tests/src/Kernel/StageEventsTest.php
@@ -100,6 +100,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
$this->stage->create();
$this->stage->require(['ext-json:*']);
$this->stage->apply();
+ $this->stage->postApply();
$this->stage->destroy();
$this->assertSame($this->events, [
@@ -154,6 +155,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
$this->stage->create();
$this->stage->require(['ext-json:*']);
$this->stage->apply();
+ $this->stage->postApply();
$this->stage->destroy();
$this->fail('Expected \Drupal\package_manager\Exception\StageValidationException to be thrown.');
diff --git a/package_manager/tests/src/Kernel/StageOwnershipTest.php b/package_manager/tests/src/Kernel/StageOwnershipTest.php
index c25a85930f..bcfe025be7 100644
--- a/package_manager/tests/src/Kernel/StageOwnershipTest.php
+++ b/package_manager/tests/src/Kernel/StageOwnershipTest.php
@@ -114,6 +114,7 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
['vendor/lib:0.0.1'],
],
'apply' => [],
+ 'postApply' => [],
'destroy' => [],
];
foreach ($callbacks as $method => $arguments) {
@@ -179,6 +180,7 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
['vendor/lib:0.0.1'],
],
'apply' => [],
+ 'postApply' => [],
'destroy' => [],
];
foreach ($callbacks as $method => $arguments) {
diff --git a/package_manager/tests/src/Kernel/StageTest.php b/package_manager/tests/src/Kernel/StageTest.php
index 584ad1e8ac..addfa40f70 100644
--- a/package_manager/tests/src/Kernel/StageTest.php
+++ b/package_manager/tests/src/Kernel/StageTest.php
@@ -192,6 +192,15 @@ class StageTest extends PackageManagerKernelTestBase {
$this->expectExceptionMessage('Cannot destroy the staging area while it is being applied to the active directory.');
}
$stage->apply();
+
+ // If the stage was successfully destroyed by the event handler (i.e., the
+ // stage has been applying for too long and is therefore considered stale),
+ // the postApply() method should fail because the stage is not claimed.
+ if ($stage->isAvailable()) {
+ $this->expectException('LogicException');
+ $this->expectExceptionMessage('Stage must be claimed before performing any operations on it.');
+ }
+ $stage->postApply();
}
/**
diff --git a/src/BatchProcessor.php b/src/BatchProcessor.php
index d7ab45cbb8..8b94d08c61 100644
--- a/src/BatchProcessor.php
+++ b/src/BatchProcessor.php
@@ -122,6 +122,34 @@ class BatchProcessor {
public static function commit(string $stage_id, array &$context): void {
try {
static::getUpdater()->claim($stage_id)->apply();
+ // The batch system does not allow any single request to run for longer
+ // than a second, so this will force the next operation to be done in a
+ // new request. This helps keep the running code in as consistent a state
+ // as possible.
+ // @see \Drupal\package_manager\Stage::apply()
+ // @see \Drupal\package_manager\Stage::postApply()
+ // @todo See if there's a better way to ensure the post-apply tasks run
+ // in a new request in https://www.drupal.org/i/3293150.
+ sleep(1);
+ }
+ catch (\Throwable $e) {
+ static::handleException($e, $context);
+ }
+ }
+
+ /**
+ * Calls the updater's postApply() method.
+ *
+ * @param string $stage_id
+ * The stage ID.
+ * @param array $context
+ * The current context of the batch job.
+ *
+ * @see \Drupal\automatic_updates\Updater::postApply()
+ */
+ public static function postApply(string $stage_id, array &$context): void {
+ try {
+ static::getUpdater()->claim($stage_id)->postApply();
}
catch (\Throwable $e) {
static::handleException($e, $context);
diff --git a/src/CronUpdater.php b/src/CronUpdater.php
index 3dad8ae033..d315d65d09 100644
--- a/src/CronUpdater.php
+++ b/src/CronUpdater.php
@@ -6,7 +6,11 @@ use Drupal\automatic_updates_9_3_shim\ProjectRelease;
use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\Core\Logger\LoggerChannelFactoryInterface;
use Drupal\Core\Mail\MailManagerInterface;
+use Drupal\Core\State\StateInterface;
+use Drupal\Core\Url;
use Drupal\package_manager\Exception\StageValidationException;
+use GuzzleHttp\Psr7\Uri as GuzzleUri;
+use Symfony\Component\HttpFoundation\Response;
/**
* Defines a service that updates via cron.
@@ -75,6 +79,13 @@ class CronUpdater extends Updater {
*/
protected $languageManager;
+ /**
+ * The state service.
+ *
+ * @var \Drupal\Core\State\StateInterface
+ */
+ protected $state;
+
/**
* Constructs a CronUpdater object.
*
@@ -86,15 +97,18 @@ class CronUpdater extends Updater {
* The mail manager service.
* @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
* The language manager service.
+ * @param \Drupal\Core\State\StateInterface $state
+ * The state service.
* @param mixed ...$arguments
* Additional arguments to pass to the parent constructor.
*/
- public function __construct(ReleaseChooser $release_chooser, LoggerChannelFactoryInterface $logger_factory, MailManagerInterface $mail_manager, LanguageManagerInterface $language_manager, ...$arguments) {
+ public function __construct(ReleaseChooser $release_chooser, LoggerChannelFactoryInterface $logger_factory, MailManagerInterface $mail_manager, LanguageManagerInterface $language_manager, StateInterface $state, ...$arguments) {
parent::__construct(...$arguments);
$this->releaseChooser = $release_chooser;
$this->logger = $logger_factory->get('automatic_updates');
$this->mailManager = $mail_manager;
$this->languageManager = $language_manager;
+ $this->state = $state;
}
/**
@@ -162,9 +176,91 @@ class CronUpdater extends Updater {
// stage regardless of whether the update succeeds.
try {
// @see ::begin()
- parent::begin(['drupal' => $target_version], $timeout);
+ $stage_id = parent::begin(['drupal' => $target_version], $timeout);
$this->stage();
$this->apply();
+ }
+ catch (\Throwable $e) {
+ $this->logger->error($e->getMessage());
+
+ // If an error occurred during the pre-create event, the stage will be
+ // marked as available and we shouldn't try to destroy it, since the stage
+ // must be claimed in order to be destroyed.
+ if (!$this->isAvailable()) {
+ $this->destroy();
+ }
+ return;
+ }
+
+ // Perform a subrequest to run ::postApply(), which needs to be done in a
+ // separate request.
+ // @see parent::apply()
+ $url = Url::fromRoute('automatic_updates.cron.post_apply', [
+ 'stage_id' => $stage_id,
+ 'installed_version' => $installed_version,
+ 'target_version' => $target_version,
+ 'key' => $this->state->get('system.cron_key'),
+ ]);
+ $this->triggerPostApply($url);
+ }
+
+ /**
+ * Executes a subrequest to run post-apply tasks.
+ *
+ * @param \Drupal\Core\Url $url
+ * The URL of the post-apply handler.
+ */
+ protected function triggerPostApply(Url $url): void {
+ $url = $url->setAbsolute()->toString();
+
+ // If we're using a single-threaded web server (e.g., the built-in PHP web
+ // server used in build tests), allow the post-apply request to be sent to
+ // an alternate port.
+ // @todo If using the built-in PHP web server, validate that this port is
+ // set in https://www.drupal.org/i/3293146.
+ $port = $this->configFactory->get('automatic_updates.settings')
+ ->get('cron_port');
+ if ($port) {
+ $url = (string) (new GuzzleUri($url))->withPort($port);
+ }
+
+ // Use the bare cURL API to make the request, so that we're not relying on
+ // any third-party classes or other code which may have changed during the
+ // update.
+ $curl = curl_init($url);
+ curl_setopt($curl, CURLOPT_RETURNTRANSFER, TRUE);
+ $response = curl_exec($curl);
+ $status = curl_getinfo($curl, CURLINFO_RESPONSE_CODE);
+ if ($status !== 200) {
+ $this->logger->error('Post-apply tasks failed with output: %status %response', [
+ '%status' => $status,
+ '%response' => $response,
+ ]);
+ }
+ curl_close($curl);
+ }
+
+ /**
+ * Runs post-apply tasks.
+ *
+ * @param string $stage_id
+ * The stage ID.
+ * @param string $installed_version
+ * The version of Drupal core that started the update.
+ * @param string $target_version
+ * The version of Drupal core to which we updated.
+ *
+ * @return \Symfony\Component\HttpFoundation\Response
+ * An empty 200 response if the post-apply tasks succeeded.
+ */
+ public function handlePostApply(string $stage_id, string $installed_version, string $target_version): Response {
+ $this->claim($stage_id);
+
+ // Run post-apply tasks in their own try-catch block so that, if anything
+ // raises an exception, we'll log it and proceed to destroy the stage as
+ // soon as possible (which is also what we do in ::performUpdate()).
+ try {
+ $this->postApply();
$this->logger->info(
'Drupal core has been updated from %previous_version to %target_version',
@@ -189,13 +285,6 @@ class CronUpdater extends Updater {
$this->logger->error($e->getMessage());
}
- // If an error occurred during the pre-create event, the stage will be
- // marked as available and we shouldn't try to destroy it, since the stage
- // must be claimed in order to be destroyed.
- if ($this->isAvailable()) {
- return;
- }
-
// If any pre-destroy event subscribers raise validation errors, ensure they
// are formatted and logged. But if any pre- or post-destroy event
// subscribers throw another exception, don't bother catching it, since it
@@ -206,6 +295,8 @@ class CronUpdater extends Updater {
catch (StageValidationException $e) {
$this->logger->error($e->getMessage());
}
+
+ return new Response();
}
/**
diff --git a/src/Form/UpdateReady.php b/src/Form/UpdateReady.php
index 1867539cc7..abc4caa53f 100644
--- a/src/Form/UpdateReady.php
+++ b/src/Form/UpdateReady.php
@@ -199,6 +199,7 @@ class UpdateReady extends FormBase {
->setTitle($this->t('Apply updates'))
->setInitMessage($this->t('Preparing to apply updates'))
->addOperation([BatchProcessor::class, 'commit'], [$stage_id])
+ ->addOperation([BatchProcessor::class, 'postApply'], [$stage_id])
->addOperation([BatchProcessor::class, 'clean'], [$stage_id])
->setFinishCallback([BatchProcessor::class, 'finishCommit'])
->toArray();
diff --git a/tests/modules/automatic_updates_test/src/TestController.php b/tests/modules/automatic_updates_test/src/TestController.php
index 82dbb17b66..e48e80dd94 100644
--- a/tests/modules/automatic_updates_test/src/TestController.php
+++ b/tests/modules/automatic_updates_test/src/TestController.php
@@ -32,6 +32,7 @@ class TestController extends ControllerBase {
$updater->begin(['drupal' => $to_version]);
$updater->stage();
$updater->apply();
+ $updater->postApply();
$updater->destroy();
// The code base has been updated, but as far as the PHP runtime is
diff --git a/tests/src/Build/UpdateTestBase.php b/tests/src/Build/UpdateTestBase.php
index f80214db3f..c2ef131588 100644
--- a/tests/src/Build/UpdateTestBase.php
+++ b/tests/src/Build/UpdateTestBase.php
@@ -42,6 +42,19 @@ abstract class UpdateTestBase extends TemplateProjectTestBase {
'automatic_updates_test_cron',
'automatic_updates_test_release_history',
]);
+
+ // When checking for updates, we need to be able to make sub-requests, but
+ // the built-in PHP server is single-threaded. Therefore, open a second
+ // server instance on another port, which will serve the metadata about
+ // available updates.
+ $port = $this->findAvailablePort();
+ $this->metadataServer = $this->instantiateServer($port);
+
+ $code = <<<END
+\$config['automatic_updates.settings']['cron_port'] = $port;
+\$config['update.settings']['fetch']['url'] = 'http://localhost:$port/test-release-history';
+END;
+ $this->writeSettings($code);
}
/**
@@ -54,22 +67,7 @@ abstract class UpdateTestBase extends TemplateProjectTestBase {
*/
protected function setReleaseMetadata(array $xml_map): void {
$xml_map = var_export($xml_map, TRUE);
- $code = <<<END
-\$config['update_test.settings']['xml_map'] = $xml_map;
-END;
-
- // When checking for updates, we need to be able to make sub-requests, but
- // the built-in PHP server is single-threaded. Therefore, if needed, open a
- // second server instance on another port, which will serve the metadata
- // about available updates.
- if (empty($this->metadataServer)) {
- $port = $this->findAvailablePort();
- $this->metadataServer = $this->instantiateServer($port);
- $code .= <<<END
-\$config['update.settings']['fetch']['url'] = 'http://localhost:$port/test-release-history';
-END;
- }
- $this->writeSettings($code);
+ $this->writeSettings("\$config['update_test.settings']['xml_map'] = $xml_map;");
}
/**
diff --git a/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php b/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
index 8e7046d441..532ca14047 100644
--- a/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
+++ b/tests/src/Kernel/AutomaticUpdatesKernelTestBase.php
@@ -5,6 +5,7 @@ namespace Drupal\Tests\automatic_updates\Kernel;
use Drupal\automatic_updates\CronUpdater;
use Drupal\automatic_updates\Updater;
use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\Url;
use Drupal\Tests\automatic_updates\Traits\ValidationTestTrait;
use Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase;
use Drupal\Tests\package_manager\Kernel\TestStageTrait;
@@ -53,6 +54,11 @@ abstract class AutomaticUpdatesKernelTestBase extends PackageManagerKernelTestBa
if (in_array('package_manager.validator.file_system', $this->disableValidators, TRUE)) {
$this->disableValidators[] = 'automatic_updates.validator.file_system_permissions';
}
+ // If Package Manager's symlink validator is disabled, also disable the
+ // Automatic Updates validator which wraps it.
+ if (in_array('package_manager.validator.symlink', $this->disableValidators, TRUE)) {
+ $this->disableValidators[] = 'automatic_updates.validator.symlink';
+ }
// Always disable the Xdebug validator to allow test to run with Xdebug on.
$this->disableValidators[] = 'automatic_updates.validator.xdebug';
parent::setUp();
@@ -168,4 +174,14 @@ class TestCronUpdater extends CronUpdater {
use TestStageTrait;
+ /**
+ * {@inheritdoc}
+ */
+ protected function triggerPostApply(Url $url): void {
+ // Subrequests don't work in kernel tests, so just call the post-apply
+ // handler directly.
+ $parameters = $url->getRouteParameters();
+ $this->handlePostApply($parameters['stage_id'], $parameters['installed_version'], $parameters['target_version']);
+ }
+
}
diff --git a/tests/src/Kernel/CronUpdaterTest.php b/tests/src/Kernel/CronUpdaterTest.php
index ab16c9dd4a..f07e16086f 100644
--- a/tests/src/Kernel/CronUpdaterTest.php
+++ b/tests/src/Kernel/CronUpdaterTest.php
@@ -60,6 +60,9 @@ class CronUpdaterTest extends AutomaticUpdatesKernelTestBase {
$this->disableValidators[] = 'automatic_updates.validator.staged_database_updates';
$this->disableValidators[] = 'automatic_updates.staged_projects_validator';
$this->disableValidators[] = 'automatic_updates.validator.scaffold_file_permissions';
+ // Since staging operations are bypassed, ignore any symbolic links in the
+ // running code base.
+ $this->disableValidators[] = 'package_manager.validator.symlink';
parent::setUp();
$this->logger = new TestLogger();
diff --git a/tests/src/Kernel/ReadinessValidation/ReadinessValidationManagerTest.php b/tests/src/Kernel/ReadinessValidation/ReadinessValidationManagerTest.php
index 2aff4c4ec6..d9fb9faf80 100644
--- a/tests/src/Kernel/ReadinessValidation/ReadinessValidationManagerTest.php
+++ b/tests/src/Kernel/ReadinessValidation/ReadinessValidationManagerTest.php
@@ -257,6 +257,7 @@ class ReadinessValidationManagerTest extends AutomaticUpdatesKernelTestBase {
$updater->begin(['drupal' => '9.8.1']);
$updater->stage();
$updater->apply();
+ $updater->postApply();
$updater->destroy();
// The readiness validation manager shouldn't have any stored results.
--
GitLab