Skip to content

fix(schema): store two-state columns as tinyint(1), not enum('0','1') #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mastacontrola merged 1 commit into from
Aug 25, 2026
Merged

fix(schema): store two-state columns as tinyint(1), not enum('0','1') #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions ldap/class/ldapmanager.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,12 @@ public function createSql()
'LONGTEXT',
'LONGTEXT',
'LONGTEXT',
"ENUM('0', '1')",
'TINYINT(1)',
'VARCHAR(255)',
"ENUM('0','1')",
'TINYINT(1)',
'VARCHAR(255)',
"ENUM('0', '1')",
"ENUM('0', '1')",
'TINYINT(1)',
'TINYINT(1)',
// Words rather than the 0/1/2 this table uses elsewhere:
// the value is read straight out in LDAP::authLDAP(), and a
// strategy name that says what it does beats a sentinel
Expand Down Expand Up @@ -874,6 +874,42 @@ function () {
// 26
"ALTER TABLE `LDAPServers` ADD COLUMN `lsTlsCaCert` "
. "VARCHAR(255) NOT NULL DEFAULT ''",
// 27 - two-state columns become tinyint(1) (fogproject ADR
// 0028). They were enum('0','1'), and an integer written to an
// ENUM is a member INDEX rather than a value: 1 selects the
// member '0' -- FALSE -- and 0 is the error value
// STRICT_TRANS_TABLES refuses. tinyint has no such trap.
//
// Appended rather than folded into the createSql() step above,
// for the same reason as every ALTER here: installdb() SKIPS the
// pSchema steps an install has already passed instead of
// replaying them, so an edit to an earlier step is invisible to
// everyone already past it. createSql() now declares these
// TINYINT(1) for a fresh install; this is what an existing one
// gets. The historical ADD COLUMN steps above still say ENUM and
// must stay that way -- rewriting one changes nothing for anyone
// who ran it, and applyUpdates() tolerates 1060 so a fresh
// install runs them harmlessly against columns that are already
// tinyint.
//
// 🔴 Schema::enumToTinyint() and not a hand-written ALTER: a
// direct `ALTER TABLE t MODIFY c TINYINT(1)` converts an ENUM BY
// INDEX, turning every '0' into 1 and every '1' into 2 -- both
// truthy, silently, on every upgrading server. The helper goes
// through VARCHAR(1) so the conversion is by label, and carries
// this table's nullability and defaults across (lsAllowAPI is nullable and lsUseGroupMatch has no default at all).
function () {
return Schema::enumToTinyint(
[
'LDAPServers' => [
'lsAllowAPI',
'lsDisplayNameEnabled',
'lsIsLDAPs',
'lsUseGroupMatch',
],
]
);
},
];
}
/**
Expand Down
27 changes: 26 additions & 1 deletion location/class/locationmanager.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function createSql()
'INTEGER',
'VARCHAR(40)',
'TIMESTAMP',
"ENUM('0', '1')",
'TINYINT(1)',
"ENUM('http', 'https')"
],
[
Expand Down Expand Up @@ -134,6 +134,31 @@ public function schema()
'ALTER TABLE `%s` DROP INDEX `index2`',
$this->tablename
),
// 3 - lTftpEnabled becomes tinyint(1) (fogproject ADR 0028). It
// was enum('0','1'), and an integer written to an ENUM is a
// member INDEX rather than a value: 1 selects the member '0' --
// FALSE -- and 0 is the error value STRICT_TRANS_TABLES refuses.
// tinyint has no such trap.
//
// Appended rather than folded into step 0, for the same reason
// as step 2: installdb() SKIPS the pSchema steps an install has
// already passed instead of replaying them, so an edit to an
// earlier step is invisible to everyone already past it.
// createSql() now declares TINYINT(1) for a fresh install; this
// is what an existing one gets.
//
// 🔴 Schema::enumToTinyint() and not a hand-written ALTER: a
// direct `ALTER TABLE t MODIFY c TINYINT(1)` converts an ENUM BY
// INDEX, turning every '0' into 1 and every '1' into 2 -- both
// truthy, silently, on every upgrading server. The helper goes
// through VARCHAR(1) so the conversion is by label.
function () {
return Schema::enumToTinyint(
[
$this->tablename => ['lTftpEnabled'],
]
);
},
];
}
/**
Expand Down
47 changes: 42 additions & 5 deletions oidc/class/oidcmanager.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ public function createSql()
'VARCHAR(255)',
'VARCHAR(255)',
'VARCHAR(255)',
"ENUM('0', '1')",
"ENUM('0', '1')",
"ENUM('0', '1')",
"ENUM('0', '1')",
"ENUM('0', '1')",
'TINYINT(1)',
'TINYINT(1)',
'TINYINT(1)',
'TINYINT(1)',
'TINYINT(1)',
'VARCHAR(255)'
],
[
Expand Down Expand Up @@ -236,6 +236,43 @@ function () {
// has been told about yet.
"ALTER TABLE `OIDCProviders` ADD COLUMN `opAutoRedirect` "
. "ENUM('0', '1') NOT NULL DEFAULT '0'",
// 8 - two-state columns become tinyint(1) (fogproject ADR
// 0028). They were enum('0','1'), and an integer written to an
// ENUM is a member INDEX rather than a value: 1 selects the
// member '0' -- FALSE -- and 0 is the error value
// STRICT_TRANS_TABLES refuses. tinyint has no such trap.
//
// Appended rather than folded into the createSql() step above,
// for the same reason as every ALTER here: installdb() SKIPS the
// pSchema steps an install has already passed instead of
// replaying them, so an edit to an earlier step is invisible to
// everyone already past it. createSql() now declares these
// TINYINT(1) for a fresh install; this is what an existing one
// gets. The historical ADD COLUMN steps above still say ENUM and
// must stay that way -- rewriting one changes nothing for anyone
// who ran it, and applyUpdates() tolerates 1060 so a fresh
// install runs them harmlessly against columns that are already
// tinyint.
//
// 🔴 Schema::enumToTinyint() and not a hand-written ALTER: a
// direct `ALTER TABLE t MODIFY c TINYINT(1)` converts an ENUM BY
// INDEX, turning every '0' into 1 and every '1' into 2 -- both
// truthy, silently, on every upgrading server. The helper goes
// through VARCHAR(1) so the conversion is by label, and carries
// this table's nullability and defaults across (all five are NOT NULL DEFAULT '0').
function () {
return Schema::enumToTinyint(
[
'OIDCProviders' => [
'opAllowAPI',
'opAutoRedirect',
'opEnabled',
'opJITProvision',
'opSingleLogout',
],
]
);
},
];
}
/**
Expand Down
177 changes: 177 additions & 0 deletions tests/booleans-are-tinyint.test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
<?php
/**
* A plugin's two-state columns are tinyint(1), and it converts them through
* the shared helper.
*
* fogproject ADR 0028. FOG spelled its two-state columns enum('0','1') for
* years, which put a trap in every one of them: an integer written to an ENUM
* is a member INDEX, not a value, so 1 selects the member '0' -- FALSE -- and
* 0 is the error value STRICT_TRANS_TABLES refuses. Core converted its columns
* in schema step 368; each plugin owns its own schema (ADR 0009) and so
* converts its own.
*
* WHAT THIS PINS:
*
* 1. createSql() declares no ENUM('0','1') column. That is what a fresh
* install gets, and it is what a newly added boolean would break.
*
* 2. Each plugin that had two-state columns converts them by calling
* Schema::enumToTinyint(), not by writing its own ALTER. This is the
* load-bearing one: a direct `ALTER TABLE t MODIFY c TINYINT(1)`
* converts an ENUM BY INDEX, so every '0' becomes 1 and every '1'
* becomes 2 -- both truthy, silently, on every upgrading server. The
* helper goes through VARCHAR(1) so the conversion is by label. A plugin
* hand-rolling the ALTER is the exact mistake this forbids.
*
* It does NOT pin which columns each plugin converts -- check 1 covers a new
* one without anyone editing this file.
*
* Deliberately NOT pinned: the historical `ADD COLUMN ... ENUM('0','1')`
* steps further down each schema(). installdb() skips the steps an install
* has already passed rather than replaying them, so editing one is invisible
* to everyone past it and changes nothing; they must stay as they are.
*
* PHP version 7.4+
*
* @category Tests
* @package FOGProject
* @author Tom Elliott <tommygunsster@gmail.com>
* @license http://opensource.org/licenses/gpl-3.0 GPLv3
* @link https://fogproject.org
*/

