Skip to content

Commit e9baed9

Browse files
authored
Fix checkSync() to only consider migrations from current context (#1061)
When using the unified `cake_migrations` table, `BakeMigrationDiffCommand::checkSync()` was comparing the last migrated version (from ALL contexts including plugins) against the last migration file in the current context (app or specific plugin). This caused false "not in sync" errors when a plugin had more recent migrations than the app being baked. The fix filters migrated versions to only consider those that have corresponding files in the current context before comparing. Fixes #1060
1 parent e485847 commit e9baed9

File tree

2 files changed

+97
-10
lines changed

2 files changed

+97
-10
lines changed

src/Command/BakeMigrationDiffCommand.php

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
use Cake\Event\Event;
3434
use Cake\Event\EventManager;
3535
use Error;
36+
use Migrations\Db\Adapter\UnifiedMigrationsTableStorage;
3637
use Migrations\Migration\ManagerFactory;
3738
use Migrations\Util\TableFinder;
3839
use Migrations\Util\UtilTrait;
@@ -169,13 +170,18 @@ protected function setup(Arguments $args): void
169170

170171
$migratedItems = [];
171172
if ($tableExists) {
172-
$query = $connection->selectQuery();
173-
/** @var array $migratedItems */
174-
$migratedItems = $query
173+
$query = $connection->selectQuery()
175174
->select(['version'])
176175
->from($this->phinxTable)
177-
->orderBy(['version DESC'])
178-
->execute()->fetchAll('assoc');
176+
->orderBy(['version DESC']);
177+
178+
// In unified table mode, filter by the current plugin context
179+
if ($this->phinxTable === UnifiedMigrationsTableStorage::TABLE_NAME) {
180+
$query->where(['plugin IS' => $this->plugin]);
181+
}
182+
183+
/** @var array $migratedItems */
184+
$migratedItems = $query->execute()->fetchAll('assoc');
179185
}
180186

181187
$this->migratedItems = $migratedItems;
@@ -484,22 +490,56 @@ protected function getIndexes(): void
484490
/**
485491
* Checks that the migrations history is in sync with the migrations files
486492
*
493+
* Compares the last migrated version against the last migration file,
494+
* but only considers migrations that belong to the current context (app or plugin).
495+
* This avoids false positives when migrations from other plugins have been
496+
* run more recently than the last app migration.
497+
*
498+
* @see https://github.com/cakephp/migrations/issues/1060
487499
* @return bool Whether migrations history is sync or not
488500
*/
489501
protected function checkSync(): bool
490502
{
503+
// No files and no migrations - nothing to check
491504
if (!$this->migrationsFiles && !$this->migratedItems) {
492505
return true;
493506
}
494507

495-
if ($this->migratedItems) {
496-
$lastVersion = $this->migratedItems[0]['version'];
497-
$lastFile = end($this->migrationsFiles);
508+
// No files in this context - nothing to sync
509+
// This allows baking a snapshot for a new plugin even when
510+
// the unified migrations table has records from other contexts
511+
if (!$this->migrationsFiles) {
512+
return true;
513+
}
498514

499-
return $lastFile && str_contains($lastFile, (string)$lastVersion);
515+
// Have files - need to verify they're synced
516+
// Extract version numbers from current context's migration files
517+
$fileVersions = [];
518+
foreach ($this->migrationsFiles as $file) {
519+
$filename = basename($file);
520+
if (preg_match('/^(\d+)_/', $filename, $matches)) {
521+
$fileVersions[] = $matches[1];
522+
}
523+
}
524+
525+
// Filter migrated versions to only those that have corresponding files in this context
526+
$contextMigratedVersions = [];
527+
foreach ($this->migratedItems as $item) {
528+
if (in_array((string)$item['version'], $fileVersions, true)) {
529+
$contextMigratedVersions[] = (string)$item['version'];
530+
}
500531
}
501532

502-
return false;
533+
if (empty($contextMigratedVersions)) {
534+
// No migrations from this context have been run yet
535+
return false;
536+
}
537+
538+
// Get the most recent migrated version within this context
539+
$lastVersion = max($contextMigratedVersions);
540+
$lastFile = end($this->migrationsFiles);
541+
542+
return $lastFile && str_contains($lastFile, $lastVersion);
503543
}
504544

505545
/**

tests/TestCase/Command/BakeMigrationDiffCommandTest.php

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,53 @@ public function testHistoryNotInSync()
142142
$this->assertExitCode(BaseCommand::CODE_ERROR);
143143
}
144144

145+
/**
146+
* Tests that baking a diff succeeds when app migrations are in sync
147+
* even if a plugin has more recent migrations.
148+
*
149+
* This test specifically tests the unified table mode where all migrations
150+
* (app and plugins) are stored in a single cake_migrations table.
151+
*
152+
* @see https://github.com/cakephp/migrations/issues/1060
153+
* @return void
154+
*/
155+
public function testCheckSyncWithPluginMigrationsMoreRecent(): void
156+
{
157+
// This bug only affects unified table mode
158+
Configure::write('Migrations.legacyTables', false);
159+
160+
// Run app migrations first to get them in sync
161+
$this->exec('migrations migrate -c test --no-lock');
162+
$this->assertExitSuccess();
163+
164+
// Create a schema dump file (required for baking a diff)
165+
$this->exec('migrations dump -c test');
166+
$this->assertExitSuccess();
167+
168+
// Insert a more recent migration record for a plugin
169+
// This simulates having a plugin migration that was run after the last app migration
170+
$this->insertMigrationRecord('test', 99999999999999, 'PluginMigration', 'SomePlugin');
171+
172+
// Now try to bake a diff for the app - this should succeed
173+
// because all app migration files have been migrated
174+
$this->exec('bake migration_diff CheckSyncTest -c test');
175+
176+
// Should not contain the "not in sync" error
177+
$this->assertOutputNotContains('Your migrations history is not in sync');
178+
$this->assertExitSuccess();
179+
180+
// Clean up generated files
181+
$path = ROOT . DS . 'config' . DS . 'Migrations' . DS;
182+
$this->generatedFiles = array_merge(
183+
$this->generatedFiles,
184+
glob($path . '*_CheckSyncTest.php') ?: [],
185+
);
186+
$this->generatedFiles[] = $path . 'schema-dump-test.lock';
187+
188+
// Restore legacy table mode
189+
Configure::delete('Migrations.legacyTables');
190+
}
191+
145192
/**
146193
* Tests that baking a diff while history is empty and no migration files exists
147194
* will fall back to baking a snapshot

0 commit comments

Comments
 (0)