Problem/Motivation

As documented the schema can include: fields, indexes, unique keys and foreign keys. Except for unique keys these are all processed. It seems this got lost somewhere in #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields, the code to remove the keys on deletion is still there.

Proposed resolution

Add (back) unique key support.

Remaining tasks

Test (that comes first and will be attached - and failing to start with).
Add the code required as for 'indexes' and 'foreign keys'.

API changes

None - it will work as documented.

CommentFileSizeAuthor
#11 2742103-11.interdiff.txt821 bytesekes
#11 2742103-11.getDedicatedTableSchema-unique_keys.patch3.7 KBekes
#8 2742103-08.interdiff.txt2.25 KBekes
#8 2742103-08.getDedicatedTableSchema-unique_keys.patch3.7 KBekes
#4 2742103-04.getDedicatedTableSchema-unique_keys.patch2.54 KBekes
#3 2742103-02.getDedicatedTableSchema-unique_keys.patch1.18 KBekes
#2 2742103-01.getDedicatedTableSchema-unique_keys.patch556 bytesekes

Comments

ekes created an issue. See original summary.

ekes’s picture

ekes commented
StatusFileSize
new 2742103-01.getDedicatedTableSchema-unique_keys.patch556 bytes

Think this is the correct test. Adds unique keys along with the indexes.

ekes’s picture

ekes commented
StatusFileSize
new 2742103-02.getDedicatedTableSchema-unique_keys.patch1.18 KB

Correct test.

It also didn't test additional index in the schema, so adding that as well.

ekes’s picture

ekes commented
Status: Active » Needs review
StatusFileSize
new 2742103-04.getDedicatedTableSchema-unique_keys.patch2.54 KB

With fix.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +DevDaysMilan 
  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1837,6 +1837,24 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
+      $real_name = $this->getFieldIndexName($storage_definition, $index_name);
...
       $real_name = $this->getFieldIndexName($storage_definition, $specifier);

    Just a note for myself: this method is used for the foreign keys as well 

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1837,6 +1837,24 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
+        else {
+          $data_schema['unique keys'][$real_name][] = $table_mapping->getFieldColumnName($storage_definition, $column_name);
+        }

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -829,8 +829,12 @@ public function testDedicatedTableSchema() {
+      'unique keys' => array(
+        'shape' => array('shape'),
+      ),

    It seems to be that adding a testcase for this case would be helpful as well.

ekes’s picture

ekes commented

It's adding index and unique key tests, and foreign key is already tested. What should be added further?

dawehner’s picture

dawehner
Primary language German
commented

Well a unique key without an array for the column names.

ekes’s picture

ekes commented
StatusFileSize
new 2742103-08.getDedicatedTableSchema-unique_keys.patch3.7 KB
new 2742103-08.interdiff.txt2.25 KB

Ah. So it was testing that line: $data_schema['unique keys'][$real_name][] = $table_mapping->getFieldColumnName($storage_definition, $column_name);. What wasn't tested there was the array case in the other half of the if/else where it is a column name and a size. So for completeness added that for index and unique.

dawehner’s picture

dawehner
Primary language German
commented

Cool, thank you!

amateescu’s picture

amateescu commented

The patch looks great to me, I just found a small nit:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1837,6 +1837,24 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
+        // Indexes can be specified as either a column name or an array with

Should we replace 'Indexes' with 'Unique keys' at the beginning of the sentence?

ekes’s picture

ekes commented
StatusFileSize
new 2742103-11.getDedicatedTableSchema-unique_keys.patch3.7 KB
new 2742103-11.interdiff.txt821 bytes

Ah yes. Corrected.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Nice, looks great now :)

  • catch committed 60a1752 on 8.2.x 
    Issue #2742103 by ekes, dawehner, amateescu:...

  • catch committed 2660334 on 8.1.x 
    Issue #2742103 by ekes, dawehner, amateescu:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.