Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new updateColumn() method for safer column modifications that preserves unspecified attributes by default, while maintaining backward compatibility with the existing changeColumn() method. The changeColumn() method is updated to support optional attribute preservation via a preserve_unspecified flag and now accepts null as the column type to preserve the existing type.
- Adds
updateColumn()as the recommended method for column modifications with automatic attribute preservation - Enhances
changeColumn()to supportnulltype parameter and optional attribute preservation - Implements comprehensive test coverage for both methods and their preservation behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Db/Table.php | Adds updateColumn() method, updates changeColumn() to support null type and preservation logic, adds mergeColumnOptions() helper |
| tests/TestCase/Db/Adapter/MysqlAdapterTest.php | Adds comprehensive test cases for column update/change preservation behavior |
| docs/en/writing-migrations.rst | Documents both updateColumn() (recommended) and changeColumn() methods with examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
as long as schema reflection represents the column properly I don't see a problem supporting this kind of functionality. |
ccef79e to
083230c
Compare
…umn + options behavior - Remove non-functional enum/set values preservation code Schema reflection doesn't populate Column::values, so preserving it has no effect - Add test documenting that options array is ignored when Column object is passed This clarifies the existing behavior where Column objects take full precedence Addresses: #941 (comment) Addresses: #941 (comment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
3196ff9 to
cad5475
Compare
- Tests that updateColumn() works correctly when passed a Column object - Addresses review feedback to ensure Column type support is tested 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
cad5475 to
7509eff
Compare
src/Db/Table.php
Outdated
| if (!$typeChanging && !isset($options['limit']) && !isset($options['length'])) { | ||
| $limit = $existingColumn->getLimit(); | ||
| if ($limit !== null) { | ||
| $existingOptions['limit'] = $limit; | ||
| } | ||
| } |
There was a problem hiding this comment.
How does one remove length with this API?
There was a problem hiding this comment.
To remove a length constraint using this API, you would typically need to explicitly set 'length' => null or 'limit' => null in the options array when calling updateColumn(). For example:
$table->updateColumn('my_column', 'text', ['length' => null]);
I added a test case that shows it works, but from the code above I also see that using isset() instead of array_key_exists() would not set it from the options.
Implement cakephp/phinx#123 in a BC way.
An alternative way, if we want more speaking API, could be
updateColumnAttributes(field, array options)if we are only interested in changing attributes