DefaultTableMapping::getFieldColumName is broken for base tables

Let's try this.

Done :)

Correct interdiff

Documented and cleaned-up things a bit

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -130,18 +130,38 @@ public function getFieldNames($table_name) {
   +      $this->columnMapping[$field_name] = array();
   +      foreach (array_keys($this->fieldStorageDefinitions[$field_name]->getColumns()) as $column_name) {
   +        $this->columnMapping[$field_name][$column_name] = $this->getFieldColumnName($this->fieldStorageDefinitions[$field_name], $column_name);
   +      }
   +    }
   +    return $this->columnMapping[$field_name];
   

   Lovely :-)
   For readability, I guess we could extract
   $storage_definition = $this->fieldStorageDefinitions[$field_name]; ?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -130,18 +130,38 @@ public function getFieldNames($table_name) {
   +  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $column_name) {
   

   So we're not :
   - moving to "only use the column name without prefixing with the field name in dedicated tables",
   - nor fixing the inconsistency of single or double underscore separator
   in that issue ?

Very nice fix. I briefly discussed this with @dawehner in IRC a while back but I did not come up with this solution. Great!

One thing though:

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -130,18 +130,38 @@ public function getFieldNames($table_name) {
+  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $column_name) {
...
+    return $column_name;

I thinl double usage of variables is not only rather confusing but is also very error-prone if we ever decide to touch this in the future. Let's rename either of the $column_name variables please.

@tstoeckler - double usage of $column_name variable : very true.

Actually, it's a problem that the var names and method names use "column name" for both:
- the raw property name in the field schema ('value')
- the actual, final column name in the schema ('field_name__value')

Since getFieldColumnName() is in TableMappingInterface, maybe keep "column name" for the actual schema column, and "property" for the raw property name ?

Good points, thanks!

Sorry :-/

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -130,18 +131,40 @@ public function getFieldNames($table_name) {
   +      foreach (array_keys($this->fieldStorageDefinitions[$field_name]->getColumns()) as $column_name) {
   

   foreach (aray_keys($field_storage_definition)->getColumns() as $property_name)) {

   I know the FSDI method is getColumns() :-/, but IMO we should name our vars according to the business logic this code here is about, which is : "compute schema column names for a field property".

   We want to avoid
   $mapping[$column_name] = get the column name for column name :-p

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -130,18 +131,40 @@ public function getFieldNames($table_name) {
           foreach (array_keys($this->fieldStorageDefinitions[$field_name]->getColumns()) as $column_name) {
   +        $storage_definition = $this->fieldStorageDefinitions[$field_name];
   

   I meant $storage_definition should be defined one line up and used in the foreach() line as well :-p

3. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -130,18 +131,40 @@ public function getFieldNames($table_name) {
   +  public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $property_name) {
   +    $column_name = $property_name;
   

   Why do we start with $column_name = $property_name ?
   There's currently no case where the final column name in the schema is equal to the property name, right ?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
   @@ -94,12 +94,12 @@ public function getReservedColumns();
   -   * @param string $column
   +   * @param string $property_name
       *   The name of the column.
   

   The param description text should be updated accordingly ? :-)

Why do we start with $column_name = $property_name ?
There's currently no case where the final column name in the schema is equal to the property name, right ?

Actually there is one case, I refactored the related code a bit to make it more clear.

One more fix

Forgot to update unit tests :(

Attaching interdiff #18

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageException.php
@@ -0,0 +1,16 @@
+ * Exception thrown when a SQL storage operation fail.

Extreme nitpick: fail should fails ?

@plach: yay, thanks !

Once @swentel's #24 is fixed, this is RTBC if green.

Here it is

Thanks !

The bot will tell us when he is kind enough to grab this :-)

Looks like a bot fluke

I had to revert the changes introduced in #21: it broke entity query conditions on the deleted field property.

OK, never mind then, we'll keep the cleanup of reservedColumns() for another issue. Sorry about that.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bcfde9e and pushed to 8.0.x. Thanks!

Opened #2392809: EFQ should not call DefaultTableMapping::getFieldColumName() for the 'deleted' column