fix: apply the CustomFieldData migration as one statement - #822
Merged
Conversation
`40024210101.sql` gives CustomFieldData the identity it should always have had,
dropping the surrogate `id` and making (moduleId, itemId, definitionId) the
primary key. It did so as two statements, and DDL commits as it goes, so those
are two commits:
drop column id -> succeeds
add primary key -> ERROR 1062 Duplicate entry '1-1-1' for key 'PRIMARY'
retry -> ERROR 1091 Can't DROP COLUMN `id`
Any installation holding a duplicate of that triple — which the surrogate key
allowed — loses the column, keeps its old database version because
`UpgradeDatabase::apply()` throws before writing it, and then fails every retry
on the already-applied statement before reaching the one that failed. The
upgrade can neither be finished nor repeated.
As one ALTER the server applies both or neither: the duplicate still refuses it,
the table is left as it was, and once the operator clears the duplicates the
same file succeeds.
`MigrationIsAtomicTest` runs the real file through the real parser against a
real server, on a table shaped the way an installation's is before the
migration. The unit test hard-coded both statements and errored the moment they
became one, which is what it is there for; it now pins the single statement and
points at the new test for the reason.
blaipr
added a commit
that referenced
this pull request
Aug 18, 2026
Two guards over the way #822 stopped being repeatable, both of which fail today if the thing they describe is undone. A migration that is refused part way has to leave the operator something to run again. `40024210101.sql` gave CustomFieldData its natural primary key as two statements; DDL commits as it goes, so the drop stood alone when the key was refused on a duplicate, and every retry then died on `Can't DROP COLUMN id` before reaching the statement that had failed. Each version file must now be a single statement, or wholly inside one transaction with no DDL in it — DDL would commit and end the transaction under the statements after it — or written so that running it twice is harmless. Both files satisfy it, one each way. `Upgrade::getTargetUpgradeHandlers()` yields a handler's versions in the order `getAttributes()` returns them, which is the order they are written in the class. Nothing sorts them. They ascend today, so the schema change runs before the data migration that needs it; a version added at the top of the list would run first. Out-of-order migrations are not something an installation reports. The messages say what to do about it rather than only what is wrong, because whoever sees them is adding a migration and has a choice of three ways to satisfy the rule. CLAUDE.md records the pattern these two guard, which covers half of the recent fixes: a guard that is not where the change happens — a limit tested against a row already read, a counter written back from a stale read, a hash stored outside the transaction holding what it describes, two commits made out of one logical change. Where the codebase gets it right the guard and the change are one statement. The suite counts there are refreshed to match.
blaipr
added a commit
that referenced
this pull request
Aug 18, 2026
Two guards over the way #822 stopped being repeatable, both of which fail today if the thing they describe is undone. A migration that is refused part way has to leave the operator something to run again. `40024210101.sql` gave CustomFieldData its natural primary key as two statements; DDL commits as it goes, so the drop stood alone when the key was refused on a duplicate, and every retry then died on `Can't DROP COLUMN id` before reaching the statement that had failed. Each version file must now be a single statement, or wholly inside one transaction with no DDL in it — DDL would commit and end the transaction under the statements after it — or written so that running it twice is harmless. Both files satisfy it, one each way. `Upgrade::getTargetUpgradeHandlers()` yields a handler's versions in the order `getAttributes()` returns them, which is the order they are written in the class. Nothing sorts them. They ascend today, so the schema change runs before the data migration that needs it; a version added at the top of the list would run first. Out-of-order migrations are not something an installation reports. The messages say what to do about it rather than only what is wrong, because whoever sees them is adding a migration and has a choice of three ways to satisfy the rule. CLAUDE.md records the pattern these two guard, which covers half of the recent fixes: a guard that is not where the change happens — a limit tested against a row already read, a counter written back from a stale read, a hash stored outside the transaction holding what it describes, two commits made out of one logical change. Where the codebase gets it right the guard and the change are one statement. The suite counts there are refreshed to match.
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
1 participant
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.
40024210101.sqlgivesCustomFieldDatathe identity it should always have had — dropping the surrogateid, making(moduleId, itemId, definitionId)the primary key. It did so as two statements, and DDL commits as it goes, so those are two commits.Reproduced against a real server, on a table holding a duplicate of that triple — which the surrogate key allowed:
UpgradeDatabase::apply()throws before writing the new database version, which is correct — but it means the version stays old and the upgrade is retried from the top, where it now dies on the already-applied statement before ever reaching the one that failed. The installation can neither finish the upgrade nor repeat it, and the column it needed is gone.One ALTER
As a single statement the server applies both clauses or neither. Verified: the same duplicate still refuses it,
idis still there afterwards, and once the operator clears the duplicates the identical file succeeds and the composite key is in place.Nothing else changes — the migration is keyed by database version, so an installation that has already run it will not run it again.
Tests
MigrationIsAtomicTestruns the real file through the real parser (MysqlFileParser) against a real server, on a table shaped the way an installation's is before the migration. It asserts the refusal leaves the table exactly as it was — because that is what leaves the operator something to run again — then clears the duplicate and asserts the same file applies.Reverting the file fails it on precisely that: the migration was refused, so the table must be as it was — with
idstill there, or the operator has nothing left to run again.The existing unit test hard-coded both statements and errored the moment they became one, which is what it is there for. It now pins the single statement and carries the reason.
3967 unit tests pass; PHPStan level 6 on
srcand PHPCS clean.