From 1fcd8e928a457d86e8239d4adfa85ffc56be9f80 Mon Sep 17 00:00:00 2001 From: Freek van Rijt Date: Tue, 4 Aug 2026 17:15:46 +0200 Subject: [PATCH] feat(installer): let a migration report that it did not finish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A migration that depends on something outside the shop, such as an API call, can fail for a reason that will clear up on its own. Throwing is a poor fit: it aborts the upgrade, and because the installer never records a migration that throws, the shop retries a fatal on every load. Completing quietly is no better, since the work is then never attempted again. Timestamped migrations can now call markFailed() instead. The reason is logged as an error, the migration is left unrecorded so it is picked up on the next load, and the remaining migrations still run — one that could not finish should not hold up the rest of the upgrade. Added to TimestampedMigrationInterface rather than a separate contract: AbstractTimestampedMigration is its only implementer, so nothing outside it has to change. Part of INT-1695 Co-Authored-By: Claude Opus 5 (1M context) --- .../TimestampedMigrationInterface.php | 13 +++++ .../AbstractTimestampedMigration.php | 30 ++++++++++ .../Installer/Service/InstallerService.php | 14 ++++- .../MockFailingTimestampedMigration.php | 31 +++++++++++ .../Service/InstallerServiceTest.php | 55 +++++++++++++++++++ 5 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 tests/Bootstrap/MockFailingTimestampedMigration.php diff --git a/src/App/Installer/Contract/TimestampedMigrationInterface.php b/src/App/Installer/Contract/TimestampedMigrationInterface.php index f53edb586..6b298e98b 100644 --- a/src/App/Installer/Contract/TimestampedMigrationInterface.php +++ b/src/App/Installer/Contract/TimestampedMigrationInterface.php @@ -13,4 +13,17 @@ interface TimestampedMigrationInterface extends MigrationInterface * a timestamp, sorting ids alphabetically also sorts the migrations oldest-to-newest. */ public function getId(): string; + + /** + * Whether the migration ran but did not finish its work. + * + * A migration that depends on something outside the shop — an API call, say — can fail for reasons + * that will clear up on their own. Throwing would abort the upgrade and, because the installer never + * records a migration that throws, leave the shop retrying a fatal on every load. Reporting failure + * instead lets the upgrade continue while keeping the migration unrecorded, so it is picked up again + * next time. + * + * @see \MyParcelNL\Pdk\App\Installer\Migration\AbstractTimestampedMigration::markFailed() + */ + public function hasFailed(): bool; } diff --git a/src/App/Installer/Migration/AbstractTimestampedMigration.php b/src/App/Installer/Migration/AbstractTimestampedMigration.php index 4bd662756..76c1348fb 100644 --- a/src/App/Installer/Migration/AbstractTimestampedMigration.php +++ b/src/App/Installer/Migration/AbstractTimestampedMigration.php @@ -6,6 +6,7 @@ use LogicException; use MyParcelNL\Pdk\App\Installer\Contract\TimestampedMigrationInterface; +use MyParcelNL\Pdk\Facade\Logger; /** * Base for file-based, timestamp-named migrations. @@ -21,6 +22,35 @@ abstract class AbstractTimestampedMigration implements TimestampedMigrationInter /** @var string */ private $id = ''; + /** @var bool */ + private $failed = false; + + /** + * @inheritDoc + */ + public function hasFailed(): bool + { + return $this->failed; + } + + /** + * Report that this run did not finish, so the installer leaves the migration unrecorded. + * + * Call this instead of throwing when the work could not be completed for a reason that may resolve + * itself, so the upgrade carries on and the migration is attempted again on the next load. The reason + * is logged as an error, because a migration that quietly keeps failing is worse than one that fails + * loudly. + * + * @param string $reason What could not be done, in terms a reader of the log will understand + * @param array $context Extra detail for the log entry + */ + protected function markFailed(string $reason, array $context = []): void + { + $this->failed = true; + + Logger::error($reason, $context + ['migration' => $this->id]); + } + /** * Called by the InstallerService loader once the migration file has been required. * Anonymous-class migrations cannot know their own filename, so identity is injected. diff --git a/src/App/Installer/Service/InstallerService.php b/src/App/Installer/Service/InstallerService.php index b8a665f86..388732515 100644 --- a/src/App/Installer/Service/InstallerService.php +++ b/src/App/Installer/Service/InstallerService.php @@ -556,8 +556,20 @@ private function runUpMigrations(Collection $migrations): void } $migration->up(); - $this->markMigrationApplied($migration); $ran[] = $id; + + // A migration that reports failure is deliberately left unrecorded, so it runs again on + // the next load. The remaining migrations still run: one that could not finish should not + // hold up the rest of the upgrade. + if ($migration instanceof TimestampedMigrationInterface && $migration->hasFailed()) { + Logger::warning('Migration did not finish and will be attempted again.', [ + 'migration' => $id, + ]); + + return; + } + + $this->markMigrationApplied($migration); }); } } diff --git a/tests/Bootstrap/MockFailingTimestampedMigration.php b/tests/Bootstrap/MockFailingTimestampedMigration.php new file mode 100644 index 000000000..f378e1759 --- /dev/null +++ b/tests/Bootstrap/MockFailingTimestampedMigration.php @@ -0,0 +1,31 @@ +setIdentity('2025_01_01_000000_mock_failing'); + } + + public function up(): void + { + if (isset($GLOBALS['__migration_order'])) { + $GLOBALS['__migration_order'][] = $this->getId(); + } + + $this->markFailed('Mock migration could not finish.', ['reason' => 'test']); + } +} diff --git a/tests/Unit/App/Installer/Service/InstallerServiceTest.php b/tests/Unit/App/Installer/Service/InstallerServiceTest.php index 2498c6fd9..f3d433908 100644 --- a/tests/Unit/App/Installer/Service/InstallerServiceTest.php +++ b/tests/Unit/App/Installer/Service/InstallerServiceTest.php @@ -509,6 +509,61 @@ public function up(): void unset($GLOBALS['__migration_order']); }); +it('leaves a migration that reports failure unrecorded, so it runs again', function () { + /** @var PdkSettingsRepositoryInterface $settingsRepository */ + $settingsRepository = Pdk::get(PdkSettingsRepositoryInterface::class); + $installedVersionKey = Pdk::get('settingKeyInstalledVersion'); + $appliedMigrationsKey = Pdk::get('settingKeyAppliedMigrations'); + + $settingsRepository->store($installedVersionKey, '1.1.0'); + $settingsRepository->store($appliedMigrationsKey, null); + + \MyParcelNL\Pdk\Tests\Bootstrap\MockMigrationService::addUpgradeMigration( + \MyParcelNL\Pdk\Tests\Bootstrap\MockFailingTimestampedMigration::class + ); + + Installer::install(); + + // Recording it would strand the shop: nothing would ever attempt the work again. + expect($settingsRepository->get($appliedMigrationsKey)) + ->not->toContain('2025_01_01_000000_mock_failing'); +}); + +it('keeps running later migrations after one reports failure', function () { + /** @var PdkSettingsRepositoryInterface $settingsRepository */ + $settingsRepository = Pdk::get(PdkSettingsRepositoryInterface::class); + $installedVersionKey = Pdk::get('settingKeyInstalledVersion'); + $appliedMigrationsKey = Pdk::get('settingKeyAppliedMigrations'); + + $settingsRepository->store($installedVersionKey, '1.1.0'); + $settingsRepository->store($appliedMigrationsKey, null); + + // The failing one sorts first by id, so the other only runs if failure does not halt the pass. + \MyParcelNL\Pdk\Tests\Bootstrap\MockMigrationService::addUpgradeMigration( + \MyParcelNL\Pdk\Tests\Bootstrap\MockFailingTimestampedMigration::class + ); + \MyParcelNL\Pdk\Tests\Bootstrap\MockMigrationService::addUpgradeMigration( + \MyParcelNL\Pdk\Tests\Bootstrap\MockTimestampedMigration20260101::class + ); + + $GLOBALS['__migration_order'] = []; + + Installer::install(); + + $order = $GLOBALS['__migration_order']; + + expect($order) + ->toContain('2025_01_01_000000_mock_failing') + ->toContain('2026_01_01_000000_mock_timestamped'); + + // The one that succeeded is still recorded, so only the failure is retried. + expect($settingsRepository->get($appliedMigrationsKey)) + ->toContain('2026_01_01_000000_mock_timestamped') + ->not->toContain('2025_01_01_000000_mock_failing'); + + unset($GLOBALS['__migration_order']); +}); + it('runs a new timestamp migration even when current version is an RC below installed version', function () { // Simulate the WC test environment: installed is 1.3.0, but this build reports 1.3.0-rc.999 Pdk::set('appInfo', new AppInfo([