Conversation
Signed-off-by: Joeri Colman <colmanjoeri@msn.com>
| * The dependency information is obtained from the target database, | ||
| * making this independent of the source/target database vendor. |
There was a problem hiding this comment.
I guess this works because the schema is already applied to the target DB?
There was a problem hiding this comment.
Correct, this code runs after the createSchema or when running in the clearSchema. Only problem could be if you use the clearSchema option if there never was a migration database setup before, but in that case you should just not add that option.
| * | ||
| * @param Connection $connection Target database connection | ||
| * @param array<string> $tables Tables to sort | ||
| * @param bool $reverse Reverse the sorted tables for dropping tables |
There was a problem hiding this comment.
IMO this parameter name should be more expressive, because it's not clear what the "default" order is supposed to be.
There was a problem hiding this comment.
renamed the parameter to dependenciesFirst.
|
|
||
| foreach ($tables as $table) { | ||
| foreach ($schemaManager->listTableForeignKeys($table) as $foreignKey) { | ||
| $parent = $foreignKey->getForeignTableName(); |
There was a problem hiding this comment.
parent is not really the correct term, just call it foreignTable.
| // Ignore references to tables which aren't being converted. | ||
| if (!isset($tableSet[$parent])) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This should never happen, because then something is seriously wrong.
There was a problem hiding this comment.
Even thought other things might be seriously wrong isn't it best to catch this in this code?
| // Ignore self-references. They don't impose an ordering | ||
| // requirement on the table itself. |
There was a problem hiding this comment.
I'm not sure how we copy tables, but could this mean that rows also need to be copied in the correct order by dependency?
There was a problem hiding this comment.
In the case that a table self-references itself yes. But that seemed out of scope for this fix. In my case I didn't have self-referencing tables and I'm not sure if there are apps using those.
| * | ||
| * Tables without dependencies can be copied immediately. | ||
| */ | ||
| $ready = []; |
There was a problem hiding this comment.
While reading the code I was expecting this variable to already hold the final table order, so please find a different more descriptive name for it.
| * the conversion, but the failure will accurately expose the | ||
| * cyclic dependency rather than being hidden by this sorter. | ||
| */ | ||
| if (count($result) !== count($tables)) { |
There was a problem hiding this comment.
I wonder if this should even throw an exception to abort the entire process.
There was a problem hiding this comment.
This ties in with copying rows in a correct order. For now these tables are just added to the list and if there happens to be no problem in the order of the rows then the convert will just succeed. In the other case an exception will be thrown while doing the copy off data as was now the case for any table with a foreign key and alphabetically incorrect order.
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Joeri Colman <colmanjoeri@msn.com>
Signed-off-by: Joeri Colman <colmanjoeri@msn.com>
Signed-off-by: Joeri Colman <colmanjoeri@msn.com>
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Joeri Colman <colmanjoeri@msn.com>
This branch has not been deployed
2 participants
occ db:convert-typefails with "foreign key constraint" #43290Summary
Sort the table order based on foreign keys before starting the copy of tables. The revers logic is also applied when dropping the tables when using
--clear-schema.TODO
Checklist
3. to review, feature component)stable32)AI (if applicable)
Ai helped writing the
sortTablesByForeignKeysmethod body.