Add foreign key to unit fields to cleanup if unit is deleted #16836
Conversation
Signed-off-by: Matthias Hamacher <[email protected]>
Review Checklist
|
Signed-off-by: Matthias Hamacher <[email protected]>
Signed-off-by: Matthias Hamacher <[email protected]>
|
There was a problem hiding this comment.
Hello @matthiashamacher ,
thanks for the pr, I like the idea. However, I think we could do some improvements here. What do you think about my comments?
|
|
||
| if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue | ||
| || $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange) | ||
| && $fkey === 'unit' | ||
| ) { | ||
| try { | ||
| $this->db->executeQuery( | ||
| sprintf( | ||
| 'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`) | ||
| REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL', | ||
| $objectDatastoreTable, | ||
| self::getForeignKeyName($objectDatastoreTable, $key . '__' . $fkey), | ||
| $key . '__' . $fkey | ||
| ) | ||
| ); | ||
| } catch (DriverException $e) { | ||
| // Ignore if the foreign key already exists | ||
| } | ||
| } |
There was a problem hiding this comment.
Any chance we could somehow extract that into a method? It seems to be same code over and over again.
WE introduced a indexExists method here, instead of just executing the statement and ignoring the error. Maybe we could do the same for foreign keys?
| if (str_contains($value, 'unit') === true) { | ||
| $this->db->executeQuery( | ||
| sprintf( | ||
| 'ALTER TABLE `%s` DROP FOREIGN KEY %s', | ||
| $table, | ||
| self::getForeignKeyName($table, $value) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
If we introduce a foreignKeyExists method, we could also use it here, right?
And I am still not sure about the str_contains check. Could there be some side effects, if there are columns that just happen to have unit in their name?
Changes in this pull request
Resolves #3376