Issue #2392209 by plach: DefaultTableMapping::getFieldColumName is broken for base tables

core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Core\Entity\Sql;
 
+use Drupal\Component\Utility\String;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 
 /**
@@ -130,20 +131,34 @@ public function getFieldNames($table_name) {
    */
   public function getColumnNames($field_name) {
     if (!isset($this->columnMapping[$field_name])) {
-      $column_names = array_keys($this->fieldStorageDefinitions[$field_name]->getColumns());
-      if (count($column_names) == 1) {
-        $this->columnMapping[$field_name] = array(reset($column_names) => $field_name);
-      }
-      else {
-        $this->columnMapping[$field_name] = array();
-        foreach ($column_names as $column_name) {
-          $this->columnMapping[$field_name][$column_name] = $field_name . '__' . $column_name;
-        }
+      $this->columnMapping[$field_name] = array();
+      $storage_definition = $this->fieldStorageDefinitions[$field_name];
+      foreach (array_keys($this->fieldStorageDefinitions[$field_name]->getColumns()) as $property_name) {
+        $this->columnMapping[$field_name][$property_name] = $this->getFieldColumnName($storage_definition, $property_name);
       }
     }
     return $this->columnMapping[$field_name];
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $property_name) {
+    $field_name = $storage_definition->getName();
+
+    if ($this->allowsSharedTableStorage($storage_definition)) {
+      $column_name = count($storage_definition->getColumns()) == 1 ? $field_name :  $field_name . '__' . $property_name;
+    }
+    elseif ($this->requiresDedicatedTableStorage($storage_definition)) {
+      $column_name = !in_array($property_name, $this->getReservedColumns()) ? $field_name . '_' . $property_name : $property_name;
+    }
+    else {
+      throw new SqlContentEntityStorageException(String::format('Column information not available for the "@field_name" field.', array('@field_name' => $field_name)));
+    }
+
+    return $column_name;
+  }
+
   /**
    * Adds field columns for a table to the table mapping.
    *
@@ -319,11 +334,4 @@ protected function generateFieldTableName(FieldStorageDefinitionInterface $stora
     return $table_name;
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $column) {
-    return in_array($column, $this->getReservedColumns()) ? $column : $storage_definition->getName() . '_' . $column;
-  }
-
 }

core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageException.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchemaException.
+ */
+
+namespace Drupal\Core\Entity\Sql;
+
+use Drupal\Core\Entity\EntityStorageException;
+
+/**
+ * Exception thrown when a SQL storage operation fails.
+ */
+class SqlContentEntityStorageException extends EntityStorageException {
+}

core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php

@@ -90,16 +90,16 @@ public function getExtraColumns($table_name);
   public function getReservedColumns();
 
   /**
-   * Generates a column name for a field.
+   * Generates a column name for a field property.
    *
    * @param \Drupal\Core\Field\FieldStorageDefinitionInterface $storage_definition
    *   The field storage definition.
-   * @param string $column
-   *   The name of the column.
+   * @param string $property_name
+   *   The name of the property.
    *
    * @return string
    *   A string containing a generated column name for a field data table that is
    *   unique among all other fields.
    */
-  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $column);
+  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $property_name);
 }

core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Core\Entity\Sql;
 use Drupal\Core\Entity\Sql\DefaultTableMapping;
