diff --git a/ldap/class/ldapmanager.class.php b/ldap/class/ldapmanager.class.php index a7fc833..4d5d048 100644 --- a/ldap/class/ldapmanager.class.php +++ b/ldap/class/ldapmanager.class.php @@ -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 @@ -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', + ], + ] + ); + }, ]; } /** diff --git a/location/class/locationmanager.class.php b/location/class/locationmanager.class.php index 33f33d1..d5d4e92 100644 --- a/location/class/locationmanager.class.php +++ b/location/class/locationmanager.class.php @@ -59,7 +59,7 @@ public function createSql() 'INTEGER', 'VARCHAR(40)', 'TIMESTAMP', - "ENUM('0', '1')", + 'TINYINT(1)', "ENUM('http', 'https')" ], [ @@ -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'], + ] + ); + }, ]; } /** diff --git a/oidc/class/oidcmanager.class.php b/oidc/class/oidcmanager.class.php index edbc41e..adb7c0a 100644 --- a/oidc/class/oidcmanager.class.php +++ b/oidc/class/oidcmanager.class.php @@ -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)' ], [ @@ -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', + ], + ] + ); + }, ]; } /** diff --git a/tests/booleans-are-tinyint.test.php b/tests/booleans-are-tinyint.test.php new file mode 100644 index 0000000..bdb3f86 --- /dev/null +++ b/tests/booleans-are-tinyint.test.php @@ -0,0 +1,177 @@ + + * @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);