Skip to content

Commit 97d5804

Browse files
rumblefrogrumblefrog
authored
fix(updater): portable information_schema guard for lockout columns (#1498) (#1499)
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.
1 parent db60c19 commit 97d5804

3 files changed

Lines changed: 192 additions & 18 deletions

File tree

‎web/phpstan-baseline.neon‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -515,9 +515,3 @@ parameters:
515515
identifier: variable.undefined
516516
count: 4
517517
path: updater/data/705.php
518-
519-
-
520-
message: '#^Variable \$this might not be defined\.$#'
521-
identifier: variable.undefined
522-
count: 4
523-
path: updater/data/801.php

‎web/tests/integration/UpdaterMigrationPortableSqlTest.php‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sbpp\Tests\Integration;
6+
7+
use PHPUnit\Framework\TestCase;
8+
9+
/**
10+
* Issue #1498: `web/updater/data/801.php` shipped
11+
* `ALTER TABLE ... ADD IF NOT EXISTS` — a MariaDB-only DDL extension that
12+
* stock MySQL (and MySQL-compatible engines like Percona Server) reject
13+
* with `SQLSTATE[42000] 1064` mid-upgrade. The dev stack + CI both run
14+
* MariaDB (which accepts the syntax), so the runtime idempotency test in
15+
* `Updater801LockoutColumnsTest` AND PHPStan's dba gate both passed it
16+
* through; the break only surfaced on self-hosters running MySQL / Percona,
17+
* which the docs support as first-class engines (prerequisites.mdx:
18+
* "MySQL >= 5.6 ... 8.0+ is fine and recommended").
19+
*
20+
* A runtime test can't catch this — it would need a live MySQL to fail on,
21+
* and the suite runs against MariaDB. This is a STATIC source-scan guard:
22+
* no v2-era migration (version >= 800) may use the `ADD ... IF NOT EXISTS`
23+
* conditional-DDL form. The portable shape is an information_schema
24+
* existence probe + a plain `ADD COLUMN` (see 801.php for the reference).
25+
*
26+
* Scope (focused, per #1498)
27+
* --------------------------
28+
* Only migrations with version >= 800 (the 8xx v2 block, 801-810 today)
29+
* are scanned. Two reasons:
30+
*
31+
* - Every supported upgrade path the issue names (v2 rc5 -> rc6,
32+
* v1.8.x -> v2) starts at `config.version = 705`, so the updater only
33+
* runs 801+. 801 is the first migration past 705 and the actual
34+
* blocker. Future migrations are always numbered above the current
35+
* max (811+), so a `>= 800` floor guards every migration that will
36+
* ever be added going forward.
37+
* - Ten pre-800 migrations carry the same MariaDB-only syntax
38+
* (1, 112, 150, 153, 160, 241, 291, 295, 351, 355). They only run for
39+
* ancient (pre-356, SB 1.5.x-era) installs upgrading straight to v2 on
40+
* MySQL / Percona — a real but rarer path, and a couple have quirks
41+
* (295 is a compound `ADD ..., ADD ...`; 355 has a hardcoded `sb_mods`
42+
* prefix). They're a tracked follow-up, deliberately out of scope here
43+
* so this fix stays small and low-risk.
44+
*
45+
* `CREATE TABLE IF NOT EXISTS` (805.php, 700.php, 475.php, ...) is VALID on
46+
* every engine and is intentionally NOT matched — the regex is anchored on
47+
* `ADD ... IF NOT EXISTS`, the column / index form MySQL lacks.
48+
*
49+
* Mirrors `DeadJsCallSitesTest`'s pure-file-scan shape: extends
50+
* `PHPUnit\Framework\TestCase` (no DB / Smarty bring-up), strips PHP
51+
* comments via `php_strip_whitespace()` so a migration's own docblock
52+
* mentioning the forbidden syntax (801.php does) doesn't false-fire.
53+
*/
54+
final class UpdaterMigrationPortableSqlTest extends TestCase
55+
{
56+
/**
57+
* Matches the MariaDB-only `ALTER TABLE ... ADD [COLUMN|KEY|INDEX|...]
58+
* IF NOT EXISTS` shape. Anchored on `ADD` + up to two keyword tokens +
59+
* `IF NOT EXISTS`, so `CREATE TABLE IF NOT EXISTS` (valid everywhere)
60+
* is left alone and punctuation (`;`, backticks) can't be spanned by
61+
* the `[A-Za-z]+` token class.
62+
*/
63+
private const ADD_IF_NOT_EXISTS_REGEX = '/\bADD\s+(?:[A-Za-z]+\s+){0,2}IF\s+NOT\s+EXISTS\b/i';
64+
65+
/**
66+
* The v2-era floor. Migrations at or above this version are the
67+
* actively-maintained set every supported upgrade path runs; see the
68+
* class docblock for why pre-800 migrations are out of scope.
69+
*/
70+
private const V2_MIGRATION_FLOOR = 800;
71+
72+
public function testV2MigrationsAvoidMariaDbOnlyAddIfNotExists(): void
73+
{
74+
$dataDir = ROOT . 'updater/data';
75+
$this->assertDirectoryExists($dataDir, 'updater/data/ must live under web/ for the scan to find it');
76+
77+
$store = $this->loadStore();
78+
79+
$scanned = [];
80+
$hits = [];
81+
foreach ($store as $version => $file) {
82+
if ((int) $version < self::V2_MIGRATION_FLOOR) {
83+
continue;
84+
}
85+
86+
$file = (string) $file;
87+
$path = $dataDir . '/' . $file;
88+
$this->assertFileExists(
89+
$path,
90+
"store.json registers migration {$version} => {$file}, but the file is missing.",
91+
);
92+
93+
$scanned[] = $file;
94+
$stripped = php_strip_whitespace($path);
95+
if (preg_match(self::ADD_IF_NOT_EXISTS_REGEX, $stripped) === 1) {
96+
$hits[] = "{$file} (version {$version})";
97+
}
98+
}
99+
100+
// Non-vacuous: 801 is the #1498 fix and must be in the scanned set,
101+
// so a store.json typo / floor change can't make the test pass by
102+
// scanning nothing.
103+
$this->assertContains(
104+
'801.php',
105+
$scanned,
106+
'Expected 801.php in the v2 migration scan — store.json or the version floor changed unexpectedly.',
107+
);
108+
109+
$this->assertSame(
110+
0,
111+
count($hits),
112+
"v2 migration(s) use MariaDB-only `ALTER TABLE ... ADD ... IF NOT EXISTS`, which MySQL / Percona reject "
113+
. "with SQLSTATE[42000] 1064 mid-upgrade (issue #1498). Use a portable information_schema existence "
114+
. "probe + plain `ADD COLUMN` instead (see web/updater/data/801.php for the reference shape).\n\n"
115+
. "Offending migration(s):\n - "
116+
. implode("\n - ", $hits),
117+
);
118+
}
119+
120+
/**
121+
* Decode the migration registry. JSON object keys arrive as PHP array
122+
* keys (numeric-looking keys become ints); values are filenames.
123+
*
124+
* @return array<array-key, mixed>
125+
*/
126+
private function loadStore(): array
127+
{
128+
$storePath = ROOT . 'updater/store.json';
129+
$this->assertFileExists($storePath);
130+
131+
$decoded = json_decode((string) file_get_contents($storePath), true);
132+
$this->assertIsArray($decoded, 'updater/store.json must decode to an array of version => filename.');
133+
134+
return $decoded;
135+
}
136+
}

‎web/updater/data/801.php‎

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,58 @@
11
<?php
22

3-
// Issue #1472: panels upgrading from v1.8.x (or re-running the updater
4-
// after a partial pass) may already carry the lockout columns — the bare
5-
// ADD COLUMN shape fatals with SQLSTATE[42S21] Duplicate column name.
6-
// ADD IF NOT EXISTS matches the idempotent contract used by sibling
7-
// migrations (112.php, 150.php, …) and converges to the same schema.
8-
$this->dbs->query("ALTER TABLE `:prefix_admins` ADD IF NOT EXISTS `attempts` INT DEFAULT 0");
9-
$this->dbs->execute();
10-
11-
$this->dbs->query("ALTER TABLE `:prefix_admins` ADD IF NOT EXISTS `lockout_until` DATETIME DEFAULT NULL");
12-
$this->dbs->execute();
13-
14-
return true;
3+
// Issue #1498 (regression of #1472's fix in #1473): add the admin lockout
4+
// columns idempotently on BOTH MySQL and MariaDB.
5+
//
6+
// #1473 swapped the original bare `ADD` for `ADD IF NOT EXISTS` so panels
7+
// that already carry the columns (v1.8.x installs, or a re-run after a
8+
// partial pass) stop fataling with SQLSTATE[42S21] "Duplicate column name".
9+
// But `ALTER TABLE ... ADD [COLUMN] IF NOT EXISTS` is MariaDB-only syntax —
10+
// MySQL (every version, 8.x included) rejects it with SQLSTATE[42000] 1064
11+
// "...near 'IF NOT EXISTS `attempts` INT DEFAULT 0'". The dev/CI database is
12+
// MariaDB, so the syntax sailed past both the regression test and PHPStan's
13+
// dba gate (which also introspects MariaDB), and the break only surfaced on
14+
// self-hosters running stock MySQL.
15+
//
16+
// Guard each ADD with a portable information_schema existence check instead:
17+
// same idempotent contract, runs on both engines, converges to the same
18+
// schema. Fresh installs get the columns from install/includes/sql/struc.sql
19+
// and never run the updater.
20+
//
21+
// `$this` is supplied by Updater::update(), which loads this file inside the
22+
// Updater instance scope; PHPStan can't see that, so the two `$this->dbs`
23+
// reads below are suppressed the same way every sibling migration is (the
24+
// older ones live in phpstan-baseline.neon; new ignores go inline).
25+
26+
/**
27+
* Add a column to `:prefix_admins` only when it isn't already present.
28+
*
29+
* Portable across MySQL + MariaDB: the existence probe is plain
30+
* information_schema (no MariaDB-only `ADD IF NOT EXISTS`), and the table
31+
* name is bound as a value resolved from the live prefix rather than the
32+
* `:prefix` placeholder so the COLUMN lookup matches the real table.
33+
*/
34+
$ensureAdminColumn = static function (\Database $dbs, string $column, string $alterSql): void {
35+
$dbs->query(
36+
'SELECT COUNT(*) AS c FROM information_schema.COLUMNS '
37+
. 'WHERE TABLE_SCHEMA = DATABASE() '
38+
. 'AND TABLE_NAME = :table '
39+
. 'AND COLUMN_NAME = :column'
40+
);
41+
$dbs->bind(':table', $dbs->getPrefix() . '_admins');
42+
$dbs->bind(':column', $column);
43+
$row = $dbs->single();
44+
45+
if (is_array($row) && (int) ($row['c'] ?? 0) > 0) {
46+
return; // column already present — idempotent no-op
47+
}
48+
49+
$dbs->query($alterSql);
50+
$dbs->execute();
51+
};
52+
53+
// @phpstan-ignore variable.undefined
54+
$ensureAdminColumn($this->dbs, 'attempts', 'ALTER TABLE `:prefix_admins` ADD COLUMN `attempts` INT DEFAULT 0');
55+
// @phpstan-ignore variable.undefined
56+
$ensureAdminColumn($this->dbs, 'lockout_until', 'ALTER TABLE `:prefix_admins` ADD COLUMN `lockout_until` DATETIME DEFAULT NULL');
57+
58+
return true;

0 commit comments

Comments
 (0)