New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added native UUID field type for MariaDB1070+ #6316
base: 4.1.x
Are you sure you want to change the base?
Conversation
|
||
use function sprintf; | ||
|
||
class AlterUuidColumnTest extends FunctionalTestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to limit this test to MariaDB 10.7 and higher because we're specifically testing the migration from old table definitions on that platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, other platforms different than MariaDB 10.7 are skipped.
$table = new Table($tableName); | ||
$table->addColumn($uuidField, Types::STRING, [ | ||
'length' => 36, | ||
'notnull' => false, | ||
'fixed' => true, | ||
]); | ||
|
||
return $table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use the schema manager to create this table. Use plain SQL to create the table as DBAL 4.0 would've created it if we configured a GUID column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Now the table is created using plain SQL and then the table is introspected.
), | ||
); | ||
|
||
$childTable->removeForeignKey('fk_parent_uuid_id'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily removing the foreign key is kind of what I would expect the comparator to do, tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove the foreign key the test will fail. The error received is:
"General error: 1833 Cannot change column 'id': used in a foreign key constraint 'fk_parent_uuid_id' of table 'doctrine_tests.child_uuid_table'"
Is it intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This is precisely what needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the test so that it must fail. I will try to allocate some time during next week in order to see how can I fix this issue. Any insight is welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some research I figure out that in order to automatically to drop and create the foreign keys I will need that introspectTable and hereby the Table class will contains information about which tables and and foreign keys are affected. I also tried another approach that seems to work when I run in the MySQL console and it's using compound statements (MariaDB support it), however the "executeStatement" doesn't support this type of statements.
Do you have any insight that can help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid compound statements and I don't think we need them.
I added the native UUID field type for MariaDB1070+ into the branch 4.1.x.
I also provided the following tests:
The tests also check for data integrity after the column modification.