$root = dirname(__DIR__);
$failures = [];
$checks = 0;

/**
* Records one assertion.
*
* @param string $what what is being asserted
* @param bool $ok whether it holds
* @param array $failures collected failures
* @param int $checks running count
*
* @return void
*/
function btCheck($what, $ok, &$failures, &$checks)
{
$checks++;
if (!$ok) {
$failures[] = $what;
}
}

/**
* The body of one method, comments stripped -- prose in this repo discusses
* ENUM spellings at length and must not satisfy or fail a check.
*
* @param string $file the file to read
* @param string $method the method name
*
* @return string
*/
function btMethod($file, $method)
{
$clean = '';
foreach (token_get_all((string) file_get_contents($file)) as $token) {
if (is_array($token)
&& ($token[0] === T_COMMENT || $token[0] === T_DOC_COMMENT)
) {
continue;
}
$clean .= is_array($token) ? $token[1] : $token;
}
$at = strpos($clean, 'function ' . $method . '(');
if (false === $at) {
return '';
}
// Brace-match rather than cutting at the next `function `: these
// schema() bodies contain closures, so the naive cut stops short of
// exactly the step being checked for.
$open = strpos($clean, '{', $at);
if (false === $open) {
return '';
}
$depth = 0;
$len = strlen($clean);
for ($i = $open; $i < $len; $i++) {
if ($clean[$i] === '{') {
$depth++;
} elseif ($clean[$i] === '}') {
$depth--;
if ($depth === 0) {
return substr($clean, $at, $i - $at + 1);
}
}
}
return substr($clean, $at);
}

