From f606ec1f7e1cce5926fa22511b77261de5761f5e Mon Sep 17 00:00:00 2001 From: mscherer Date: Sat, 28 Mar 2026 13:06:30 +0100 Subject: [PATCH] Fix checkSync() to only consider migrations from current context 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 --- src/Command/BakeMigrationDiffCommand.php | 60 +++++++++++++++---- .../Command/BakeMigrationDiffCommandTest.php | 47 +++++++++++++++ 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index d0490daf..920af937 100644 --- a/src/Command/BakeMigrationDiffCommand.php +++ b/src/Command/BakeMigrationDiffCommand.php @@ -33,6 +33,7 @@ use Cake\Event\Event; use Cake\Event\EventManager; use Error; +use Migrations\Db\Adapter\UnifiedMigrationsTableStorage; use Migrations\Migration\ManagerFactory; use Migrations\Util\TableFinder; use Migrations\Util\UtilTrait; @@ -169,13 +170,18 @@ protected function setup(Arguments $args): void $migratedItems = []; if ($tableExists) { - $query = $connection->selectQuery(); - /** @var array $migratedItems */ - $migratedItems = $query + $query = $connection->selectQuery() ->select(['version']) ->from($this->phinxTable) - ->orderBy(['version DESC']) - ->execute()->fetchAll('assoc'); + ->orderBy(['version DESC']); + + // In unified table mode, filter by the current plugin context + if ($this->phinxTable === UnifiedMigrationsTableStorage::TABLE_NAME) { + $query->where(['plugin IS' => $this->plugin]); + } + + /** @var array $migratedItems */ + $migratedItems = $query->execute()->fetchAll('assoc'); } $this->migratedItems = $migratedItems; @@ -484,22 +490,56 @@ protected function getIndexes(): void /** * Checks that the migrations history is in sync with the migrations files * + * Compares the last migrated version against the last migration file, + * but only considers migrations that belong to the current context (app or plugin). + * This avoids false positives when migrations from other plugins have been + * run more recently than the last app migration. + * + * @see https://github.com/cakephp/migrations/issues/1060 * @return bool Whether migrations history is sync or not */ protected function checkSync(): bool { + // No files and no migrations - nothing to check if (!$this->migrationsFiles && !$this->migratedItems) { return true; } - if ($this->migratedItems) { - $lastVersion = $this->migratedItems[0]['version']; - $lastFile = end($this->migrationsFiles); + // No files in this context - nothing to sync + // This allows baking a snapshot for a new plugin even when + // the unified migrations table has records from other contexts + if (!$this->migrationsFiles) { + return true; + } - return $lastFile && str_contains($lastFile, (string)$lastVersion); + // Have files - need to verify they're synced + // Extract version numbers from current context's migration files + $fileVersions = []; + foreach ($this->migrationsFiles as $file) { + $filename = basename($file); + if (preg_match('/^(\d+)_/', $filename, $matches)) { + $fileVersions[] = $matches[1]; + } + } + + // Filter migrated versions to only those that have corresponding files in this context + $contextMigratedVersions = []; + foreach ($this->migratedItems as $item) { + if (in_array((string)$item['version'], $fileVersions, true)) { + $contextMigratedVersions[] = (string)$item['version']; + } } - return false; + if (empty($contextMigratedVersions)) { + // No migrations from this context have been run yet + return false; + } + + // Get the most recent migrated version within this context + $lastVersion = max($contextMigratedVersions); + $lastFile = end($this->migrationsFiles); + + return $lastFile && str_contains($lastFile, $lastVersion); } /** diff --git a/tests/TestCase/Command/BakeMigrationDiffCommandTest.php b/tests/TestCase/Command/BakeMigrationDiffCommandTest.php index 2dc4d388..963571e1 100644 --- a/tests/TestCase/Command/BakeMigrationDiffCommandTest.php +++ b/tests/TestCase/Command/BakeMigrationDiffCommandTest.php @@ -142,6 +142,53 @@ public function testHistoryNotInSync() $this->assertExitCode(BaseCommand::CODE_ERROR); } + /** + * Tests that baking a diff succeeds when app migrations are in sync + * even if a plugin has more recent migrations. + * + * This test specifically tests the unified table mode where all migrations + * (app and plugins) are stored in a single cake_migrations table. + * + * @see https://github.com/cakephp/migrations/issues/1060 + * @return void + */ + public function testCheckSyncWithPluginMigrationsMoreRecent(): void + { + // This bug only affects unified table mode + Configure::write('Migrations.legacyTables', false); + + // Run app migrations first to get them in sync + $this->exec('migrations migrate -c test --no-lock'); + $this->assertExitSuccess(); + + // Create a schema dump file (required for baking a diff) + $this->exec('migrations dump -c test'); + $this->assertExitSuccess(); + + // Insert a more recent migration record for a plugin + // This simulates having a plugin migration that was run after the last app migration + $this->insertMigrationRecord('test', 99999999999999, 'PluginMigration', 'SomePlugin'); + + // Now try to bake a diff for the app - this should succeed + // because all app migration files have been migrated + $this->exec('bake migration_diff CheckSyncTest -c test'); + + // Should not contain the "not in sync" error + $this->assertOutputNotContains('Your migrations history is not in sync'); + $this->assertExitSuccess(); + + // Clean up generated files + $path = ROOT . DS . 'config' . DS . 'Migrations' . DS; + $this->generatedFiles = array_merge( + $this->generatedFiles, + glob($path . '*_CheckSyncTest.php') ?: [], + ); + $this->generatedFiles[] = $path . 'schema-dump-test.lock'; + + // Restore legacy table mode + Configure::delete('Migrations.legacyTables'); + } + /** * Tests that baking a diff while history is empty and no migration files exists * will fall back to baking a snapshot