Group migration sets that share a migration history - #1116
Open
dereuromark wants to merge 1 commit into
Open
dereuromark wants to merge 1 commit into
dereuromark wants to merge 1 commit into
Conversation
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.
markstory
reviewed
Sep 15, 2026
| $connectionsToDrop[$connectionName] = ['name' => $connectionName, 'skip' => $set['skip']]; | ||
| } | ||
| } | ||
| $this->siblingMigrationIds = []; |
Member
There was a problem hiding this comment.
Why use an attribute instead of a parameter to shouldDropTables()? There isn't any recursion, and shouldDropTables() only has a single call site.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1115.
Migrator::runMany()evaluatesshouldDropTables()once per migration set, but a migration history is keyed by connection and plugin only.ManagerFactory::createConfig()derives the log table fromUtil::tableName($plugin), and the unifiedcake_migrationstable filters on itsplugincolumn. The source folder never enters either.So two sets that differ only in
sourceshare a single history while each one sees only its own directory on disk.Manager::printStatus()compares the whole log against one source's files and flags everything belonging to the sibling source as applied but missing,shouldDropTables()returns true, and the connection gets dropped and its history truncated on every run, even when the test database is fully up to date. Legacyphinxlogtables are affected the same way as the unified table.The fix groups the sets by the history they share (connection plus plugin) and collects the migration ids present on disk across the whole group. A logged migration found in any source of the group no longer counts as missing. Sets with distinct histories end up alone in their group and keep exactly their previous behavior.
Two points worth a reviewer's opinion:
shouldDropTables()is protected, so I left its signature alone and passed the group information through a new protected property instead of a new argument. Adding a parameter would break any subclass that overrides the method, which seemed worse on a released branch. Happy to switch if you would rather have the explicit argument.status()twice per set: once to collect the ids, once insideshouldDropTables(). Caching it needs the signature change above.testRunManyMultipleSkipwas passing only because of the spurious drop, so it now forgets one applied migration to give the second run a real reason to drop. That also lets it run against the unified table, so itsskipIfis gone.Both new tests fail on 5.x and pass with the fix, in legacy and unified mode.