Skip to content

fix(db): resolve a null in a typed write against the column it is going into - #6105

Merged
Deltik merged 4 commits into
masterfrom
e107help/6104
Aug 28, 2026
Merged

fix(db): resolve a null in a typed write against the column it is going into#6105
Deltik merged 4 commits into
masterfrom
e107help/6104

Conversation

@e107help

@e107help e107help Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Why

Reported by @Alex-e107nl in #6104: creating a download saves nothing. The form reloads with no message, and with debug on the site reports SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'download_thumb' cannot be null. Reproduced on master of 27 August in the harness on PHP 8.5 with MariaDB 10.11: fresh install, download plugin, one category, a name typed into Downloads > Create, and the table stays empty.

The form only draws the thumbnail field where e_FILE."downloadthumbs" exists (admin.php:1534, a path from the 0.7 era), so on an ordinary site the form posts no download_thumb at all. e_parse::toDB() returns anything that is not a string or an array unchanged, so the absent key reaches admin.php:1829 as null and goes into a text NOT NULL column. MySQL and MariaDB reject a single-row INSERT of NULL into a NOT NULL column whatever the sql_mode, so the row never lands.

It used to work because the deprecated array-form insert() filled every _NOTNULL column whose value was unset or null before binding: the test is isset(), and isset(null) is false. 9cf3520df2 (#5811) moved that call to the query builder, which binds what it is handed.

The API gap underneath is the part worth fixing. insertGetId() hard-wired values(), so the one shorthand that returns the new row's id was the one shorthand that could not reach the typed path. Of the 71 array-form insert() calls #5811 converted, 82 conversions took valuesTyped(), 27 took values(), and 15 took insertGetId() and had no choice. Every one of those 15 is a migrated insert(), which read its row against the table's own definition; all 15 were put on a literal bind instead. Download create is one of them.

The same defect shape was met twice inside #5811 itself and worked around at the call site, in userclass_class.php (userclass_perms) and xml_class.php (import rows). This is the third instance, so the fix goes in the write path.

What Changed

Four commits, and no call site changes, the download plugin included.

  1. getNotNullDefaults() answers the one fact a typed write is missing: does this column accept a null, and if not, what does it hold instead. _NOTNULL looks like that answer and is not. makeTableDef() files a column there only when it is NOT NULL and declares no DEFAULT, so varchar(100) NOT NULL default '' is absent from it, and a rule built on it would fix text columns and leave every varchar exposed. The new map is every NOT NULL column with its declared default, or '' where it declares none. Nullable columns stay out, since those are the ones a caller may legitimately want NULL in; AUTO_INCREMENT columns stay out too, since the server already reads a null there as "assign one" and a stand-in of 0 would hand back an id of 0 under NO_AUTO_VALUE_ON_ZERO.
  2. A null in a typed write is resolved against the column it is going into. A typed write is one that has asked for the table's own definition to apply; since feat(db): typed writes take their field types from the table they write to #5926 that means the per-column storage transform, and it now also means a null bound to a NOT NULL column is read as the absence of a value and takes that column's stand-in. Everything else keeps SQL NULL, and both explicit spellings of SQL NULL are left alone: the 'null' field type and the '_NULL_' sentinel. This cannot overwrite an intended NULL, because the map holds only columns that cannot hold one. values() and set() are the literal path and are untouched.
  3. insertGetId() stops choosing for the caller. The argument is optional, so the id is orthogonal to how the row was built (->valuesTyped($row)->insertGetId(), ->values($row)->insertGetId()). A row passed to it goes through valuesTyped(), because that is what $sql->insert($table, $row) meant and because every existing caller is one of those 15 migrated calls; that restores all 15 at once. A list of rows is now refused: there is no one id to return for it.
  4. The acceptance test that would have caught it. Nothing in the suite posted submit_download or opened admin_download.php at all, which is why six weeks of green CI said nothing.

The map is read from the table as it stands, not from a cached definition, and that is deliberate. Nothing clears e_CACHE_DB when db_verify repairs a column or an update routine adds one, so a cached map would miss a NOT NULL column added since the file was written, reproducing this very bug and permanently, and would keep a stand-in for a column since made nullable and quietly store '' where the caller meant NULL. The read costs one SHOW COLUMNS per table per request, on its own connection because the caller may be part way through a result set of its own, and only on a write that actually binds a null. A write that binds none never asks.

How It Was Tested

  • e_db_abstractTest, so both backends: getNotNullDefaults() read off a real table carrying an auto-increment key, a defaulted column, a default with a space in it, a defaultless TEXT and a nullable column, asserting exactly which three make the map; the same table with a NOT NULL column added by ALTER TABLE after the definition was put on record, which is the staleness case the live read exists for; an unknown table answering with an empty map; and the round trip this PR is about, a typed insert of null into TEXT NOT NULL landing as '' while the nullable column in the same row still gets NULL, with a literal values() insert of the same null as the control.
  • QueryBuilderTest: the stand-in applied before the field-type transform so an int column's stand-in still lands as an int; a nullable column still bound NULL; setTyped() on an UPDATE treated the same as an INSERT; the 'null' field type still binding SQL NULL; values() binding a null without asking the connection at all; the map read once for a whole row; insertGetId() reading its row against the table, taking the row the query already carries, and refusing a non-array and a list of rows.
  • 0079_DownloadCreateCest drives the real form rather than a hand-built POST, and asserts the absence of the download_thumb field as the condition under test. It covers the edit form as well, which reaches the same submit_download() through EditPage() deferring to CreatePage().
  • Revert-checks on this branch, each with the tests kept: back out the null resolution and both Cest tests fail, along with 4 QueryBuilderTest cases and 1 in each backend; back out the insertGetId change alone and both Cest tests fail again. So neither commit is carrying the other.
  • Full suites on PHP 8.5 with MariaDB 10.11: unit Tests: 2203, Assertions: 23312, Skipped: 4, acceptance OK (510 tests, 2933 assertions). The four skips are pre-existing.
  • php-floor-lint: 2063 files OK on PHP 5.6. Rector downgrade fixed point: converged in one pass with no diff.

Backwards Compatibility

  • ConnectionInterface gains getNotNullDefaults(), implemented once in ConnectionTrait so both backends carry it. No cached definition changes shape; _NOTNULL and _FIELD_TYPES are untouched, so the deprecated array-form insert() behaves exactly as before.
  • QueryBuilder::insertGetId() loses its array type hint and gates at runtime instead, per the ruling on feat(db): typed writes take their field types from the table they write to #5926 that a nullable parameter has no spelling PHP 5.6 and PHP 8.4 both accept. Passing a list of rows to it used to be accepted and now throws; passing one row now applies the table's field types, which is what the array-form insert() these 15 call sites came from always did. That form also filled NOT NULL columns a row left out altogether, which this does not: at e107's session sql_mode the server substitutes the same implicit default for an omitted column, so the two agree in practice, though a strict mode would not.
  • Behaviour: a typed write that used to fail with 1048 now succeeds with the column's default, and the same write on an UPDATE no longer depends on the session running with NO_ENGINE_SUBSTITUTION to be coerced by the server. A write that used to succeed is unaffected.
  • Not in this diff, and worth knowing: submit_download() writes its update through set() rather than a typed write, so editing a download still binds a raw null for download_thumb and still relies on the non-strict session mode to coerce it to ''. It works, and the new acceptance test covers it, but it is the same shape one line away from the insert this fixes. The other set()-loop call sites feat(db): query builder, core migration, and SQL-injection CI gate #5811 converted are in the same position.

AI Model (Optional)

Claude Opus 5 (e107help)

Checklist

  • One issue per PR: the diff is scoped to this change only
  • Commit messages explain why, not just what
  • New or changed behavior has test coverage (or explain why not)
  • No unrelated reformatting, renames, or import reordering

A typed write about to bind a null needs one fact the connection could not
supply: does this column accept one, and if not, what does it hold instead.

'_NOTNULL' looks like that answer and is not. makeTableDef() files a column
there only when it is NOT NULL *and* declares no DEFAULT, so
`varchar(100) NOT NULL default ''` is absent from it. The array-form insert()
got away with the gap because it used the map to fill columns a row had left
out altogether and could leave the rest to the server's own defaults.

getNotNullDefaults() answers it properly: every NOT NULL column of the table,
mapped to its declared DEFAULT or to '' where it declares none. Nullable
columns stay out, because those are the ones a caller may legitimately want
NULL in. AUTO_INCREMENT columns stay out too, because the server already reads
a null there as "assign one", where a stand-in of 0 would insert a literal zero
and hand back an id of 0 under NO_AUTO_VALUE_ON_ZERO.

It reads the table as it stands rather than a cached definition. Nothing clears
e_CACHE_DB when db_verify repairs a column or an update routine adds one, so a
cached map would miss a NOT NULL column added since the file was written, and
would keep a stand-in for a column since made nullable and quietly store ''
where the caller meant NULL. Both are silent and last as long as the file does,
which is the shape of the bug this exists to fix rather than an acceptable price
for avoiding a query.

That query is one SHOW COLUMNS per table per request, run on its own connection
because the caller may be part way through a result set of its own. A write that
binds no null never asks for it.

Nothing consumes the map yet.
…ng into

A typed write is one that has asked for the table's own definition to apply.
Since #5926 that means the per-column storage transform; it should also mean
that a null is read the way the column reads it. A column the table declares
NOT NULL cannot hold one, so a null bound there is not a value but the absence
of one, and it now takes that column's stand-in. Every other column takes the
null through to SQL NULL, unchanged.

This is the sentence valuesTyped() already half-implemented rather than a new
behaviour bolted onto the builder. values() and set() are the literal path and
are untouched: they bind what they are handed, and a caller who wants that keeps
it.

Two properties worth stating. Nothing here can overwrite an intended NULL,
because the map holds only columns that cannot hold one; and the 'null' field
type is exempt, so a caller who has spelled out that empty means SQL NULL still
gets SQL NULL, and still hears from the server when the column refuses it.

It also settles a disagreement between the statement types. The server rejects
a single-row INSERT of NULL into a NOT NULL column whatever the sql_mode, and
quietly coerces the same NULL on UPDATE while the mode is not strict, so the
identical row wrote two different outcomes depending on the verb. Resolving it
in the builder makes them agree and stops the UPDATE case depending on e107
running with NO_ENGINE_SUBSTITUTION.

The map is read at most once per table per query, from the same getFieldDefs()
lookup valuesTyped() already makes for its field types, so a row of twenty
columns costs nothing extra.
insertGetId() hard-wired values(), so the one shorthand that returns the new
row's id was the one shorthand that could not reach the typed path. That is not
a small ergonomic wrinkle: it is the reason discussion #6104 happened. When the
core call sites moved off the deprecated array-form insert() in 9cf3520, the
71 conversions split by whether the caller used the return value, and the 15
that did had nowhere to go but insertGetId(). Every one of them is a migrated
insert(), which read its row against the table's own definition; all 15 were
quietly put on a literal bind instead, field-type transforms and all. The
download create form is one of them, and its absent download_thumb has been
reaching MySQL as a literal NULL ever since.

So the argument becomes optional and composable. With no argument the row is
whichever one the query already carries, which puts the id back where it
belongs: orthogonal to how the row was built, rather than a fork in the road
that decides it.

	$qb->insert('download')->valuesTyped($row)->insertGetId();
	$qb->insert('download')->values($row)->insertGetId();

A row passed to it goes through valuesTyped(), because that is what
$sql->insert($table, $row) meant, and because every existing caller of this
method is one of those migrated calls. Binding a row literally stays available
by naming values() and calling this with no argument.

A list of rows is now refused rather than accepted: there is no one id to
return for it.

The parameter loses its `array` hint, since a nullable one has no spelling both
PHP 5.6 and PHP 8.4 accept, and is gated at runtime instead.
The coverage gap is the reason discussion #6104 ran for six weeks before anyone
noticed. Nothing in the suite posted submit_download or opened
admin_download.php at all, so the create form stopped saving and every job
stayed green.

The test drives the real form rather than a hand-built POST, so it also pins the
condition the report turned on: the thumbnail field is drawn only where
e_FILE."downloadthumbs" exists, and on an ordinary site the form therefore posts
no download_thumb at all. That absence is asserted, so if the form ever changes
the case being covered has to be looked at again rather than quietly slipping.

Only the create form is covered. Editing a download is served by admin_ui out of
the $fields array rather than by submit_download(), so it is a different write.
@Deltik
Deltik merged commit d1ada52 into master Aug 28, 2026
86 checks passed
@Deltik
Deltik deleted the e107help/6104 branch August 28, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants