From 9648a243cc56bd7d0c7dc5e22caa089046b0c5c0 Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Mon, 14 Sep 2026 17:01:50 +0200 Subject: [PATCH 1/2] Group migration sets that share a migration history Migrator::runMany() decided whether to drop tables once per migration set. A migration history is keyed by connection and plugin only, never by source, so two sets differing only in their source write to one history while each of them sees just its own directory on disk. Every set therefore reported its siblings' applied migrations as missing, and the connection was wiped on every run even when the database was already up to date. Sets are now grouped by the history they share, and a logged migration found on disk in any set of the group no longer counts as missing. testRunManyMultipleSkip relied on that spurious drop to trigger its failure, so it now forgets an applied migration to give the second run a genuine reason to drop. It no longer has to be skipped when the unified table is in use. --- src/TestSuite/Migrator.php | 69 +++++++++++++++- tests/TestCase/TestSuite/MigratorTest.php | 95 +++++++++++++++++++---- 2 files changed, 145 insertions(+), 19 deletions(-) diff --git a/src/TestSuite/Migrator.php b/src/TestSuite/Migrator.php index dd160249..4e7eaf97 100644 --- a/src/TestSuite/Migrator.php +++ b/src/TestSuite/Migrator.php @@ -25,6 +25,14 @@ class Migrator { protected ConnectionHelper $helper; + /** + * Migration ids found on disk in the sources that share a migration history + * with the set currently being inspected. + * + * @var array + */ + protected array $siblingMigrationIds = []; + /** * Constructor. */ @@ -84,6 +92,8 @@ public function runMany( // Detect all connections involved, and mark those with changed status. $connectionsToDrop = []; $connectionsList = []; + $sets = []; + $historyGroups = []; foreach ($options as $i => $migrationSet) { $migrationSet += ['connection' => 'test']; $skip = $migrationSet['skip'] ?? []; @@ -95,11 +105,33 @@ public function runMany( $connectionsList[$connectionName] = ['name' => $connectionName, 'skip' => $skip]; } + // Sets sharing a connection and a plugin also share a migration history, + // while each of them only has its own source directory on disk. Group + // them so that a set is not told that its siblings' applied migrations + // are missing. + $groupKey = $connectionName . '|' . ($migrationSet['plugin'] ?? ''); + $historyGroups[$groupKey][] = $migrationSet; + $sets[] = ['options' => $migrationSet, 'skip' => $skip, 'group' => $groupKey]; + } + + $groupMigrationIds = []; + foreach ($historyGroups as $groupKey => $group) { + $groupMigrationIds[$groupKey] = $this->getSiblingMigrationIds($group); + } + + foreach ($sets as $set) { + $connectionName = $set['options']['connection']; + if (isset($connectionsToDrop[$connectionName])) { + continue; + } + + $this->siblingMigrationIds = $groupMigrationIds[$set['group']]; $migrations = new Migrations(); - if (!isset($connectionsToDrop[$connectionName]) && $this->shouldDropTables($migrations, $migrationSet)) { - $connectionsToDrop[$connectionName] = ['name' => $connectionName, 'skip' => $skip]; + if ($this->shouldDropTables($migrations, $set['options'])) { + $connectionsToDrop[$connectionName] = ['name' => $connectionName, 'skip' => $set['skip']]; } } + $this->siblingMigrationIds = []; foreach ($connectionsToDrop as $item) { $this->dropTables($item['name'], $item['skip']); @@ -158,6 +190,35 @@ public function truncate(string $connection, array $skip = []): void } } + /** + * Collect the migration ids that exist on disk for a group of migration sets + * that share a single migration history. + * + * Returns an empty list for groups of one, where no sibling source exists. + * + * @param array> $group Migration sets sharing a history. + * @return array Migration ids present on disk, keyed by id. + */ + protected function getSiblingMigrationIds(array $group): array + { + if (count($group) < 2) { + return []; + } + + $ids = []; + foreach ($group as $options) { + $migrations = new Migrations(); + foreach ($migrations->status($options) as $migration) { + if ($migration['missing'] ?? false) { + continue; + } + $ids[$migration['id']] = true; + } + } + + return $ids; + } + /** * Detect if migrations have changed and the database needs to be wiped. * @@ -175,6 +236,10 @@ protected function shouldDropTables(Migrations $migrations, array $options): boo ]; foreach ($migrations->status($options) as $migration) { if ($migration['status'] === 'up' && ($migration['missing'] ?? false)) { + // The migration belongs to another source sharing this history. + if (isset($this->siblingMigrationIds[$migration['id']])) { + continue; + } $messages['missing'][] = 'Applied but, missing Migration ' . sprintf('source=%s id=%s', $migration['name'], $migration['id']); } diff --git a/tests/TestCase/TestSuite/MigratorTest.php b/tests/TestCase/TestSuite/MigratorTest.php index cf2516c1..3a980440 100644 --- a/tests/TestCase/TestSuite/MigratorTest.php +++ b/tests/TestCase/TestSuite/MigratorTest.php @@ -33,27 +33,33 @@ class MigratorTest extends TestCase protected $restore; /** - * Get the migration table name for the Migrator plugin. + * Get the migration table name for a plugin, or for the application when null. * + * @param string|null $plugin The plugin name. * @return string */ - protected function getMigratorTableName(): string + protected function getMigratorTableName(?string $plugin = 'Migrator'): string { - return Configure::read('Migrations.legacyTables') === false - ? UnifiedMigrationsTableStorage::TABLE_NAME - : 'migrator_phinxlog'; + if (Configure::read('Migrations.legacyTables') === false) { + return UnifiedMigrationsTableStorage::TABLE_NAME; + } + + return $plugin === null ? 'phinxlog' : 'migrator_phinxlog'; } /** * Build a WHERE clause for filtering by plugin in unified mode. * + * @param string|null $plugin The plugin name. * @return array */ - protected function getMigratorWhereClause(): array + protected function getMigratorWhereClause(?string $plugin = 'Migrator'): array { - return Configure::read('Migrations.legacyTables') === false - ? ['plugin' => 'Migrator'] - : []; + if (Configure::read('Migrations.legacyTables') !== false) { + return []; + } + + return $plugin === null ? ['plugin IS' => null] : ['plugin' => $plugin]; } protected function makeInspectableMigrator(): Migrator @@ -218,8 +224,6 @@ public function testRunManyMultipleSkip(): void { $connection = ConnectionManager::get('test'); $this->skipIf($connection->getDriver() instanceof Postgres); - // Skip for unified mode - migration history detection works differently - $this->skipIf(Configure::read('Migrations.legacyTables') === false); $migrator = new Migrator(); // Run migrations for the first time. @@ -228,6 +232,9 @@ public function testRunManyMultipleSkip(): void ['plugin' => 'Migrator', 'source' => 'Migrations2'], ]); + // Forget one applied migration so that the next run has a reason to drop. + $this->forgetMigration('20211001000000'); + // Run migrations the second time. Skip clauses will cause problems. try { $migrator->runMany([ @@ -253,24 +260,35 @@ public function testTruncateAfterMigrations(): void $this->assertCount(0, $connection->selectQuery()->select(['*'])->from('migrator')->execute()->fetchAll()); } - private function setMigrationEndDateToYesterday(): void + private function setMigrationEndDateToYesterday(?string $plugin = 'Migrator'): void { $query = ConnectionManager::get('test')->updateQuery() - ->update($this->getMigratorTableName()) + ->update($this->getMigratorTableName($plugin)) ->set('end_time', ChronosDate::yesterday(), 'timestamp'); - $where = $this->getMigratorWhereClause(); + $where = $this->getMigratorWhereClause($plugin); if ($where) { $query->where($where); } $query->execute(); } - private function fetchMigrationEndDate(): ChronosDate + private function forgetMigration(string $version): void + { + $query = ConnectionManager::get('test')->deleteQuery() + ->delete($this->getMigratorTableName()) + ->where(['version' => $version]); + foreach ($this->getMigratorWhereClause() as $field => $value) { + $query->where([$field => $value]); + } + $query->execute(); + } + + private function fetchMigrationEndDate(?string $plugin = 'Migrator'): ChronosDate { $query = ConnectionManager::get('test')->selectQuery() ->select('end_time') - ->from($this->getMigratorTableName()); - $where = $this->getMigratorWhereClause(); + ->from($this->getMigratorTableName($plugin)); + $where = $this->getMigratorWhereClause($plugin); if ($where) { $query->where($where); } @@ -323,6 +341,49 @@ public function testSkipMigrationDroppingIfOnlyUpMigrationsWithTwoSetsOfMigratio $this->assertTrue($this->fetchMigrationEndDate()->isYesterday()); } + public function testSkipMigrationDroppingWithTwoSourcesSharingHistory(): void + { + // Both sets resolve to the same migration history: same connection and + // same plugin. Each source only sees its own files on disk, so the + // other source's applied migrations must not be treated as missing. + $sets = [ + ['plugin' => 'Migrator'], + ['plugin' => 'Migrator', 'source' => 'Migrations2'], + ]; + + $migrator = new Migrator(); + $migrator->runMany($sets, false); + + $this->setMigrationEndDateToYesterday(); + + $migrator->runMany($sets, false); + + // Ensure that the end time is unchanged, meaning that the tables were not + // dropped and the migrations were not re-run. + $this->assertTrue($this->fetchMigrationEndDate()->isYesterday()); + } + + public function testSkipMigrationDroppingWithTwoAppSourcesSharingHistory(): void + { + // Two application sources on the same connection: no plugin, so both + // record into the same migration history. + $sets = [ + ['source' => '../Plugin/Migrator/config/Migrations'], + ['source' => '../Plugin/Migrator/config/Migrations2'], + ]; + + $migrator = new Migrator(); + $migrator->runMany($sets, false); + + $this->setMigrationEndDateToYesterday(null); + + $migrator->runMany($sets, false); + + // Ensure that the end time is unchanged, meaning that the tables were not + // dropped and the migrations were not re-run. + $this->assertTrue($this->fetchMigrationEndDate(null)->isYesterday()); + } + public function testDropMigrationsIfDownMigrations(): void { // Skip for unified mode - migration history detection works differently From 1347675fa440c9b389155f34854cb2bcdecb65be Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Tue, 15 Sep 2026 16:22:02 +0200 Subject: [PATCH 2/2] Pass sibling migration IDs explicitly --- src/TestSuite/Migrator.php | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/TestSuite/Migrator.php b/src/TestSuite/Migrator.php index 4e7eaf97..d62397c2 100644 --- a/src/TestSuite/Migrator.php +++ b/src/TestSuite/Migrator.php @@ -25,14 +25,6 @@ class Migrator { protected ConnectionHelper $helper; - /** - * Migration ids found on disk in the sources that share a migration history - * with the set currently being inspected. - * - * @var array - */ - protected array $siblingMigrationIds = []; - /** * Constructor. */ @@ -125,13 +117,11 @@ public function runMany( continue; } - $this->siblingMigrationIds = $groupMigrationIds[$set['group']]; $migrations = new Migrations(); - if ($this->shouldDropTables($migrations, $set['options'])) { + if ($this->shouldDropTables($migrations, $set['options'], $groupMigrationIds[$set['group']])) { $connectionsToDrop[$connectionName] = ['name' => $connectionName, 'skip' => $set['skip']]; } } - $this->siblingMigrationIds = []; foreach ($connectionsToDrop as $item) { $this->dropTables($item['name'], $item['skip']); @@ -224,10 +214,14 @@ protected function getSiblingMigrationIds(array $group): array * * @param \Migrations\Migrations $migrations The migrations service. * @param array $options The connection options. + * @param array $siblingMigrationIds Migration ids found in sources sharing the history. * @return bool */ - protected function shouldDropTables(Migrations $migrations, array $options): bool - { + protected function shouldDropTables( + Migrations $migrations, + array $options, + array $siblingMigrationIds = [], + ): bool { Log::write('debug', sprintf('Reading migrations status for %s...', $options['connection'])); $messages = [ @@ -237,7 +231,7 @@ protected function shouldDropTables(Migrations $migrations, array $options): boo foreach ($migrations->status($options) as $migration) { if ($migration['status'] === 'up' && ($migration['missing'] ?? false)) { // The migration belongs to another source sharing this history. - if (isset($this->siblingMigrationIds[$migration['id']])) { + if (isset($siblingMigrationIds[$migration['id']])) { continue; } $messages['missing'][] = 'Applied but, missing Migration ' .