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([