+use Drupal\Core\Entity\Sql\SqlContentEntityStorageException;
 use Drupal\Tests\UnitTestCase;
 
 /**
@@ -234,6 +235,87 @@ public function testGetExtraColumns() {
     $this->assertSame($expected, $table_mapping->getExtraColumns('foo'));
   }
 
+  /**
+   * Tests DefaultTableMapping::getFieldColumnName() with valid parameters.
+   *
+   * @param bool $base_field
+   *   Flag indicating whether the field should be treated as a base or bundle
+   *   field.
+   * @param string[] $columns
+   *   An array of available field column names.
+   * @param string $column
+   *   The name of the column to be processed.
+   * @param string $expected
+   *   The expected result.
+   *
+   * @covers ::getFieldColumnName
+   *
+   * @dataProvider providerTestGetFieldColumnName
+   */
+  public function testGetFieldColumnName($base_field, $columns, $column, $expected) {
+    $definitions['test'] = $this->setUpDefinition('test', $columns, $base_field);
+    $table_mapping = new DefaultTableMapping($definitions);
+    $result = $table_mapping->getFieldColumnName($definitions['test'], $column);
+    $this->assertEquals($expected, $result);
+  }
+
+  /**
+   * Tests DefaultTableMapping::getFieldColumnName() with invalid parameters.
+   *
+   * @param bool $base_field
+   *   Flag indicating whether the field should be treated as a base or bundle
+   *   field.
+   * @param string[] $columns
+   *   An array of available field column names.
+   * @param string $column
+   *   The name of the column to be processed.
+   *
+   * @expectedException \Drupal\Core\Entity\Sql\SqlContentEntityStorageException
+   * @expectedExceptionMessage Column information not available for the "test" field.
+   *
+   * @covers ::getFieldColumnName
+   *
+   * @dataProvider providerTestGetFieldColumnName
+   */
+  public function testGetFieldColumnNameInvalid($base_field, $columns, $column) {
+    $definitions['test'] = $this->setUpDefinition('test', $columns, $base_field);
+
+    // Mark field storage definition as custom storage.
+    $definitions['test']->expects($this->any())
+      ->method('hasCustomStorage')
+      ->willReturn(TRUE);
+
+    $table_mapping = new DefaultTableMapping($definitions);
+    $table_mapping->getFieldColumnName($definitions['test'], $column);
+  }
+
+  /**
+   * Provides test data for testGetFieldColumnName().
+   *
+   * @return array[]
+   *   An nested array where each inner array has the following values: test
+   *   field name, base field status, list of field columns, name of the column
+   *   to be retrieved, expected result, whether an exception is expected.
+   */
+  public function providerTestGetFieldColumnName() {
+    $data = [];
+    // Base field with single column.
+    $data[] = [TRUE, ['foo'], 'foo', 'test'];
+
+    // Base field with multiple columns.
+    $data[] = [TRUE, ['foo', 'bar'], 'foo', 'test__foo'];
+    $data[] = [TRUE, ['foo', 'bar'], 'bar', 'test__bar'];
+    // Bundle field with single column.
+    $data[] = [FALSE, ['foo'], 'foo', 'test_foo'];
+    // Bundle field with multiple columns.
+    $data[] = [FALSE, ['foo', 'bar'], 'foo', 'test_foo'];
+    $data[] = [FALSE, ['foo', 'bar'], 'bar', 'test_bar'];
+    // Bundle field with reserved column.
+    $data[] = [FALSE, ['foo', 'bar'], 'deleted', 'deleted'];
+
+    return $data;
+  }
+
   /**
    * Sets up a field storage definition for the test.
    *
@@ -244,11 +326,11 @@ public function testGetExtraColumns() {
    *
    * @return \Drupal\Core\Field\FieldStorageDefinitionInterface|\PHPUnit_Framework_MockObject_MockObject
    */
-  protected function setUpDefinition($name, array $column_names) {
+  protected function setUpDefinition($name, array $column_names, $base_field = TRUE) {
     $definition = $this->getMock('Drupal\Tests\Core\Field\TestBaseFieldDefinitionInterface');
     $definition->expects($this->any())
       ->method('isBaseField')
-      ->will($this->returnValue(TRUE));
+      ->willReturn($base_field);
     $definition->expects($this->any())
       ->method('getName')
       ->will($this->returnValue($name));

core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php

@@ -281,7 +281,7 @@ public function testOnEntityTypeCreate() {
     );
 
     $this->fieldDefinitions = $this->mockFieldDefinitions(array('id'));
-    $this->fieldDefinitions['id']->expects($this->once())
+    $this->fieldDefinitions['id']->expects($this->any())
       ->method('getColumns')
       ->will($this->returnValue($columns));
     $this->fieldDefinitions['id']->expects($this->once())