Render table level options when baking tables by jamisonbryant · Pull Request #1105 · cakephp/migrations

jamisonbryant · 2026-07-13T20:46:33Z

Summary

templates/bake/element/create-tables.twig emitted only the primary key portion of the table options array, so the collation and engine reflected onto the TableSchema were dropped. A table created with a non-default collation or engine was regenerated using the connection defaults instead of its real ones.

This renders those options from the reflected schema. An option is only printed when leaving it out would change the table: if a table's collation or engine already matches what the migration would produce without it, printing it is redundant, so it is omitted.

Major Changes

MigrationHelper::tableOptions() — returns the reflected table options, minus any that MysqlAdapter::createTable() would apply on its own. It resolves the same two defaults the adapter does: engine falls back to MysqlAdapter::DEFAULT_ENGINE, while collation falls back to Migrations.default_collation when that is configured, and otherwise to the database's own default collation (resolved once and cached). Comparing against the wrong default would omit an option the migration then silently changes.
MigrationHelper::stringifyTableOptions() — renders the options array inline.
- NOTE:stringifyList() cannot be reused here: it sorts the array, indents across multiple lines, and appends a trailing comma, which would reorder the arguments of every existing $this->table() statement. Table options are an ordered list, so id and primary_key keep leading it and existing output stays byte-identical.
create-tables.twig — merges the table options into the $this->table() options array.

Minor Changes

MysqlAdapter::DEFAULT_ENGINE — the default engine was inlined in createTable(). Promoted to a constant so the bake helper and the adapter share one source of truth for which engine is applied when a table does not specify one.
create-tables.twig is also rendered by diff.twig, so a diff that adds a table now carries its collation and engine as well.

Backwards Compatibility Notes

Fully backwards compatible. The changes are additive: one new public constant on MysqlAdapter and two new public methods on MigrationHelper. No signatures changed.

Baked output is unchanged for any table whose collation and engine match the defaults — every existing comparison fixture passes untouched.

encoding is not rendered as a separate option: MysqlAdapter::createTable() derives CHARACTER SET from the collation prefix, so rendering collation restores the charset as well.

Table options are reflected for MySQL only; the other dialects return [] from describeOptions(), so their output is unaffected.

The default table engine was inlined in createTable(). Promote it to a constant so there is a single source of truth for callers that need to know which engine the adapter applies when a table does not specify one.

create-tables.twig only emitted the primary key portion of the table options array, so the collation and engine reflected onto the TableSchema were dropped. Tables created with a non-default collation or engine were regenerated using the connection defaults instead of their real ones. Render them from the reflected schema, limited to the options that would not be applied anyway when the migration runs, so tables sitting on the database defaults stay portable across servers. The MySQL adapter derives the character set from the collation, so no separate encoding option is needed. Only MySQL reflects table options; other drivers report none.

Bake a snapshot of a table carrying a non-default collation and a table carrying a non-default engine, and assert both are rendered. Register the comparison file with the snapshot migration provider so the generated migration is also proven to run.

Copilot

Pull request overview

This PR fixes MySQL snapshot baking to preserve table-level options (notably collation and engine) so regenerated schemas don’t silently revert to connection defaults.

Changes:

Add MigrationHelper::tableOptions() and MigrationHelper::stringifyTableOptions() to compute and render table options while omitting defaults.
Update create-tables.twig to merge reflected table options into generated $this->table() calls (preserving option insertion order).
Promote MySQL default engine to MysqlAdapter::DEFAULT_ENGINE and add/extend snapshot comparison + command tests for non-default table options.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/TestCase/View/Helper/MigrationHelperTest.php	Adds unit tests for table option omission/inclusion and stable inline rendering/escaping.
tests/TestCase/MigrationsTest.php	Registers a new snapshot comparison fixture in the provider.
tests/TestCase/Command/BakeMigrationSnapshotCommandTest.php	Adds an integration test ensuring non-default table collation/engine are baked into snapshots.
tests/comparisons/Migration/test_snapshot_with_non_default_table_options.php	New expected snapshot output fixture including table-level `collation` and `engine`.
templates/bake/element/create-tables.twig	Emits table options array (including reflected options) when needed in `$this->table()` calls.
src/View/Helper/MigrationHelper.php	Implements table option extraction/default-resolution and inline ordered rendering.
src/Db/Adapter/MysqlAdapter.php	Introduces `DEFAULT_ENGINE` constant for shared default-engine semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            if (is_array($value)) {
+                $value = sprintf('[%s]', implode(', ', array_map(
+                    fn(mixed $item): string => (string)$this->value($item, true),
+                    $value,
+                )));
+            } else {
+                $value = (string)$this->value($value, true);
+            }


