Bug: trim() takes a character set, not a prefix, so with db_ set the rebuilt table's foreign keys point at tables that don't exist.

[karlgray]

PHP Version

8.4

CodeIgniter4 Version

4.7.5-dev

CodeIgniter4 Installation Method

Git

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cli-server (PHP built-in webserver)

Environment

development

Database

SQLite 3.45.1

What happened?

I have a fix and tests done, will be doing a PR shortly.

SQLite cannot alter or drop a column in place. CodeIgniter works around that by rebuilding
the whole table — it renames the original, creates a replacement from the metadata it
collected, copies the rows across, and drops the original. SQLite3\Table does this work,
and Forge::modifyColumn(), Forge::dropColumn(), Forge::dropForeignKey() and
Forge::dropPrimaryKey() all go through it.

If a table prefix is configured, the replacement table's foreign keys come out wrong. Each
one names a table that does not exist, because characters have been removed from both ends
of the name.

The mistake is in SQLite3\Table::createTable(), which strips the prefix off each foreign
key's target using trim():

// system/Database/SQLite3/Table.php
foreach ($this->foreignKeys as $foreignKey) {
    $this->forge->addForeignKey(
        $foreignKey->column_name,
        trim($foreignKey->foreign_table_name, $this->db->DBPrefix),
        $foreignKey->foreign_column_name,
    );
}

trim() does not remove a prefix. Its second argument is a set of characters, and it
removes any of those characters from either end of the string, repeatedly, until it reaches
one that is not in the set.

So with the prefix db_, the set is d, b and _. Given db_boards, trim() removes
the d, then the b, then the _ — and then keeps going, because the next character is
the b of boards, which is also in the set:

trim('db_boards', 'db_');   // 'oards'   — expected 'boards'
trim('db_sub',    'db_');   // 'su'      — the trailing 'b' is removed too
trim('db_bd',     'db_');   // ''        — nothing survives
trim('wp_posts',  'wp_');   // 'osts'
trim('wp_swap',   'wp_');   // 'swa'

Two things follow from this that are worth stating separately. Characters are removed from
the end of the name as well as the start, which is not what a prefix strip should ever
do. And whether a given table is affected depends entirely on which letters its name happens
to begin and end with, so the same code path works on most tables and silently breaks others.

CodeIgniter's own test configuration sets DBPrefix => 'db_' (app/Config/Database.php,
the tests group, commented "Needed to ensure we're working correctly with prefixes live").
Any project that sets a prefix is exposed.

The test suite does not cover this, despite that prefix. The tests for this exact code
path live in tests/system/Database/Live/SQLite3/AlterTableTest.php, and that class does not
use the tests group. Its setUp() builds a connection of its own:

$config = [
    'DBDriver' => 'SQLite3',
    'database' => ':memory:',
    'DBDebug'  => true,
];
$db = db_connect($config);

No DBPrefix, so the prefix is empty for every test in the class. With an empty prefix,
trim($name, '') does nothing at all and the bug cannot appear. That is why the suite has
been green over this line the whole time.

The correct code already exists in the same class. fromTable(), 240 lines earlier,
strips the prefix properly:

$prefix = $this->db->DBPrefix;

if (! empty($prefix) && str_starts_with($table, $prefix)) {
    $table = substr($table, strlen($prefix));
}

The foreign-key loop is the only place that uses trim() for this.

Steps to Reproduce

With DBPrefix set to db_ on a SQLite3 connection:

// A table with a foreign key to another whose name begins with a character
// that also appears in the prefix.
$this->forge->addField([
    'id'       => ['type' => 'INTEGER', 'auto_increment' => true],
    'board_id' => ['type' => 'INTEGER'],
    'colour'   => ['type' => 'VARCHAR', 'constraint' => 7, 'null' => true],
]);
$this->forge->addPrimaryKey('id');
$this->forge->addForeignKey('board_id', 'boards', 'id', '', 'CASCADE');
$this->forge->createTable('lists');

// Anything that rebuilds the table will trigger it. Widening a column is enough.
$this->forge->modifyColumn('lists', [
    'colour' => ['name' => 'colour', 'type' => 'VARCHAR', 'constraint' => 16, 'null' => true],
]);

// Now write to the table the foreign key pointed at.
$db->table('boards')->insert(['name' => 'Acme']);

The insert fails with:

no such table: main.db_oards

PRAGMA foreign_key_list(db_lists) shows the damage directly: its table column reads
db_oards.

Expected Output

Expected behaviour

Rebuilding a table leaves its foreign keys pointing where they pointed before, whatever
prefix is configured.

Actual behaviour

The rebuilt table's foreign keys name tables that do not exist, with characters missing from
both ends.

Anything else?

Why this is easy to miss

The migration reports success. run() performs the rebuild inside a transaction, and
transComplete() returns true — as far as SQLite is concerned, nothing went wrong. It does
not check that a foreign key's target exists when the constraint is defined, and run()
only turns PRAGMA foreign_keys back on once the rebuild has finished. So there is nothing
to fail at the point where the damage is done.

The error appears somewhere else. It surfaces at the next write to an affected table,
and the message names a table that appears nowhere in the codebase. Nothing about it points
back to the migration responsible, which is what makes this expensive to diagnose rather
than merely wrong.

Where it does not throw, it is silent. A cascade that should have fired simply does not,
because the constraint refers to a table that is not there.

It is intermittent across a schema. As above, whether a table is affected depends on its
name. A project can rebuild several tables without noticing anything, then hit this on the
first one beginning with d, b or _.

Workaround

Avoid letting SQLite3\Table rebuild any table that has a foreign key. In practice that
means keeping modifyColumn(), dropColumn(), dropForeignKey() and dropPrimaryKey()
away from those tables when the driver is SQLite.

Often the change only matters on a driver that alters columns in place, in which case it can
be made driver-specific. Widening a VARCHAR is the common case: SQLite is dynamically
typed and ignores the declared length, so skipping it there loses nothing.

private function widen(string $table, int $length): void
{
    if ($this->db->DBDriver !== 'MySQLi') {
        return;
    }

    $this->db->query(
        'ALTER TABLE ' . $this->db->protectIdentifiers($this->db->DBPrefix . $table, false, null, false)
        . ' MODIFY COLUMN `colour` VARCHAR(' . $length . ') NULL',
    );
}

If the change genuinely has to happen on SQLite, the remaining option is to drop and recreate
the table by hand, so that you write the foreign keys yourself.