Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/App/Installer/Contract/TimestampedMigrationInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
30 changes: 30 additions & 0 deletions src/App/Installer/Migration/AbstractTimestampedMigration.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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]);
Comment thread
FreekVR marked this conversation as resolved.
}

/**
* Called by the InstallerService loader once the migration file has been required.
* Anonymous-class migrations cannot know their own filename, so identity is injected.
Expand Down
14 changes: 13 additions & 1 deletion src/App/Installer/Service/InstallerService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment thread
FreekVR marked this conversation as resolved.

$this->markMigrationApplied($migration);
});
}
}
31 changes: 31 additions & 0 deletions tests/Bootstrap/MockFailingTimestampedMigration.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

namespace MyParcelNL\Pdk\Tests\Bootstrap;

use MyParcelNL\Pdk\App\Installer\Migration\AbstractTimestampedMigration;

/**
* A migration that runs but reports it could not finish, the way one depending on an API call would.
*
* Its id sorts before MockTimestampedMigration20260101, so tests can assert that a migration which fails
* does not stop the ones after it from running.
*/
final class MockFailingTimestampedMigration extends AbstractTimestampedMigration
{
public function __construct()
{
// In production code, setIdentity is called by the InstallerService loader.
$this->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']);
}
}
55 changes: 55 additions & 0 deletions tests/Unit/App/Installer/Service/InstallerServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Comment thread
FreekVR marked this conversation as resolved.
/** @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([
Expand Down
Loading