Fix DBI::buildFieldDefinition for null defaults#80
Merged
Conversation
Before: a field declared with `'default' => null` generated invalid SQL because the is_string() check fell through and concatenated null (which PHP coerces to empty string). Result: `\`language\` varchar(5) DEFAULT NOT NULL,` with a dangling space, which MariaDB rejects with a syntax error. After: emit `DEFAULT NULL` explicitly, which is valid SQL and matches what `'default' => null` obviously intended. The Application model's `language` field carries `'default' => null`, so any fresh call to `DBI::createTable(Application)` has been blowing up since that field was introduced. The bug hasn't surfaced in production because existing customer installs created the Application table before the field was added — only fresh installs hit it. gust/lib/Database.php carries an identical copy of this bug. Patching gust's copy is explicitly out of scope for the MCP/auth migration initiative (see design doc section 11 — gust evaluation) and stays on the "fix when gust is next touched" list. Added a characterization test in tests/DatabaseTest.php that creates a throwaway table with a `'default' => null` column and asserts both success and `DEFAULT NULL` in the resulting SHOW CREATE TABLE output. The test explicitly re-selects the kytedev database because the preceding createDatabase tests leave the mysqli connection without an active database context. 31/31 unit tests green.
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.
Summary
DBI::buildFieldDefinitionso a field declared with'default' => nullemits validDEFAULT NULLSQL instead of a danglingDEFAULTthat MariaDB rejects.testCreateTableAcceptsNullDefault) covering the regression.Why
Applicationmodel'slanguagefield carries'default' => null. Any freshDBI::createTable(Application)has been blowing up since that field was added — existing customer installs are unaffected because the table already exists. The bug was uncovered while writing a Phase 2 MCP integration test that needed a freshApplicationtable.gust/lib/Database.phpcontains an identical copy of this bug. Patching gust is out of scope for this PR (separate initiative per design doc §11); noted in the commit message for the next gust-refresh session.Test plan
docker compose -f tests/docker-compose.test.yml run --rm php vendor/bin/phpunit --testsuite unitgreen locally (31/31)DEFAULT NULLappears inSHOW CREATE TABLEfor a field with'default' => null