test: a migration declared above the app version can never run - #867
Merged
blaipr merged 1 commit intoAug 23, 2026
Merged
Conversation
`ModuleBase::checkUpgradeNeeded()` asks whether the stored version is behind the version the code reports, and `Version::getVersionStringNormalized()` builds that from `AppInfoInterface::APP_VERSION` and `APP_BUILD` alone. Those two constants have not moved since the rewrite was imported, so they still say `400.21031301` — while both shipped migrations declare `400.24210101` and `400.24240101`, above it. `Installer` stamps the same value it reports, so every installation this codebase performs records `400.21031301` and is never behind. No upgrade is triggered, and neither migration can run. It fails silently and looks exactly like an installation that had nothing to do. This asserts the invariant over every declared version: a handler above the application's own can never be the reason an upgrade happens. The two that are currently in that state are listed by name, in the same shape RoutesAreDispatchableTest uses for the action ids that resolve to no controller — the test asserts those two are *still* unreachable and that nothing else is, so a migration added tomorrow cannot quietly join them. Resolving the two is a deployment decision rather than a test change, and the reasoning is recorded on the constant: bumping APP_BUILD makes both reachable, but `40024210101.sql` drops a column that `dbstructure.sql` already ships without, so it would then run against schemas that already have it applied and fail. The migration has to be made conditional first, or those two accepted as applying only to installations arriving from 3.2. Checked by removing one of the two from the list: the test fails, naming the handler, the version it declares and the version the application reports.
blaipr
deleted the
test/a-migration-declared-above-the-app-version-can-never-run
branch
August 23, 2026 23:05
blaipr
added a commit
that referenced
this pull request
Aug 23, 2026
…873) #867 recorded the two shipped migrations as declaring versions the application can never reach, and left resolving that as a deployment decision. The first half is true and the second was the wrong conclusion, so this corrects the record before somebody acts on it. `AppInfoInterface::APP_BUILD` has not moved since the rewrite was imported, so the code reports `400.21031301` while the migrations declare `400.24210101` and `400.24240101`. But `checkUpgradeNeeded()` compares the *stored* version against that, and `getTargetUpgradeHandlers()` then selects every handler above the stored one. An installation arriving from 3.2 stores something like `320.19012701`, which is behind both — so it triggers the upgrade and runs both migrations. Measured against the real Version class rather than reasoned about. What cannot reach them is an installation this rewrite performed, which is stamped with the version the code reports. Neither migration applies to one: `dbstructure.sql` already ships CustomFieldData with the composite primary key and no `id` column, and text written after the escaping change is stored as typed. Bumping APP_BUILD would make it worse. `40024210101.sql` would run against schemas that already have it applied, dropping a column that is not there; and `40024240101.sql` decodes stored text, saying in its own header that it is not idempotent and that no decode of stored text could be — so on an installation whose text is already correct it turns a name typed as `Q&A` into something else. The population that would benefit is the narrow one installed by this rewrite before the escaping change; the population that would be damaged is every installation since. The two stay on the list, and the list keeps doing the job it was added for: a third migration cannot join them unnoticed. No behaviour changes here — only the reasoning recorded on the constant, which is what the next person will read before touching APP_BUILD.
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.
ModuleBase::checkUpgradeNeeded()asks whether the stored version is behind the versionthe code reports, and
Version::getVersionStringNormalized()builds that fromAppInfoInterface::APP_VERSIONandAPP_BUILDalone. Those two constants have not movedsince the rewrite was imported, so they still say
400.21031301— while both shippedmigrations declare
400.24210101and400.24240101, above it.Installerstamps the same value it reports, so every installation this codebase performsrecords
400.21031301and is never behind. No upgrade is triggered, and neither migrationcan run. It fails silently and looks exactly like an installation that had nothing to do.
This asserts the invariant over every declared version: a handler above the application's
own can never be the reason an upgrade happens. The two that are currently in that state
are listed by name, in the same shape RoutesAreDispatchableTest uses for the action ids
that resolve to no controller — the test asserts those two are still unreachable and
that nothing else is, so a migration added tomorrow cannot quietly join them.
Resolving the two is a deployment decision rather than a test change, and the reasoning is
recorded on the constant: bumping APP_BUILD makes both reachable, but
40024210101.sqldrops a column that
dbstructure.sqlalready ships without, so it would then run againstschemas that already have it applied and fail. The migration has to be made conditional
first, or those two accepted as applying only to installations arriving from 3.2.
Checked by removing one of the two from the list: the test fails, naming the handler, the
version it declares and the version the application reports.