$managers = glob($root . '/*/class/*manager.class.php');
btCheck('plugin managers were found', count($managers) > 0, $failures, $checks);

// The plugins that shipped two-state columns. Named, because "calls
// enumToTinyint" is only a requirement for a plugin that has something to
// convert -- every other manager must be free of both.
$converters = [
'ldap/class/ldapmanager.class.php',
'oidc/class/oidcmanager.class.php',
'location/class/locationmanager.class.php',
];

foreach ($managers as $file) {
$rel = str_replace($root . '/', '', $file);
$create = btMethod($file, 'createSql');
btCheck(
sprintf(
'%s createSql() declares no ENUM(\'0\',\'1\') column -- a '
. 'two-state column is TINYINT(1) (fogproject ADR 0028); an '
. 'integer written to that enum is a member index, so 1 stores '
. 'FALSE',
$rel
),
!preg_match("/ENUM\(\s*'0'\s*,\s*'1'\s*\)/i", $create),
$failures,
$checks
);

$schema = btMethod($file, 'schema');
if (in_array($rel, $converters, true)) {
btCheck(
sprintf(
'%s schema() converts its two-state columns through '
. 'Schema::enumToTinyint() -- a hand-written '
. 'ALTER ... MODIFY TINYINT(1) converts an ENUM by INDEX and '
. 'silently switches on every flag in the table',
$rel
),
false !== strpos($schema, 'Schema::enumToTinyint'),
$failures,
$checks
);
}

btCheck(
sprintf(
'%s schema() writes no ALTER of its own to TINYINT -- that is '
. 'the conversion that goes by index; use '
. 'Schema::enumToTinyint()',
$rel
),
!preg_match('/MODIFY[^;]{0,80}TINYINT/i', $schema),
$failures,
$checks
);
}

printf("%d checks\n", $checks);
if (count($failures) > 0) {
foreach ($failures as $f) {
printf(" FAIL %s\n", $f);
}
printf("%d failed\n", count($failures));
exit(1);
}
printf("all passed\n");
exit(0);