Clear the Image field to NULL, not to the 0 the constraint refuses - #1655
Merged
Merged
Conversation
"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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM
mastacontrola
added a commit
to FOGProject/fog-plugins
that referenced
this pull request
Sep 2, 2026
The core half of this is FOGProject/fogproject#1655: "Clear on all" on Image failed because the mass edit cleared `hosts`.`hostImage` to 0 and a foreign key refuses a 0 that names no row. Asked to sweep for the same shape here, and it found one. LocationDeleteMassItems::deletemassitems() cleaned up after a deleted storage node with ->update(['storagenodeID' => $arguments['itemIDs']], '', 0) where the third argument is the UPDATE DATA. FOGManagerController::update() takes an associative array of columns, or an array of them, and returns false for anything else -- so the call had ALWAYS been a silent no-op, and had it ever worked, 0 is precisely the value the constraint now refuses. `location`.`lStorageNodeID` is nullable with ON DELETE SET NULL, so it writes null now. Kept rather than left to the constraint alone: a server between deploying code and running the schema updater has the column and not the foreign key, and there this is the only thing doing the work. The storagegroup arm above it was the same no-op and is removed, because there is nothing it could ever have done. `lStorageGroupID` is NOT NULL with a RESTRICT foreign key -- confirmed on a live install -- so a storage group any location still points at cannot be deleted at all, and the database refusing it is the intended answer. A location without a storage group is not a valid location. The line read as cleanup that made the delete safe; it never ran. tests/fk-writes-are-null-not-zero.test.php gates all 23 plugin-owned columns that core's schema-constraints.php constrains, not just the three `config` ones where a sentinel was ever plausible -- enumerating only the risky half is how the next one gets missed. It also refuses update($find, $op, <scalar>) anywhere in the tree, which is the shape that made this a no-op and which no caller can have meant. Comments are stripped with the tokenizer first, so documenting a defect does not trip the check that catches it. The field-name mapping is asserted, not trusted: renaming a field behind a constrained column fails here rather than quietly making every grep match nothing. Four mutations were run. The first version of the scalar-update check did NOT catch the real bug -- `\[[^\]]*\]` cannot span the inner `]` in `$arguments['itemIDs']`, so it skipped the exact line it existed for and passed. Bounded on `;` instead, it goes red. Also checked and clean: capone already migrated cImageID/cOSID off the 0 sentinel and guards with isValid() before writing; tasktypeedit and taskstateedit own no tables and delete through core's shared path, which already explains a refusal via ConstraintViolation::explain(). Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported from the UI: "Clear on all" on Image fails. It does, and here is why.
The bug
massEditCoreFields()declared Image as clearing to0. ADR 0031 put a foreign key onhosts.hostImage→images.imageID, and zero is not exempt from a constraint just because it reads as an absence — no image has id 0, so the database rejects the UPDATE and the whole mass edit fails.Verified against a live 1.6 install rather than reasoned about:
SchemaReconcilerswept the legacy zeros when it added the constraint, so NULL is what "no image" IS on any 1.6 database.Two more changes, or the fix would not have worked
MassEdit::columnUpdates()used$spec[$key]['empty'] ?? ''.??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 isarray_key_exists()now: a spec that genuinely omitsemptystill gets'', an explicitnullsurvives as NULL, and an explicit0is still0.massEditPost()discardedupdate()'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)". That is how a clear that never landed reads as a clear that did. It throws now.The gate derives the answer rather than restating it
tests/fk-columns-clear-to-null.test.phpreads the twelve'sentinel' => 0relations out ofcommons/schema-constraints.php, maps each column back through Host's own$databaseFields, and requires any mass-edit field naming one of them to clear tonull. A future field on a foreign-key column fails on the day it is added, not the day somebody clears it.The map is scoped to one Item class deliberately —
imageIDishosts.hostImageon Host andtasks.taskImageIDon Task, and a global lookup names the wrong column (my first cut did exactly that).Three mutations, all red:
'empty' => 0?? ''Blast radius
All twelve sentinel columns are nullable, carry a live FK, and hold zero rows with
0today — so every one of them would reject a literal 0. A sweep ofpackages/web/found no other writer; several paths (RegisterClient,MulticastManager,Image) already writenullcorrectly. Image was the only instance.The plugin half is FOGProject/fog-plugins#37.
sh tests/run-all.sh301/301; both phpstan configs clean.🤖 Generated with Claude Code
https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM