Repository navigation
docs(api): validate Migrator section - #2463
Conversation
| createTable("articles") { | ||
| t.string("title"); | ||
| t.text("body"); | ||
| t.text(columnNames="excerpt", allowNull=true); | ||
| t.timestamps(); | ||
| }; |
There was a problem hiding this comment.
🔴 Multiple new examples use Rails ActiveRecord-style block syntax — createTable("name") { t.string(...); ... }; — which is not valid CFML and will produce a parse/runtime error if copied. createTable() returns a TableDefinition object and does not accept a closure; t is undefined inside the block; and there's no t.create() to emit DDL. Affects three new files: tabledefinition/text.txt example #6 (lines 17-22), tabledefinition/float.txt example #5 (lines 14-18), and migration/announce.txt example #1 (lines 5-9). Replace with the assignment-style idiom used elsewhere in this PR: t = createTable(name="..."); t.string(...); ...; t.create();.
Extended reasoning...
Summary
Three of the new reference examples added in this PR use a Rails ActiveRecord migration block syntax that is not valid CFML and will not run as written. The pattern in question:
createTable("articles") {
t.string(columnNames="title", null=false);
t.text("body");
t.timestamps();
};appears in three places:
vendor/wheels/public/docs/reference/tabledefinition/text.txtexample Added the cfheader status code message so Search Engines know the site is #6 (lines 17-22)vendor/wheels/public/docs/reference/tabledefinition/float.txtexample New master - Microsoft Access Adapter #5 (lines 14-18)vendor/wheels/public/docs/reference/migration/announce.txtexample New master #1 (lines 5-9)
Why the syntax is wrong
Looking at the actual function definition in vendor/wheels/migrator/Migration.cfc:55-63:
public TableDefinition function createTable(
required string name,
boolean force = "false",
boolean id = "true",
string primaryKey = "id"
) {
arguments.adapter = this.adapter;
return CreateObject("component", "TableDefinition").init(argumentCollection = arguments);
}createTable() accepts only scalar arguments and returns a TableDefinition object. It does not accept a closure/block argument and does not invoke any callback. CFScript also does not allow a function-call expression to be followed by a curly-brace body — that grammar simply does not exist in the language.
Step-by-step proof of the failure
If a user copies the announce.txt example #1 into a migration and runs wheels dbmigrate latest, here is what happens:
- The CFML parser reaches
createTable(name="articles", force=true). This is a complete expression-statement — it returns aTableDefinitionvalue which is then discarded. - The parser then sees
{ t.string(...); ...};. Because CFScript does not have function-call-with-block syntax, this is parsed either as a struct literal or as a free-standing block, neither of which is what the author intended. On most engines (Lucee, ACF) this produces a compile-time syntax error. - Even if the engine somehow tolerated the trailing block, every reference to
tinside it (t.string(...),t.text(...),t.timestamps()) would throwVariable T is undefinedbecause nothing in the surrounding scope bindst. - There is also no
t.create()call, which is the method that actually emits theCREATE TABLEDDL. So even if everything else worked, no table would be created.
Existing correct idiom in this PR
The rest of the PR (createtable.txt, biginteger.txt, binary.txt, boolean.txt, char.txt, date.txt, datetime.txt, decimal.txt, integer.txt, references.txt, string.txt, time.txt, timestamp.txt, timestamps.txt, uniqueidentifier.txt, etc.) uses the correct Wheels-CFML idiom:
t = createTable(name="articles");
t.string(columnNames="title", limit=255, allowNull=false);
t.text(columnNames="body");
t.timestamps();
t.create();Suggested fix
Rewrite the three offending examples in this style. While doing so, also note that announce.txt example #1 line 6 uses null=false — the documented parameter name is allowNull (used everywhere else in this PR), so that should also be updated.
Impact
These are public-facing reference docs. A new user who copies any of these three examples will get a parse error or a runtime "T is not defined" error — and the docs as written will mislead them into thinking the framework supports a Rails-style DSL it does not.
| createTable(name="articles", force=true) { | ||
| t.string(columnNames="title", null=false); |
There was a problem hiding this comment.
🔴 In announce.txt, two examples pass null=false (line 6) and null=true (line 28) — but neither TableDefinition.string() nor addColumn() has a null parameter; the correct argument name is allowNull. CFML silently accepts unknown named arguments, so users copy/pasting these examples will get column definitions with the default nullability instead of the intended constraint, and every other example added in this PR consistently uses allowNull. Replace null=false with allowNull=false on line 6 and null=true with allowNull=true on line 28.
Extended reasoning...
What the bug is
The two examples in the new announce.txt file pass an unrecognized null keyword argument:
- Line 6:
t.string(columnNames="title", null=false); - Line 28:
addColumn(table="users", columnType="string", columnName="apiKey", limit=64, null=true);
Both call sites are documenting how to constrain (or relax) NULLability on a column, but the actual parameter is named allowNull, not null.
Why this is wrong
I verified both function signatures directly:
vendor/wheels/migrator/TableDefinition.cfc:258—public any function string(string columnNames, any limit, string default, boolean allowNull)vendor/wheels/migrator/Migration.cfc:164-174—public void function addColumn(required string table, required string columnType, required string columnName = "", string afterColumn = "", string referenceName = "", string default, boolean allowNull, numeric limit, numeric precision, numeric scale)
Neither function declares a null parameter. CFML does not error on unknown named arguments — it accepts them into the arguments struct and the called function simply never reads them. So the column ends up created with whatever the underlying default nullability is, not what the example reads as if it were specifying.
Step-by-step proof
- User copies example New master #2 from
announce.txt:addColumn(table="users", columnType="string", columnName="apiKey", limit=64, null=true); - CFML invokes
addColumnwith arguments struct:{table:"users", columnType:"string", columnName:"apiKey", limit:64, null:true}. addColumn(Migration.cfc:164) setsarguments.addColumns = trueand forwards tochangeColumn(argumentCollection=arguments).changeColumnreads named parameters by their declared names —allowNull,default,limit, etc. Thenullkey in the struct is never read.- The column is created with the database default for nullability (typically
NULLallowed). The user reads "null=true" and assumes they explicitly opted in to NULLs, but in reality the argument was silently dropped — there was never any control flow that examinedarguments.null.
Why other examples don't hit this
Every other example added in this PR — addcolumn.txt, biginteger.txt, boolean.txt, changecolumn.txt, createtable.txt, string.txt, text.txt, etc. — uses allowNull=…. The announce.txt examples are the only inconsistency.
Impact
Reference docs are copy-paste templates. A null=false in an example reads as "this column will be NOT NULL," but the produced DDL will have the database default. For a migration that is supposed to enforce a NOT NULL constraint on articles.title (example #1), this is a real correctness drift — silently passing tests in dev (where data happens to be present) but failing the moment a row is inserted without the column.
Fix
In vendor/wheels/public/docs/reference/migration/announce.txt:
- Line 6: change
null=falsetoallowNull=false - Line 28: change
null=truetoallowNull=true
| // 1. Run all pending migrations to bring the database up to the latest version | ||
| result = application.wheels.migrator.migrateToLatest(); | ||
| // result -> "Migrating from 20240101000000 up to 20240315120000. | ||
| // -------- 20240315120000_add_status_to_orders -------------------- | ||
| // " | ||
|
|
||
| // 2. Already at the latest version — no migration required | ||
| result = application.wheels.migrator.migrateToLatest(); | ||
| // result -> "Database is currently at version 20240315120000. No migration required." | ||
|
|
||
| // 3. Check for errors after migrating to latest | ||
| result = application.wheels.migrator.migrateToLatest(); | ||
| if (FindNoCase("Error", result)) { | ||
| writeOutput("Migration failed: " & result); | ||
| } else { | ||
| writeOutput("All migrations applied successfully."); | ||
| } |
There was a problem hiding this comment.
🔴 This PR adds vendor/wheels/public/docs/reference/migrator/migratetolargest.txt documenting a function (migrateToLargest) that does not exist anywhere in the codebase. The file is byte-for-byte identical to migratetolatest.txt (every example calls application.wheels.migrator.migrateToLatest()) and state.json only tracks function:migrateToLatest, so this is an accidental duplicate that should simply be deleted.
Extended reasoning...
What is wrong
The new file vendor/wheels/public/docs/reference/migrator/migratetolargest.txt documents a function that does not exist. Migrator.cfc defines migrateTo, migrateIndividual, migrateToLatest, getCurrentMigrationVersion, createMigration, getAvailableMigrations, redoMigration, and renameSystemTables — but no migrateToLargest. A case-insensitive grep across the entire repository for migrateToLargest matches only this file path itself.
How it happened
The file appears to be an accidental duplicate of the (also new) migratetolatest.txt. A diff between the two files returns nothing — they are byte-for-byte identical, both invoking application.wheels.migrator.migrateToLatest() in every example. The state.json diff in this PR only adds a function:migrateToLatest entry — there is no function:migrateToLargest entry, confirming the agent never tracked migrateToLargest as a real function and this file was created in error (most likely the agent wrote the same content under both filenames, or filename autocompletion produced the typo).
Step-by-step proof
- Read
vendor/wheels/Migrator.cfc— the public functions defined are:migrateTo(line 27),migrateIndividual(142),migrateToLatest(189),getCurrentMigrationVersion(205),createMigration(215),getAvailableMigrations(235),redoMigration(285),renameSystemTables(625). NomigrateToLargest. - Run a case-insensitive grep for
migratetolargestacross the entire codebase. The only hit is the new file itself (the path), with zero references in any.cfc, doc, snapshot, or state file. - Open
migratetolargest.txtandmigratetolatest.txtside by side.diffreturns 0; the contents (including the comment text and the call toapplication.wheels.migrator.migrateToLatest()) are identical. - Inspect
tools/docs-validation/state.jsondiff — onlyfunction:migrateToLatestis added; there is nofunction:migrateToLargestentry, so this file is not part of the agent's validation plan. - Confirm via the v4.0.0 API snapshot at
docs/api/v4.0.0.json: it containsmigrateToLatestbut nomigrateToLargest.
Impact
This ships public reference documentation for a phantom API. If a user searched the docs tree for migratetolargest they would land on a file whose every example actually calls a different function (migrateToLatest), which is confusing. It is docs-only and does not break runtime behavior.
Fix
Delete vendor/wheels/public/docs/reference/migrator/migratetolargest.txt. No other changes are required — migratetolatest.txt already correctly documents the real function.
|
|
||
| // 3. Typical down() migration reversing an addIndex call | ||
| component extends="wheels.Migrator" { |
There was a problem hiding this comment.
🔴 Three migration reference examples added in this PR have invalid extends= values that produce non-compilable migration components if copied: removeindex.txt:8 and renametable.txt:5 both use extends="wheels.Migrator" (the migration runner, which itself extends wheels.Global), and down.txt:49 uses an unsubstituted template placeholder extends="[extends]". The correct base class for individual migrations is wheels.migrator.Migration — every other example added in this same PR (addrecord, droptable, dropview, removerecord, renamecolumn, updaterecord, etc.) uses that value, so the inconsistency is clearly a copy/paste mistake; fix is a one-line change in each of the three files.
Extended reasoning...
What's wrong
Three of the new migration reference examples added in this PR declare a base class that is either wrong or a literal template token, so a user copy/pasting any of them ends up with a CFC that does not compile:
| File | Line | Current | Correct |
|---|---|---|---|
vendor/wheels/public/docs/reference/migration/removeindex.txt |
8 | component extends="wheels.Migrator" { |
component extends="wheels.migrator.Migration" { |
vendor/wheels/public/docs/reference/migration/renametable.txt |
5 | component extends="wheels.Migrator" { |
component extends="wheels.migrator.Migration" { |
vendor/wheels/public/docs/reference/migration/down.txt |
49 | component extends="[extends]" hint="Add status column to orders" { |
component extends="wheels.migrator.Migration" hint="Add status column to orders" { |
Why the current values are wrong
wheels.Migratoris the migration runner. Looking atvendor/wheels/Migrator.cfc:1you can see it itself extendswheels.Global:component output="false" extends="wheels.Global". It is not the base class for an individual migration CFC — extending it gives you a component that does not have theup()/down()dispatch helpers that live on the per-migration base.[extends]is not even a recognized placeholder. Looking atvendor/wheels/Migrator.cfc:387, the only placeholder Migrator substitutes when generating a migration file is|DBMigrateExtends|:Replace(local.templateContent, "|DBMigrateExtends|", local.extendsPath). The actuallocal.extendsPathis hard-coded just above atMigrator.cfc:374as"wheels.migrator.Migration". So[extends]is neither a valid CFC dotted path nor an active template token — it is a leftover/typo.
Step-by-step proof a user copy/paste fails
- User opens the new docs page for
down()and copies example New master #3 verbatim intoapp/migrator/20260507000000_add_status_to_orders.cfc. - They save the file and run
wheels reload(or hit any URL that touchesapplication.wheels.migrator). - Lucee/ACF tries to resolve the parent component
[extends]. Square brackets are not a legal CFML dotted path, so the engine throwsCould not find the ColdFusion Component or Interface [extends].— the migration cannot be loaded, let alone executed. - The exact same failure mode (with a different message) happens for
wheels.Migrator: the component resolves — but instantiating it triggerswheels.Global's init chain (which expects to be the framework root), and even if that succeeds, the component is missing the per-migration helpers (up/downdispatch,announce, etc.) that come fromwheels.migrator.Migration. The migrator harness will not recognize it as a valid migration andmigrateToLatest()will skip or fail on it.
Why we know the intended value
Every other component example added in this same PR uses the correct value wheels.migrator.Migration:
addrecord.txt:13,droptable.txt:2,dropview.txt:2,removerecord.txt:11,renamecolumn.txt:5,updaterecord.txt:15,createview.txt:11,execute.txt:7— all useextends="wheels.migrator.Migration".
So the three flagged occurrences are inconsistent within the very same PR, not a stylistic choice.
Suggested fix
A one-line change in each of the three files:
--- a/vendor/wheels/public/docs/reference/migration/removeindex.txt
+++ b/vendor/wheels/public/docs/reference/migration/removeindex.txt
@@ -8 +8 @@
-component extends="wheels.Migrator" {
+component extends="wheels.migrator.Migration" {
--- a/vendor/wheels/public/docs/reference/migration/renametable.txt
+++ b/vendor/wheels/public/docs/reference/migration/renametable.txt
@@ -5 +5 @@
-component extends="wheels.Migrator" {
+component extends="wheels.migrator.Migration" {
--- a/vendor/wheels/public/docs/reference/migration/down.txt
+++ b/vendor/wheels/public/docs/reference/migration/down.txt
@@ -49 +49 @@
-component extends="[extends]" hint="Add status column to orders" {
+component extends="wheels.migrator.Migration" hint="Add status column to orders" {Impact
Low runtime impact (these are docs, not framework code), but real correctness bug for users — the examples are advertised as copy/paste-ready and shipped in this PR, so anyone learning Wheels migrations from these three pages today will get a broken component. Severity is "normal" rather than "nit" because the examples are net-new in this PR and the goal of the PR is explicitly to provide working reference snippets.
Summary
Agent-driven docs validation for the Migrator section.
docs/api/v4.0.0.jsonvendor/wheels/**/*.cfc(docblocks + narrow body fixes) andvendor/wheels/public/docs/reference/{scope}/{name}.txttools/docs-validation/state.jsonReview checklist
state.jsonentry withstatus: donehas a sensible reference example file added or updatedstate.jsonitems withstatus: needs_humandescribe the open question innotes