+        $default = $connection
+            ->execute('SELECT @@character_set_database AS charset, @@collation_database AS collation')
+            ->fetch('assoc');
+


markstory · 2026-07-14T04:02:29Z

        // This method is based on the MySQL docs here: https://dev.mysql.com/doc/refman/5.1/en/create-index.html
        $defaultOptions = [
-            'engine' => 'InnoDB',
+            'engine' => self::DEFAULT_ENGINE,
        ];



Could the other fallback/default value be updated as well?

dereuromark

Thanks, this is well built. Comparing the reflected options against what MysqlAdapter::createTable() would actually apply (the Migrations.default_collation config, then the database default, then the engine constant) is the right baseline, and I verified locally that all existing comparison fixtures stay byte-identical on MySQL. Two findings from digging into the failures and the test setup, see inline comments.

dereuromark · 2026-07-13T21:16:01Z

+     *  or a table schema object.
+     * @return array<string, mixed>
+     */
+    public function tableOptions(TableSchemaInterface|string $table): array


The MariaDB job failures are worth understanding before touching the fixtures, the root cause is structural. The three failing diff tests render $this->table('articles', ['collation' => 'utf8mb4_0900_ai_ci']) in down(). That schema does not come from the live database: dropped-table recreation in the diff renders the schema unserialized from the committed schema-dump-*_mysql.lock fixture, which was serialized on MySQL 8 with utf8mb4_0900_ai_ci baked into the options. MariaDB 11.8/12 preserves the 0900 alias verbatim on tables (verified on MariaDB 12.3), but its database default is utf8mb4_uca1400_ai_ci, so this method sees a non-default collation and renders it.

Since the mysql and mariadb CI jobs share the same _mysql lock fixture, no regenerated fixture can satisfy both. I would suggest not rendering table options for schemas sourced from the dump lock in the diff down() path (those options describe the server the dump was taken on, not the target), or alternatively normalizing the known collation aliases before comparing. The snapshot path is unaffected and works correctly cross-database.

Ah, I see the issue. Because the tests can't control where the dump was run, they also can't rely on it as a source-of-truth for the pre-migration state. It's a good thing the MariaDB tests caught this. The lock records the resolved collation, but not the default it was resolved against, so "does this differ from the default?" isn't really a question a dump can answer.

I think then the proper source-of-truth is the table's options on the connection itself, the one being baked against. Which means that tableOptions() shouldn't trust the object it's handed, it should reflect the table itself:

$name = $table instanceof TableSchemaInterface ? $table->name() : $table; if (!in_array($name, $this->tableNames(), true)) { return []; } $options = $this->schema($name)->getOptions();

A dropped table is absent from the connection being baked against, so the down() recreation would render no options on its own, without a flag having to be passed down from the caller. The snapshot path and the tables['add'] side of the diff are both live-reflected, so they'd keep theirs.

The part I'm unsure about: this makes tableOptions() ignore the options of the TableSchemaInterface it's handed, which feels like a violation of least surprise for a method with that signature. Maybe the argument is really only identifying which table rather than carrying its options, and the signature should just be string $table? I don't have a strong sense of which way to go.

What do you think?

Yeah, live-reflecting is the right move. I traced the wiring to be sure: the collection the helper gets is the live connection's, and the dropped tables in down() come from dumpSchema (the lock), so they're simply not in the live collection. Your guard returns [] for exactly those and leaves the add side and snapshots alone. No bake mode leaks it.

