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, [