From 39170f9426985f663e259c0c0126d14f31b5bf5f Mon Sep 17 00:00:00 2001 From: JJ Fullmer <7743340+darksidemilk@users.noreply.github.com> Date: Sat, 22 Aug 2026 09:54:38 -0500 Subject: [PATCH] Build plugin tables through createTableSql(), so optional columns get defaults GH-1245. A column declared NOT NULL with no DEFAULT is only mandatory if something enforces it, and for nine years nothing did: PDODB cleared sql_mode on every connection, so the server downgraded the error to a warning and substituted an implicit zero. Removing the clear turned every one of those declarations into a real constraint, and an INSERT that omits one now fails with error 1364. FOG's schema step 348 repairs the tables an install already has. It cannot repair ours: createSql() runs as step 0 of each plugin's own schema(), so a plugin installed AFTER the migration gets a table built the old way -- every optional column mandatory again. FOGManagerController::createTableSql() (fogproject #1275) takes the identical arguments and fills a default into every NOT NULL column that has none, leaving bare only the primary key and auto-increment column, anything the model declares required, and anything whose name ends in ID. A default a manager passes explicitly always wins, so nothing stated here changes. Measured against the live 1.6 install, across all 23 tables the probe can build: 60 columns now carry a default where 29 did, and the 52 left bare are each bare for one of those three stated reasons. Every CREATE TABLE is still accepted by the server. The test stub gains a pass-through createTableSql(). It deliberately does not reimplement the rule: the real one only ADDS defaults, so a pass-through cannot mask an assertion about a default a manager sets deliberately -- which is what oidc-provider-safety checks -- and a copy of the rule would only give it somewhere to drift from. Requires fogproject FOG_PLUGINS_VERSION to be bumped to the release carrying this, and that release requires #1275. Co-Authored-By: Claude --- capone/class/caponemanager.class.php | 2 +- helloworld/class/helloworldmanager.class.php | 2 +- ldap/class/ldapgroupmanager.class.php | 2 +- .../ldapgrouproleassociationmanager.class.php | 2 +- ...groupusergroupassociationmanager.class.php | 2 +- ldap/class/ldapmanager.class.php | 2 +- ldap/class/ldapusergrantmanager.class.php | 2 +- .../locationassociationmanager.class.php | 2 +- location/class/locationmanager.class.php | 2 +- ntfy/class/ntfymanager.class.php | 2 +- oidc/class/oidcgroupmanager.class.php | 2 +- .../oidcgrouproleassociationmanager.class.php | 2 +- ...groupusergroupassociationmanager.class.php | 2 +- oidc/class/oidcidentitymanager.class.php | 2 +- oidc/class/oidcmanager.class.php | 2 +- oidc/class/oidcusergrantmanager.class.php | 2 +- ou/class/ouassociationmanager.class.php | 2 +- ou/class/oumanager.class.php | 2 +- pushbullet/class/pushbulletmanager.class.php | 2 +- slack/class/slackmanager.class.php | 2 +- .../class/subnetgroupmanager.class.php | 2 +- tests/oidc-provider-safety.test.php | 2 +- tests/stubs/fog-stubs.php | 17 ++ tests/tables-carry-column-defaults.test.php | 147 ++++++++++++++++++ .../windowskeyassociationmanager.class.php | 2 +- windowskey/class/windowskeymanager.class.php | 2 +- .../class/wolbroadcastmanager.class.php | 2 +- 27 files changed, 189 insertions(+), 25 deletions(-) create mode 100644 tests/tables-carry-column-defaults.test.php diff --git a/capone/class/caponemanager.class.php b/capone/class/caponemanager.class.php index 124f763e..17d76d32 100644 --- a/capone/class/caponemanager.class.php +++ b/capone/class/caponemanager.class.php @@ -36,7 +36,7 @@ class CaponeManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/helloworld/class/helloworldmanager.class.php b/helloworld/class/helloworldmanager.class.php index 9e9b27d9..0b81ddb8 100644 --- a/helloworld/class/helloworldmanager.class.php +++ b/helloworld/class/helloworldmanager.class.php @@ -45,7 +45,7 @@ class HelloWorldManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ldap/class/ldapgroupmanager.class.php b/ldap/class/ldapgroupmanager.class.php index 7ec8a3f9..765e7bd2 100644 --- a/ldap/class/ldapgroupmanager.class.php +++ b/ldap/class/ldapgroupmanager.class.php @@ -43,7 +43,7 @@ class LDAPGroupManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ldap/class/ldapgrouproleassociationmanager.class.php b/ldap/class/ldapgrouproleassociationmanager.class.php index 8b8e362f..90232d24 100644 --- a/ldap/class/ldapgrouproleassociationmanager.class.php +++ b/ldap/class/ldapgrouproleassociationmanager.class.php @@ -42,7 +42,7 @@ class LDAPGroupRoleAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ldap/class/ldapgroupusergroupassociationmanager.class.php b/ldap/class/ldapgroupusergroupassociationmanager.class.php index 70393cac..89cf481c 100644 --- a/ldap/class/ldapgroupusergroupassociationmanager.class.php +++ b/ldap/class/ldapgroupusergroupassociationmanager.class.php @@ -42,7 +42,7 @@ class LDAPGroupUserGroupAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ldap/class/ldapmanager.class.php b/ldap/class/ldapmanager.class.php index f1179e75..a7fc833e 100644 --- a/ldap/class/ldapmanager.class.php +++ b/ldap/class/ldapmanager.class.php @@ -40,7 +40,7 @@ class LDAPManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ldap/class/ldapusergrantmanager.class.php b/ldap/class/ldapusergrantmanager.class.php index 86d69683..81fc942e 100644 --- a/ldap/class/ldapusergrantmanager.class.php +++ b/ldap/class/ldapusergrantmanager.class.php @@ -43,7 +43,7 @@ class LDAPUserGrantManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/location/class/locationassociationmanager.class.php b/location/class/locationassociationmanager.class.php index f9ea26be..1271f268 100644 --- a/location/class/locationassociationmanager.class.php +++ b/location/class/locationassociationmanager.class.php @@ -39,7 +39,7 @@ class LocationAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/location/class/locationmanager.class.php b/location/class/locationmanager.class.php index 36588086..33f33d14 100644 --- a/location/class/locationmanager.class.php +++ b/location/class/locationmanager.class.php @@ -37,7 +37,7 @@ class LocationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ntfy/class/ntfymanager.class.php b/ntfy/class/ntfymanager.class.php index 3a05ab8a..147384fe 100644 --- a/ntfy/class/ntfymanager.class.php +++ b/ntfy/class/ntfymanager.class.php @@ -65,7 +65,7 @@ public function createSql() $keys = [ 'nID' ]; - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, $fields, diff --git a/oidc/class/oidcgroupmanager.class.php b/oidc/class/oidcgroupmanager.class.php index a5a2884d..756f2b51 100644 --- a/oidc/class/oidcgroupmanager.class.php +++ b/oidc/class/oidcgroupmanager.class.php @@ -49,7 +49,7 @@ class OIDCGroupManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/oidc/class/oidcgrouproleassociationmanager.class.php b/oidc/class/oidcgrouproleassociationmanager.class.php index 5ed188ad..97c3e407 100644 --- a/oidc/class/oidcgrouproleassociationmanager.class.php +++ b/oidc/class/oidcgrouproleassociationmanager.class.php @@ -41,7 +41,7 @@ class OIDCGroupRoleAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/oidc/class/oidcgroupusergroupassociationmanager.class.php b/oidc/class/oidcgroupusergroupassociationmanager.class.php index 5686238e..f9d733f6 100644 --- a/oidc/class/oidcgroupusergroupassociationmanager.class.php +++ b/oidc/class/oidcgroupusergroupassociationmanager.class.php @@ -41,7 +41,7 @@ class OIDCGroupUserGroupAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/oidc/class/oidcidentitymanager.class.php b/oidc/class/oidcidentitymanager.class.php index f2535f50..af7007ae 100644 --- a/oidc/class/oidcidentitymanager.class.php +++ b/oidc/class/oidcidentitymanager.class.php @@ -34,7 +34,7 @@ class OIDCIdentityManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/oidc/class/oidcmanager.class.php b/oidc/class/oidcmanager.class.php index 89f3036f..35cfea70 100644 --- a/oidc/class/oidcmanager.class.php +++ b/oidc/class/oidcmanager.class.php @@ -34,7 +34,7 @@ class OIDCManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/oidc/class/oidcusergrantmanager.class.php b/oidc/class/oidcusergrantmanager.class.php index c1d0bc13..98d2ab36 100644 --- a/oidc/class/oidcusergrantmanager.class.php +++ b/oidc/class/oidcusergrantmanager.class.php @@ -40,7 +40,7 @@ class OIDCUserGrantManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ou/class/ouassociationmanager.class.php b/ou/class/ouassociationmanager.class.php index f97424af..21416d9c 100644 --- a/ou/class/ouassociationmanager.class.php +++ b/ou/class/ouassociationmanager.class.php @@ -36,7 +36,7 @@ class OUAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/ou/class/oumanager.class.php b/ou/class/oumanager.class.php index 7538d35b..14e20403 100644 --- a/ou/class/oumanager.class.php +++ b/ou/class/oumanager.class.php @@ -36,7 +36,7 @@ class OUManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/pushbullet/class/pushbulletmanager.class.php b/pushbullet/class/pushbulletmanager.class.php index 3f772b1f..2e565248 100644 --- a/pushbullet/class/pushbulletmanager.class.php +++ b/pushbullet/class/pushbulletmanager.class.php @@ -66,7 +66,7 @@ public function createSql() 'pID', 'pToken' ]; - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, $fields, diff --git a/slack/class/slackmanager.class.php b/slack/class/slackmanager.class.php index 81ecf8f7..845533a2 100644 --- a/slack/class/slackmanager.class.php +++ b/slack/class/slackmanager.class.php @@ -36,7 +36,7 @@ class SlackManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/subnetgroup/class/subnetgroupmanager.class.php b/subnetgroup/class/subnetgroupmanager.class.php index 95d4c345..4d0d12d7 100644 --- a/subnetgroup/class/subnetgroupmanager.class.php +++ b/subnetgroup/class/subnetgroupmanager.class.php @@ -38,7 +38,7 @@ class SubnetGroupManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/tests/oidc-provider-safety.test.php b/tests/oidc-provider-safety.test.php index ebfe37ca..7db29162 100644 --- a/tests/oidc-provider-safety.test.php +++ b/tests/oidc-provider-safety.test.php @@ -223,7 +223,7 @@ function () { (new OIDCManager())->createSql(); $call = Schema::$lastCall; if (count($call) < 7) { - fail('OIDCManager::createSql() did not call Schema::createTable()'); + fail('OIDCManager::createSql() did not reach Schema::createTable()'); } else { list(, , $cols, , , $defaults, $uniques) = $call; $defaultFor = array_combine($cols, $defaults); diff --git a/tests/stubs/fog-stubs.php b/tests/stubs/fog-stubs.php index 8f3732d4..cb939cad 100644 --- a/tests/stubs/fog-stubs.php +++ b/tests/stubs/fog-stubs.php @@ -93,6 +93,23 @@ public function save() */ class FOGManagerController { + /** + * Passes a createTable() call straight through. + * + * The real one (GH-1245) fills a default into every NOT NULL column that + * has none, leaving the primary key, anything the model declares + * required, and anything whose name ends in ID. It only ADDS -- a default + * the caller passed explicitly always wins -- so a pass-through here + * cannot mask an assertion about a default a manager sets deliberately, + * which is what these tests check. Reimplementing the rule in the stub + * would only give it somewhere to drift from. + * + * @return string + */ + public function createTableSql(...$args) + { + return Schema::createTable(...$args); + } } /** diff --git a/tests/tables-carry-column-defaults.test.php b/tests/tables-carry-column-defaults.test.php new file mode 100644 index 00000000..23575f1f --- /dev/null +++ b/tests/tables-carry-column-defaults.test.php @@ -0,0 +1,147 @@ +createTableSql()` (fogproject, FOGManagerController) fills a default + * into every NOT NULL column that has none, leaving bare only the primary + * key, anything the model declares required, and anything whose name ends in + * ID. `Schema::createTable()` does not: it is the raw builder underneath, and + * calling it directly is what leaves a table mandatory throughout. Measured + * against the live 1.6 install, the difference is 31 columns. + * + * So this is a one-line rule with a real consequence, and the failure is + * silent -- a plugin whose table is wrong looks fine until someone saves a + * record without filling in an optional field. + * + * PHP version 7.4+ + * + * @category Tests + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +$root = dirname(__DIR__); +$failures = []; +$checks = 0; + +/** + * Source with comments removed, so a commented-out call can neither satisfy + * a check nor fail one -- two managers mention Schema::createTable() in + * prose, explaining how it names its indexes. + * + * @param string $file the file to read + * + * @return string + */ +function tcStrip($file) +{ + $clean = ''; + foreach (token_get_all(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; + } + + return $clean; +} + +/** + * Records a check. + * + * @param bool $ok whether it passed + * @param string $message what failed, stated as the defect + * + * @return void + */ +function tcCheck($ok, $message) +{ + global $checks, $failures; + $checks++; + if (!$ok) { + $failures[] = $message; + } +} + +$managers = []; +$it = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($root)); +foreach ($it as $file) { + $path = $file->getPathname(); + if (!preg_match('/manager\.class\.php$/', $file->getFilename())) { + continue; + } + if (false !== strpos($path, '/tests/') + || false !== strpos($path, '/.git/') + ) { + continue; + } + $managers[] = $path; +} +sort($managers); + +$builders = 0; +foreach ($managers as $path) { + $src = tcStrip($path); + if (false === strpos($src, 'createTable(') + && false === strpos($src, 'createTableSql(') + ) { + continue; + } + $builders++; + $short = str_replace($root . '/', '', $path); + tcCheck( + false === strpos($src, 'Schema::createTable('), + sprintf( + '%s calls Schema::createTable() directly, so its table is built ' + . 'with every optional column NOT NULL and no default. FOG\'s ' + . 'schema step cannot repair it -- the table is created by this ' + . 'plugin, after the step has run. Use $this->createTableSql(), ' + . 'which takes the identical arguments.', + $short + ) + ); + tcCheck( + false !== strpos($src, 'createTableSql('), + sprintf( + '%s builds a table without going through createTableSql()', + $short + ) + ); +} + +tcCheck( + $builders >= 20, + sprintf( + 'only %d table-building managers were found, so the checks above ' + . 'pass vacuously -- the scan did not reach the plugins', + $builders + ) +); + +if (count($failures)) { + fwrite(STDERR, 'FAIL (' . count($failures) . " of $checks):\n"); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + exit(1); +} + +echo "ok $checks checks passed\n"; +exit(0); diff --git a/windowskey/class/windowskeyassociationmanager.class.php b/windowskey/class/windowskeyassociationmanager.class.php index acb6003f..8d1e73f9 100644 --- a/windowskey/class/windowskeyassociationmanager.class.php +++ b/windowskey/class/windowskeyassociationmanager.class.php @@ -39,7 +39,7 @@ class WindowsKeyAssociationManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/windowskey/class/windowskeymanager.class.php b/windowskey/class/windowskeymanager.class.php index f158403f..87cfdea7 100644 --- a/windowskey/class/windowskeymanager.class.php +++ b/windowskey/class/windowskeymanager.class.php @@ -36,7 +36,7 @@ class WindowsKeyManager extends FOGManagerController */ public function createSql() { - return Schema::createTable( + return $this->createTableSql( $this->tablename, true, [ diff --git a/wolbroadcast/class/wolbroadcastmanager.class.php b/wolbroadcast/class/wolbroadcastmanager.class.php index b956f4ca..54a55c08 100644 --- a/wolbroadcast/class/wolbroadcastmanager.class.php +++ b/wolbroadcast/class/wolbroadcastmanager.class.php @@ -35,7 +35,7 @@ class WolbroadcastManager extends FOGManagerController public function install() { $this->uninstall(); - $sql = Schema::createTable( + $sql = $this->createTableSql( $this->tablename, true, [