And your reasoning is the real point: the lock stores the resolved collation but not the default it was resolved against, so "does this differ from the default?" can't be answered from a dump. Omitting is the honest answer, and it beats chasing collation aliases across servers.

On the signature, I'd just make it string $table. Every other TableSchemaInterface|string method here uses the object's data; this one would be the odd one out that takes it and throws it away, which is the surprise you're feeling. It only needs the name now, and call sites can pass $table->name().

Two nits:

$this->tableNames() isn't a thing here, use $this->getConfig('collection')->listTables().

Worth calling out in the PR: a truly dropped table recreated in down() now comes back with the target server's defaults instead of its old options. Unavoidable, and better than baking a source-server collation into a cross-server down(), but nicer as a stated decision.

If we go this route I'd add a docblock note (reflects the live table, no options for an absent one) and a regression test for the drop/recreate path that caught this.

dereuromark · 2026-07-13T21:16:01Z

+        } finally {
+            $connection->execute('ALTER TABLE parts ENGINE=InnoDB');
+            $connection->execute(sprintf(
+                'ALTER TABLE events CONVERT TO CHARACTER SET %s COLLATE %s',


This restore silently corrupts the shared schema even though CI stays green: CONVERT TO CHARACTER SET back to utf8mb4 grows the byte length per character, which promotes events.description from text to mediumtext. Verified locally on MySQL 8.4, after this test runs the column is permanently mediumtext. The suite only passes because this test runs last in the class and nothing re-bakes events afterwards, so it is an ordering landmine for future tests.

Suggest restoring with per-column MODIFY like testSnapshotWithNonDefaultCollation does above, or dropping and recreating the table.

jamisonbryant · 2026-07-13T21:22:13Z

+    /**
+     * The storage engine applied to tables that do not specify one.
+     */
+    public const DEFAULT_ENGINE = 'InnoDB';


Personally not a fan of making this a const when default_collation is already a config setting, feels incongruous. However, my AI agent recommended doing it this way because breaking out to config would be a much larger change that was out-of-scope of this bugfix.

I guess it's fine, easier also to use for other usage, e.g. within tests.

markstory

Nice work on the tests 👏

markstory · 2026-07-14T04:02:29Z

        // This method is based on the MySQL docs here: https://dev.mysql.com/doc/refman/5.1/en/create-index.html
        $defaultOptions = [
-            'engine' => 'InnoDB',
+            'engine' => self::DEFAULT_ENGINE,
        ];



Could the other fallback/default value be updated as well?

markstory · 2026-07-14T04:04:45Z

+        }
+        $this->defaultCollationResolved = true;
+
+        $configured = Configure::read('Migrations.default_collation');


We inherited the _ in other config names from phinx. I don't know if we should continue that trend, or break away and use the more common convention of camelBacked names.

Migrations has been part of Cake for a long time now, I am personally in favor of adopting defaultCollation and defaultEngine as fully-managed config settings, however that should probably happen in a follow-up branch unless we want to rebase this off of 5.next.

jamisonbryant added 3 commits July 13, 2026 15:22

Add MysqlAdapter::DEFAULT_ENGINE constant

ab26ad4

The default table engine was inlined in createTable(). Promote it to a constant so there is a single source of truth for callers that need to know which engine the adapter applies when a table does not specify one.

jamisonbryant self-assigned this Jul 13, 2026

jamisonbryant added bug bake labels Jul 13, 2026

jamisonbryant requested a review from Copilot July 13, 2026 20:48

Copilot started reviewing on behalf of jamisonbryant July 13, 2026 20:49 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

dereuromark reviewed Jul 13, 2026

View reviewed changes

jamisonbryant commented Jul 13, 2026

View reviewed changes

markstory reviewed Jul 14, 2026

View reviewed changes

Uh oh!

Conversation

jamisonbryant commented Jul 13, 2026

Summary

Major Changes

Minor Changes

Backwards Compatibility Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dereuromark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants