From f466d3e35779de18803ed2ed5cd9e0b9a2ed3db6 Mon Sep 17 00:00:00 2001 From: rumblefrog Date: Sat, 6 Jun 2026 17:29:26 -0400 Subject: [PATCH] fix(updater): portable information_schema guard for lockout columns (#1498) Migration 801.php added the admin lockout columns with `ALTER TABLE ... ADD IF NOT EXISTS`, a MariaDB-only DDL extension. Stock MySQL (8.x included) and MySQL-compatible engines like Percona Server reject it with SQLSTATE[42000] 1064 mid-upgrade, so panels on those engines could not upgrade past 801 (v2 rc5 -> rc6, v1.8.x -> v2). The dev stack and CI both run MariaDB, which accepts the syntax, so the runtime idempotency test and PHPStan's dba gate both passed it through; the break only surfaced on self-hosters running MySQL / Percona, both first-class supported engines per the docs. Guard each ADD with a portable information_schema existence probe + a plain `ADD COLUMN`. Same idempotent contract, runs on both engines, converges to the same schema. Edited 801.php in place rather than shipping a new migration because the effect is unchanged (the Updater only runs versions above config.version, so MariaDB installs that already ran 801 never re-run it; MySQL installs that fataled never advanced past it and pick up the fixed version on the next pass). Add UpdaterMigrationPortableSqlTest, a static source-scan guard that fails if any v2-era migration (version >= 800) reuses the MariaDB-only `ADD ... IF NOT EXISTS` form. A runtime test can't catch this (the suite runs against MariaDB). Ten pre-800 migrations carry the same syntax but only run on ancient-install -> v2 paths; they're a tracked follow-up, deliberately out of scope here. --- web/phpstan-baseline.neon | 6 - .../UpdaterMigrationPortableSqlTest.php | 136 ++++++++++++++++++ web/updater/data/801.php | 68 +++++++-- 3 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 web/tests/integration/UpdaterMigrationPortableSqlTest.php diff --git a/web/phpstan-baseline.neon b/web/phpstan-baseline.neon index deaa745c0..32a0fe498 100644 --- a/web/phpstan-baseline.neon +++ b/web/phpstan-baseline.neon @@ -515,9 +515,3 @@ parameters: identifier: variable.undefined count: 4 path: updater/data/705.php - - - - message: '#^Variable \$this might not be defined\.$#' - identifier: variable.undefined - count: 4 - path: updater/data/801.php diff --git a/web/tests/integration/UpdaterMigrationPortableSqlTest.php b/web/tests/integration/UpdaterMigrationPortableSqlTest.php new file mode 100644 index 000000000..7bee7f025 --- /dev/null +++ b/web/tests/integration/UpdaterMigrationPortableSqlTest.php @@ -0,0 +1,136 @@ += 5.6 ... 8.0+ is fine and recommended"). + * + * A runtime test can't catch this — it would need a live MySQL to fail on, + * and the suite runs against MariaDB. This is a STATIC source-scan guard: + * no v2-era migration (version >= 800) may use the `ADD ... IF NOT EXISTS` + * conditional-DDL form. The portable shape is an information_schema + * existence probe + a plain `ADD COLUMN` (see 801.php for the reference). + * + * Scope (focused, per #1498) + * -------------------------- + * Only migrations with version >= 800 (the 8xx v2 block, 801-810 today) + * are scanned. Two reasons: + * + * - Every supported upgrade path the issue names (v2 rc5 -> rc6, + * v1.8.x -> v2) starts at `config.version = 705`, so the updater only + * runs 801+. 801 is the first migration past 705 and the actual + * blocker. Future migrations are always numbered above the current + * max (811+), so a `>= 800` floor guards every migration that will + * ever be added going forward. + * - Ten pre-800 migrations carry the same MariaDB-only syntax + * (1, 112, 150, 153, 160, 241, 291, 295, 351, 355). They only run for + * ancient (pre-356, SB 1.5.x-era) installs upgrading straight to v2 on + * MySQL / Percona — a real but rarer path, and a couple have quirks + * (295 is a compound `ADD ..., ADD ...`; 355 has a hardcoded `sb_mods` + * prefix). They're a tracked follow-up, deliberately out of scope here + * so this fix stays small and low-risk. + * + * `CREATE TABLE IF NOT EXISTS` (805.php, 700.php, 475.php, ...) is VALID on + * every engine and is intentionally NOT matched — the regex is anchored on + * `ADD ... IF NOT EXISTS`, the column / index form MySQL lacks. + * + * Mirrors `DeadJsCallSitesTest`'s pure-file-scan shape: extends + * `PHPUnit\Framework\TestCase` (no DB / Smarty bring-up), strips PHP + * comments via `php_strip_whitespace()` so a migration's own docblock + * mentioning the forbidden syntax (801.php does) doesn't false-fire. + */ +final class UpdaterMigrationPortableSqlTest extends TestCase +{ + /** + * Matches the MariaDB-only `ALTER TABLE ... ADD [COLUMN|KEY|INDEX|...] + * IF NOT EXISTS` shape. Anchored on `ADD` + up to two keyword tokens + + * `IF NOT EXISTS`, so `CREATE TABLE IF NOT EXISTS` (valid everywhere) + * is left alone and punctuation (`;`, backticks) can't be spanned by + * the `[A-Za-z]+` token class. + */ + private const ADD_IF_NOT_EXISTS_REGEX = '/\bADD\s+(?:[A-Za-z]+\s+){0,2}IF\s+NOT\s+EXISTS\b/i'; + + /** + * The v2-era floor. Migrations at or above this version are the + * actively-maintained set every supported upgrade path runs; see the + * class docblock for why pre-800 migrations are out of scope. + */ + private const V2_MIGRATION_FLOOR = 800; + + public function testV2MigrationsAvoidMariaDbOnlyAddIfNotExists(): void + { + $dataDir = ROOT . 'updater/data'; + $this->assertDirectoryExists($dataDir, 'updater/data/ must live under web/ for the scan to find it'); + + $store = $this->loadStore(); + + $scanned = []; + $hits = []; + foreach ($store as $version => $file) { + if ((int) $version < self::V2_MIGRATION_FLOOR) { + continue; + } + + $file = (string) $file; + $path = $dataDir . '/' . $file; + $this->assertFileExists( + $path, + "store.json registers migration {$version} => {$file}, but the file is missing.", + ); + + $scanned[] = $file; + $stripped = php_strip_whitespace($path); + if (preg_match(self::ADD_IF_NOT_EXISTS_REGEX, $stripped) === 1) { + $hits[] = "{$file} (version {$version})"; + } + } + + // Non-vacuous: 801 is the #1498 fix and must be in the scanned set, + // so a store.json typo / floor change can't make the test pass by + // scanning nothing. + $this->assertContains( + '801.php', + $scanned, + 'Expected 801.php in the v2 migration scan — store.json or the version floor changed unexpectedly.', + ); + + $this->assertSame( + 0, + count($hits), + "v2 migration(s) use MariaDB-only `ALTER TABLE ... ADD ... IF NOT EXISTS`, which MySQL / Percona reject " + . "with SQLSTATE[42000] 1064 mid-upgrade (issue #1498). Use a portable information_schema existence " + . "probe + plain `ADD COLUMN` instead (see web/updater/data/801.php for the reference shape).\n\n" + . "Offending migration(s):\n - " + . implode("\n - ", $hits), + ); + } + + /** + * Decode the migration registry. JSON object keys arrive as PHP array + * keys (numeric-looking keys become ints); values are filenames. + * + * @return array + */ + private function loadStore(): array + { + $storePath = ROOT . 'updater/store.json'; + $this->assertFileExists($storePath); + + $decoded = json_decode((string) file_get_contents($storePath), true); + $this->assertIsArray($decoded, 'updater/store.json must decode to an array of version => filename.'); + + return $decoded; + } +} diff --git a/web/updater/data/801.php b/web/updater/data/801.php index bad4398b6..70b526002 100644 --- a/web/updater/data/801.php +++ b/web/updater/data/801.php @@ -1,14 +1,58 @@ dbs->query("ALTER TABLE `:prefix_admins` ADD IF NOT EXISTS `attempts` INT DEFAULT 0"); -$this->dbs->execute(); - -$this->dbs->query("ALTER TABLE `:prefix_admins` ADD IF NOT EXISTS `lockout_until` DATETIME DEFAULT NULL"); -$this->dbs->execute(); - -return true; \ No newline at end of file +// Issue #1498 (regression of #1472's fix in #1473): add the admin lockout +// columns idempotently on BOTH MySQL and MariaDB. +// +// #1473 swapped the original bare `ADD` for `ADD IF NOT EXISTS` so panels +// that already carry the columns (v1.8.x installs, or a re-run after a +// partial pass) stop fataling with SQLSTATE[42S21] "Duplicate column name". +// But `ALTER TABLE ... ADD [COLUMN] IF NOT EXISTS` is MariaDB-only syntax — +// MySQL (every version, 8.x included) rejects it with SQLSTATE[42000] 1064 +// "...near 'IF NOT EXISTS `attempts` INT DEFAULT 0'". The dev/CI database is +// MariaDB, so the syntax sailed past both the regression test and PHPStan's +// dba gate (which also introspects MariaDB), and the break only surfaced on +// self-hosters running stock MySQL. +// +// Guard each ADD with a portable information_schema existence check instead: +// same idempotent contract, runs on both engines, converges to the same +// schema. Fresh installs get the columns from install/includes/sql/struc.sql +// and never run the updater. +// +// `$this` is supplied by Updater::update(), which loads this file inside the +// Updater instance scope; PHPStan can't see that, so the two `$this->dbs` +// reads below are suppressed the same way every sibling migration is (the +// older ones live in phpstan-baseline.neon; new ignores go inline). + +/** + * Add a column to `:prefix_admins` only when it isn't already present. + * + * Portable across MySQL + MariaDB: the existence probe is plain + * information_schema (no MariaDB-only `ADD IF NOT EXISTS`), and the table + * name is bound as a value resolved from the live prefix rather than the + * `:prefix` placeholder so the COLUMN lookup matches the real table. + */ +$ensureAdminColumn = static function (\Database $dbs, string $column, string $alterSql): void { + $dbs->query( + 'SELECT COUNT(*) AS c FROM information_schema.COLUMNS ' + . 'WHERE TABLE_SCHEMA = DATABASE() ' + . 'AND TABLE_NAME = :table ' + . 'AND COLUMN_NAME = :column' + ); + $dbs->bind(':table', $dbs->getPrefix() . '_admins'); + $dbs->bind(':column', $column); + $row = $dbs->single(); + + if (is_array($row) && (int) ($row['c'] ?? 0) > 0) { + return; // column already present — idempotent no-op + } + + $dbs->query($alterSql); + $dbs->execute(); +}; + +// @phpstan-ignore variable.undefined +$ensureAdminColumn($this->dbs, 'attempts', 'ALTER TABLE `:prefix_admins` ADD COLUMN `attempts` INT DEFAULT 0'); +// @phpstan-ignore variable.undefined +$ensureAdminColumn($this->dbs, 'lockout_until', 'ALTER TABLE `:prefix_admins` ADD COLUMN `lockout_until` DATETIME DEFAULT NULL'); + +return true;