From 47863c26e2e6192486279226fdee610c400569bd Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 01:50:37 +0200 Subject: [PATCH] docs: the upgrade path is reachable for the installations it targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- .../Upgrade/UpgradePathCannotRegressTest.php | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php index b3642d523..126f75222 100644 --- a/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php +++ b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php @@ -227,24 +227,41 @@ private static function isWrappedInATransaction(array $statements): bool * @return string[] */ /** - * Versions declared above the application's own, which therefore cannot be reached. + * Versions declared above the application's own. * * `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 constants have not moved since - * the rewrite was imported, so an installation stamped with them — which is every installation - * this codebase performs, since `Installer` stamps the same value — is never behind, no upgrade - * is ever triggered, and a handler declared above that version never runs. + * `AppInfoInterface::APP_VERSION` and `APP_BUILD` alone. Those two have not moved since the + * rewrite was imported, so they still say `400.21031301`, below both migrations. * - * Both entries below are in that state. They are listed rather than merely tolerated so that a - * migration added tomorrow cannot quietly join them: the test asserts these two are *still* - * unreachable, and that nothing else is. + * That reads like the upgrade path is dead. It is not, and the difference matters before + * anybody "fixes" it by bumping the constant. * - * Resolving them is a deployment decision rather than a test change. 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 these two accepted as applying only to - * installations arriving from 3.2. + * An installation arriving from 3.2 stores something like `320.19012701`. That is behind + * `400.21031301`, so the upgrade is triggered, and `getTargetUpgradeHandlers()` selects every + * handler declared above the *stored* version — which is both of these. The population these + * migrations were written for reaches them. Measured against the real class rather than + * reasoned about: `checkVersion('320.19012701', '400.24210101')` is true, and so is the same + * question for `400.24240101`. + * + * What cannot reach them is an installation this rewrite performed itself, which `Installer` + * stamps 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 rather than + * double-encoded. + * + * Bumping APP_BUILD would make things worse rather than better. `40024210101.sql` would then + * run against schemas that already have it applied — it drops a column that is not there — + * and, more seriously, `40024240101.sql` decodes stored text and says in its own header that + * it is not idempotent and that no decode of stored text could be. Running it on an + * installation whose text is already correct turns a name somebody typed as `Q&A` into + * `Q&A`. The narrow population that would benefit — installed by this rewrite before the + * escaping change — is smaller than the population that would be damaged. + * + * So the two are listed rather than resolved, and the list is what stops a third joining them + * unnoticed: the test asserts these two are still above the application's version, and that + * nothing else is. A migration written for a version this codebase never stamps has to be a + * decision, not an accident. */ private const VERSIONS_THE_APPLICATION_CANNOT_REACH = ['400.24210101', '400.24240101'];