Keep ON UPDATE clause when baking migrations#1103
Merged
Merged
Conversation
CakePHP reflects `ON UPDATE CURRENT_TIMESTAMP` on datetime/timestamp columns as an `onUpdate` column attribute, but MigrationHelper dropped it in two places: the valid-options list in attributes() and the wanted- options list in getColumnOption(). The clause never reached the template, so any column declaring it lost it when snapshotted. Databases rebuilt from a snapshot then silently stopped auto-updating those columns on writes that bypass the ORM, while existing databases kept the real schema. Let the attribute through both filters and translate it to Phinx's `update` option, mirroring the existing collate/collation handling. Both filters drop the attribute when empty, since Column::toArray() always emits an onUpdate key and Column::setUpdate() is not nullable.
The two existing tests fed getColumnOption() a hand-built array, which bypasses attributes() entirely. Removing 'onUpdate' from the valid-options list in attributes() therefore broke snapshot output with zero test failures, even though it is one of the two changes the fix depends on. Add a test that drives attributes() directly and one that walks the whole snapshot path (columns() -> getColumnOption()). Both build the schema by hand rather than reflecting it, so they run on every driver.
There was a problem hiding this comment.
Pull request overview
This PR fixes snapshot baking for MySQL ON UPDATE column clauses by allowing CakePHP’s reflected onUpdate column attribute to survive MigrationHelper filtering and then rendering it as Phinx’s supported update option, preventing silent schema drift when rebuilding databases from a snapshot.
Changes:
- Allow
onUpdatethroughMigrationHelper::attributes()and drop it when empty/null to avoid emitting a meaningless key. - Allow
onUpdatethroughMigrationHelper::getColumnOption(), drop it when empty/null, and translateonUpdate→updatefor Phinx output. - Add/extend unit tests to cover both the attribute preservation and the render-time translation/removal behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/TestCase/View/Helper/MigrationHelperTest.php | Adds coverage ensuring onUpdate survives attributes() and is rendered as Phinx update, while null/empty values are filtered out. |
| src/View/Helper/MigrationHelper.php | Updates allowlists and adds onUpdate → update translation with empty/null guarding in both attributes() and getColumnOption(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dereuromark
approved these changes
Jul 13, 2026
markstory
reviewed
Jul 14, 2026
markstory
left a comment
markstory
left a comment
Member
There was a problem hiding this comment.
Looks good, would it be difficult to get a regression test for bake migration_snapshot so that we know the bug is fixed and that we won't regress in the future?
The MigrationHelper tests build their schema by hand, so they prove the helper emits the option but not that a baked snapshot keeps it. Cover the end-to-end path: reflect a real ON UPDATE column, bake a snapshot, and compare the generated migration against a checked-in file. Gated on MySQL because only MysqlSchemaDialect reflects onUpdate; the other drivers never produce the attribute. Mirrors the existing non-default-collation test, which faces the same constraint. The comparison file differs from the plain snapshot by a single 'update' => 'CURRENT_TIMESTAMP' line on users.updated, while users.created stays bare, so the test also pins down that the option is not applied to columns that never declared it.
jamisonbryant
marked this pull request as ready for review
July 14, 2026 15:09
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
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
Fixes #1102.
bake migration_snapshotdroppedON UPDATE CURRENT_TIMESTAMPfromdatetime/timestampcolumns. CakePHP core reflects the clause correctly as anonUpdatecolumn attribute;MigrationHelperfiltered it out before it reached the template, so databases rebuilt from a snapshot silently lost the clause.This lets the attribute through both of the helper's allowlists and translates it to Phinx's
updatecolumn option at render time.Verified end-to-end against real MySQL by baking a snapshot of a table with an
ON UPDATEcolumn, applying the generated migration to a fresh database, and reading back the DDLMajor Changes
MigrationHelper::attributes()— addonUpdateto$validOptionsso the attribute survives the snapshot path, and drop it when empty so columns without the clause do not gain a null key.MigrationHelper::getColumnOption()— addonUpdateto$wantedOptions, drop it when empty, and translate it to Phinx'supdateoption.Both follow the pattern established by
fixed(#1014, added to both allowlists; #1048, filtering the null case) and by the existingcollatetocollationtranslation ingetColumnOption().Minor Changes
Column::toArray()always emits anonUpdatekey (null when the column has no clause) and the diff path feeds those arrays straight intogetColumnOption(), whileColumn::setUpdate()is typedstring. Passing the null through would render'update' => nullinto generated migrations and fail.Table::addTimestamps()also uses'update' => ''to mean "no clause", and core'scolumnSql()guards on!== '', soempty()is the correct test.Backwards Compatibility Notes
Fully backwards compatible. Both changed methods are public, but the change is additive only — a key appears solely for MySQL
datetime/timestampcolumns that genuinely carry anON UPDATEclause. No key is removed or renamed, no existing output shape changes, and userland bake templates are unaffected. Existing migration files are untouched.