From a5c9ea685558500110baa1b8f9f69ce16b9d1561 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Tue, 1 Sep 2026 20:07:52 -0500 Subject: [PATCH 1/2] Clear the Image field to NULL, not to the 0 the constraint refuses "Clear on all" on Image failed outright. `massEditCoreFields()` declared the field as clearing to `0`, and ADR 0031 put a foreign key on `hosts`.`hostImage` -> `images`.`imageID`. Zero is not exempt from a constraint just because it reads as an absence: no image has id 0, so the database rejected the UPDATE and the whole mass edit went with it. Verified against a live 1.6 install rather than reasoned about. The column is `int(11) NULL DEFAULT NULL`, `fk_hosts_hostImage` is applied, 77 hosts hold NULL for "no image" and NOT ONE holds 0 -- SchemaReconciler swept the legacy zeros when it added the constraint. NULL is simply what "no image" IS on any 1.6 database. Two more things had to change or the fix would not have worked: MassEdit::columnUpdates() read `$spec[$key]['empty'] ?? ''`, and `??` treats null as absent -- so the corrected spec would have been silently turned back into '', which an int column stores as the very 0 that fails. It is array_key_exists() now, so a spec that genuinely omits `empty` still gets '' while an explicit null survives as NULL. massEditPost() discarded update()'s return value. perform_update() answers false and writes a fault rather than throwing, so a refused write reported "Updated 1 field(s) on 86 host(s)" -- which is how a clear that never landed reads as a clear that did. It throws now. tests/fk-columns-clear-to-null.test.php is the gate, and it DERIVES the answer instead of restating it: it reads the twelve `'sentinel' => 0` relations out of commons/schema-constraints.php, maps each column back through Host's own $databaseFields, and requires any mass-edit field naming one of them to clear to null. A future field on a foreign-key column fails on the day it is added rather than the day somebody clears it. The map is scoped to ONE Item class deliberately: `imageID` is `hosts`.`hostImage` on Host and `tasks`.`taskImageID` on Task, and a global lookup names the wrong column. Three mutations were run and all go red: restoring `empty => 0`, restoring the `?? ''`, and discarding the update return value again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM --- packages/web/src/Pages/HostManagement.php | 25 +- packages/web/src/Util/MassEdit.php | 29 ++- tests/fk-columns-clear-to-null.test.php | 255 +++++++++++++++++++++ tests/mass-edit-endpoint-is-gated.test.php | 5 +- 4 files changed, 303 insertions(+), 11 deletions(-) create mode 100644 tests/fk-columns-clear-to-null.test.php diff --git a/packages/web/src/Pages/HostManagement.php b/packages/web/src/Pages/HostManagement.php index 588b55c5bb..65ee1eeee0 100644 --- a/packages/web/src/Pages/HostManagement.php +++ b/packages/web/src/Pages/HostManagement.php @@ -4226,9 +4226,18 @@ private function massEditSelection() private function massEditCoreFields() { return [ + // NULL, not 0. `hosts`.`hostImage` carries a foreign key to + // `images`.`imageID` (ADR 0031, schema-constraints.php:145) and + // 0 is not exempt from a constraint just because it looks like + // an absence -- there is no image with id 0, so clearing to 0 is + // rejected outright and the whole mass edit fails. The column is + // nullable and the reconciler already swept every legacy 0 to + // NULL, so NULL is what "no image" actually IS on any 1.6 + // database. Verified against a live install: 77 hosts hold NULL + // and none holds 0. 'image' => [ 'field' => 'imageID', - 'empty' => 0, + 'empty' => null, 'label' => _('Image'), 'kind' => 'image', 'tab' => 'general' @@ -5143,8 +5152,18 @@ public function massEditPost() // non-empty because an UPDATE with no assignments is either // a syntax error or, worse, a statement whose WHERE is the // only part left. - self::getClass('HostManager') - ->update(['id' => $hosts], '', $updates); + // The return value is CHECKED. perform_update() answers + // false and writes a fault rather than throwing, so ignoring + // it reported "Updated 1 field(s) on 86 host(s)" for a write + // the database had refused -- which is how a clear that + // never landed reads as a clear that did. + if (!self::getClass('HostManager') + ->update(['id' => $hosts], '', $updates) + ) { + throw new \Exception( + _('The database refused the update; nothing was changed') + ); + } $affected = count($hosts); } // Row-backed fields count toward the same affectedCount: one diff --git a/packages/web/src/Util/MassEdit.php b/packages/web/src/Util/MassEdit.php index fca97ad5e5..34fe128ffa 100644 --- a/packages/web/src/Util/MassEdit.php +++ b/packages/web/src/Util/MassEdit.php @@ -231,8 +231,11 @@ public static function resolveComposite(array $keys, $actions, $values) * @param array $resolved output of resolve() * @param array $spec key => ['field' => , * 'empty' => ]. - * A key absent from the spec is skipped: the spec - * is what says a key may touch a column at all. + * `empty` may be null, and null is written as + * NULL rather than treated as absent -- that is + * what a column with a foreign key needs. A key + * absent from the spec is skipped: the spec is + * what says a key may touch a column at all. * * @return array field name => value, ready for a manager update(). EMPTY * when nothing was set or cleared -- and an empty map must @@ -260,11 +263,23 @@ public static function columnUpdates(array $resolved, array $spec) $updates[$spec[$key]['field']] = $instruction['value']; } elseif (self::CLEAR === $action) { // Per field, because "empty" is not one value. A varchar - // column clears to '', an int foreign key to 0; writing '' - // into an int column stores 0 on a permissive server and - // errors on a strict one, so the answer belongs in the spec - // rather than in a cast here. - $updates[$spec[$key]['field']] = $spec[$key]['empty'] ?? ''; + // column clears to '', a plain int to 0, and a column + // carrying a FOREIGN KEY to null -- 0 is not exempt from a + // constraint, so clearing `hosts`.`hostImage` to 0 is + // rejected outright unless an image with id 0 exists, which + // none does. The answer belongs in the spec rather than in a + // cast here. + // + // array_key_exists, NOT `?? ''`. Null is the whole point for + // the foreign-key case and `??` treats it as absent, so the + // coalesce silently turned every intended NULL back into '' + // -- which an int column then stores as 0, i.e. exactly the + // value that fails. A spec that genuinely omits `empty` + // still gets ''. + $updates[$spec[$key]['field']] = + array_key_exists('empty', $spec[$key]) + ? $spec[$key]['empty'] + : ''; } } diff --git a/tests/fk-columns-clear-to-null.test.php b/tests/fk-columns-clear-to-null.test.php new file mode 100644 index 0000000000..f42525226a --- /dev/null +++ b/tests/fk-columns-clear-to-null.test.php @@ -0,0 +1,255 @@ + 0` entry in + * commons/schema-constraints.php saying 0 was once its "none" value. Every + * one of them rejects a literal 0 today. A hand-written list would be a + * second place that has to agree with the constraint file, and the failure + * when it did not agree would be exactly this bug again, silently. So the + * constraint file is read, the column is mapped back to its FOGController + * field name through the Items' own $databaseFields, and any mass-edit spec + * naming one of those fields is required to clear it to null. + * + * A NEW field added to the spec for an FK-bearing column fails here on the + * day it is added rather than on the day somebody clears it. + * + * The second half is executed rather than grepped: columnUpdates() used + * `$spec[$key]['empty'] ?? ''`, and `??` treats null as absent -- so even + * with the spec corrected, the coalesce turned the intended NULL straight + * back into '', which an int column stores as 0. The fix and the spec change + * are worthless without each other, so both are checked. + * + * Usage: php tests/fk-columns-clear-to-null.test.php + * Exit status 0 = pass, 1 = fail. + * + * PHP version 7.4+ + * + * @category Tests + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +require_once __DIR__ . '/lib/fog-test-harness.php'; + +// boot() owns the temp dirs, the constants, Initiator and the language +// globals -- the same preamble every other test in the suite uses, rather +// than a hand-rolled copy that drifts from it. +FogTestHarness::boot('fk-clear'); + +$root = dirname(__DIR__); +$webroot = $root . '/packages/web'; + +use FOG\Util\MassEdit; + +$checks = 0; +$failures = []; +$check = static function ($what, $ok) use (&$checks, &$failures) { + $checks++; + if (!$ok) { + $failures[] = $what; + } +}; + +// --- The FK-bearing columns, straight off the constraint file -------------- + +$constraintFile = $webroot . '/commons/schema-constraints.php'; +$constraintSrc = (string)file_get_contents($constraintFile); +$check( + 'the constraint file is readable', + '' !== $constraintSrc +); + +// Every relation that declares a 0 sentinel: the constraint file saying so +// IS the statement that 0 used to mean "none" here and no longer may. +preg_match_all( + "/'child'\s*=>\s*'([^']+)'\s*,\s*'column'\s*=>\s*'([^']+)'" + . "(?=[^\]]*'sentinel'\s*=>\s*0)/", + $constraintSrc, + $m, + PREG_SET_ORDER +); +$sentinelColumns = []; +foreach ($m as $hit) { + $sentinelColumns[strtolower($hit[2])] = $hit[1]; +} +$check( + 'the constraint file still declares sentinel columns (' + . count($sentinelColumns) . ' found)', + count($sentinelColumns) > 0 +); + +// --- Map each column back to the field name a spec would name -------------- + +// Scoped to ONE Item class, because a field name is only unique within one. +// `imageID` is `hosts`.`hostImage` on Host and `tasks`.`taskImageID` on Task, +// and a global map keyed on the field name silently picks whichever file was +// read last -- which would name the wrong column in a failure message and, +// worse, would flag a field that is a plain int here because it is a foreign +// key somewhere else. +$fieldMap = static function ($itemClass) use ($webroot) { + $src = (string)@file_get_contents( + $webroot . '/src/Items/' . $itemClass . '.php' + ); + $map = []; + if ('' === $src + || !preg_match( + '/\$databaseFields\s*=\s*\[(.*?)\n \];/s', + $src, + $block + ) + ) { + return $map; + } + preg_match_all( + "/'([A-Za-z0-9_]+)'\s*=>\s*'([A-Za-z0-9_]+)'/", + $block[1], + $pairs, + PREG_SET_ORDER + ); + foreach ($pairs as $pair) { + $map[$pair[1]] = strtolower($pair[2]); + } + return $map; +}; + +// massEditCoreFields() names Host's fields, so Host's map is the right one. +$hostFields = $fieldMap('Host'); +$check( + 'Host\'s databaseFields map was parsed (' . count($hostFields) . ' fields)', + count($hostFields) > 10 +); +$fieldsForColumn = []; +foreach ($hostFields as $field => $column) { + if (isset($sentinelColumns[$column])) { + $fieldsForColumn[$field] = $column; + } +} +$check( + 'at least one sentinel column maps to a field name (' + . implode(', ', array_keys($fieldsForColumn)) . ')', + count($fieldsForColumn) > 0 +); +$check( + 'imageID maps to `hosts`.`hostImage` -- the field the original bug was on', + ($fieldsForColumn['imageID'] ?? null) === 'hostimage' +); + +// --- No mass-edit spec may clear one of them to anything but null --------- + +$class = \FOG\Pages\HostManagement::class; +$page = (new \ReflectionClass($class))->newInstanceWithoutConstructor(); +$call = static function ($method) use ($class, $page) { + $m = new \ReflectionMethod($class, $method); + $m->setAccessible(true); + return $m->invoke($page); +}; +$core = $call('massEditCoreFields'); + +$wrong = []; +$guarded = 0; +foreach ($core as $key => $spec) { + if (!isset($spec['field']) || !isset($fieldsForColumn[$spec['field']])) { + continue; + } + $guarded++; + // array_key_exists, not isset: a spec that correctly says null must not + // read here as a spec that forgot to say anything. + if (!array_key_exists('empty', $spec) || null !== $spec['empty']) { + $wrong[] = $key . ' (' . $spec['field'] . ' -> `' + . $fieldsForColumn[$spec['field']] . '`)'; + } +} +$check( + 'the spec actually covers a foreign-key field, or this test proves nothing', + $guarded > 0 +); +$check( + 'every foreign-key field clears to NULL, not to a sentinel (' + . implode(', ', $wrong) . ')', + 0 === count($wrong) +); + +// --- columnUpdates() must PRESERVE that null ------------------------------ + +$resolved = MassEdit::resolve( + ['image'], + ['image' => MassEdit::CLEAR], + [] +); +$check( + 'a clear with no posted value still resolves to CLEAR', + MassEdit::CLEAR === $resolved['image']['action'] +); +$updates = MassEdit::columnUpdates($resolved, $core); +$check( + 'clearing the image reaches the column map at all', + array_key_exists('imageID', $updates) +); +$check( + 'and reaches it as NULL -- `?? \'\'` would have made it the empty string,' + . ' which an int column stores as the 0 the constraint refuses', + array_key_exists('imageID', $updates) && null === $updates['imageID'] +); + +// A spec that genuinely omits `empty` still gets the empty string, so the +// change above did not quietly turn every unspecified field into NULL. +$plain = MassEdit::columnUpdates( + MassEdit::resolve(['thing'], ['thing' => MassEdit::CLEAR], []), + ['thing' => ['field' => 'thing']] +); +$check( + 'a spec with no `empty` at all still clears to the empty string', + array_key_exists('thing', $plain) && '' === $plain['thing'] +); +// And an explicit 0 is still an explicit 0, for the plain int columns that +// genuinely clear that way. +$zero = MassEdit::columnUpdates( + MassEdit::resolve(['lvl'], ['lvl' => MassEdit::CLEAR], []), + ['lvl' => ['field' => 'lvl', 'empty' => 0]] +); +$check( + 'an explicit 0 empty is still written as 0', + array_key_exists('lvl', $zero) && 0 === $zero['lvl'] +); + +// --- The write must not report success when the database refused ---------- + +$source = (string)file_get_contents($webroot . '/src/Pages/HostManagement.php'); +$check( + 'massEditPost() checks the update return rather than discarding it', + 1 === preg_match( + '/if\s*\(\s*!\s*self::getClass\(\s*.HostManager.\s*\)\s*' + . '->update\(\s*\[\s*.id.\s*=>\s*\$hosts\s*\]/s', + $source + ) +); + +if (count($failures)) { + fwrite( + STDERR, + "FAIL: a mass edit would write a sentinel into a foreign-key column:\n" + ); + foreach ($failures as $f) { + fwrite(STDERR, " - $f\n"); + } + fwrite( + STDERR, + sprintf("%d of %d checks failed\n", count($failures), $checks) + ); + exit(1); +} + +printf("PASS foreign-key columns clear to NULL: %d checks\n", $checks); +exit(0); diff --git a/tests/mass-edit-endpoint-is-gated.test.php b/tests/mass-edit-endpoint-is-gated.test.php index d73df8f9fa..301570a793 100644 --- a/tests/mass-edit-endpoint-is-gated.test.php +++ b/tests/mass-edit-endpoint-is-gated.test.php @@ -111,7 +111,10 @@ $auth = $at('self::checkAuthAndCSRF();'); $scope = $at("Authorization::requirePageObjectScopeMass('host', \$hosts);"); $resolve = $at('$resolved = MassEdit::resolve('); -$write = $at("->update(['id' => \$hosts], '', \$updates);"); +// The call is wrapped in `if (!...)` now, so the statement no longer ends in +// `;` right after the arguments -- the write itself is unchanged, and this +// anchors on the arguments rather than on the punctuation around them. +$write = $at("->update(['id' => \$hosts], '', \$updates)"); $hook = $at("'HOST_MASSEDIT_APPLY',"); $check('it checks auth and CSRF', false !== $auth); From 5c7fe07092280e9bf2310491de6e9e9f5f3aad99 Mon Sep 17 00:00:00 2001 From: "fog-workflows[bot]" Date: Wed, 2 Sep 2026 01:09:00 +0000 Subject: [PATCH 2/2] Generated Files Sync: regenerate translations and PSR2 formatting (#1655) --- .../management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/en_US.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po | 3 +++ packages/web/management/languages/messages.pot | 3 +++ .../management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po | 3 +++ .../management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po | 3 +++ 10 files changed, 30 insertions(+) diff --git a/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po index b611c179e8..a5c1dc68f5 100644 --- a/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/de_DE.UTF-8/LC_MESSAGES/messages.po @@ -9271,6 +9271,9 @@ msgstr "Fehler beim Erstellen eines Tasks" msgid "The current user is invalid." msgstr "Task-Typ ist nicht gültig" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po index 2c1e9f92e7..b9bbc68341 100644 --- a/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/en_US.UTF-8/LC_MESSAGES/messages.po @@ -9280,6 +9280,9 @@ msgstr "Failed to create task" msgid "The current user is invalid." msgstr "Task type is not valid" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po index 0fc0a3b51d..e827bdc418 100644 --- a/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/es_ES.UTF-8/LC_MESSAGES/messages.po @@ -9443,6 +9443,9 @@ msgstr "No se pudo crear la tarea" msgid "The current user is invalid." msgstr "tipo de tarea no es válida" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po index c259762c71..d6a14afcd7 100644 --- a/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/eu_ES.UTF-8/LC_MESSAGES/messages.po @@ -9272,6 +9272,9 @@ msgstr "Fehler beim Erstellen eines Tasks" msgid "The current user is invalid." msgstr "Task-Typ ist nicht gültig" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po index ff229d8bc6..a2e3b668f2 100644 --- a/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/fr_FR.UTF-8/LC_MESSAGES/messages.po @@ -9265,6 +9265,9 @@ msgstr "Impossible de créer la tâche" msgid "The current user is invalid." msgstr "Type de tâche n'est pas valide" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po index 66db4d5575..065253cd5b 100644 --- a/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/it_IT.UTF-8/LC_MESSAGES/messages.po @@ -8986,6 +8986,9 @@ msgstr "Impossibile creare un'attività" msgid "The current user is invalid." msgstr "Tipo di attività non è valido" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po index 29a029f68a..dd03a306ec 100644 --- a/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/ja_JP.UTF-8/LC_MESSAGES/messages.po @@ -8929,6 +8929,9 @@ msgstr "タスクの作成に失敗しました" msgid "The current user is invalid." msgstr "" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/messages.pot b/packages/web/management/languages/messages.pot index 8e4fa039a4..5397aa7b0a 100644 --- a/packages/web/management/languages/messages.pot +++ b/packages/web/management/languages/messages.pot @@ -7905,6 +7905,9 @@ msgstr "" msgid "The current user is invalid." msgstr "" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po index 49cf83b80c..e675c44999 100644 --- a/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/pt_BR.UTF-8/LC_MESSAGES/messages.po @@ -9267,6 +9267,9 @@ msgstr "Falha ao criar tarefa" msgid "The current user is invalid." msgstr "tipo de tarefa não é válido" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr "" diff --git a/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po b/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po index 084d8e4326..5fd5afffa1 100644 --- a/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po +++ b/packages/web/management/languages/zh_CN.UTF-8/LC_MESSAGES/messages.po @@ -9267,6 +9267,9 @@ msgstr "无法创建任务" msgid "The current user is invalid." msgstr "任务类型无效" +msgid "The database refused the update; nothing was changed" +msgstr "" + msgid "The default printer for hosts in this group. A host that has its own default keeps it." msgstr ""