SqlContentEntityStorage::countFieldData() fatals for revision metadata fields and the UUID field

On this

Here's a test

And a fix

Still needs a test for the revision metadata stuff.

And a test for revision meta fields

well I could reverse the table names, by nature of the way they are constructed, you end up with this order:

• base
• data
• revision_base
• revision_data

Thoughts?

Looks ready

I think we need to address #10

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -334,4 +334,17 @@ protected function generateFieldTableName(FieldStorageDefinitionInterface $stora
+   * {@inheritdoc}
+   */
+  public function getFieldTableName($field_name) {
+    foreach ($this->getTableNames() as $table_name) {
+      $columns = $this->getAllColumns($table_name);
+      if (in_array($field_name, $columns, TRUE)) {
+        return $table_name;
+      }
+    }
+    return FALSE;
+  }
+
 }

mh, what about fields like 'nid' or 'type' which are stored in both?

Let's at least wait until plach can bring up his proposal he was working on already.

[edit: crosspost]

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -334,4 +334,17 @@ protected function generateFieldTableName(FieldStorageDefinitionInterface $stora
   +  public function getFieldTableName($field_name) {
   +    foreach ($this->getTableNames() as $table_name) {
   +      $columns = $this->getAllColumns($table_name);
   +      if (in_array($field_name, $columns, TRUE)) {
   +        return $table_name;
   +      }
   +    }
   +    return FALSE;
   +  }
   

   Is that really correct ? A table storing a field is in no way guaranteed to contain a column named exactly after that field.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
   @@ -102,4 +102,16 @@ public function getReservedColumns();
   +   * Returns the table name for a given column.
   +   *
   +   * @param string $field_name
   +   *   The field name for which the table name is sought.
   ...
   +  public function getFieldTableName($field_name);
   

   So is the param a column name or a field name ?

Discussed this with Daniel and Alex:

We can return a single table and inspect them in order of "importance": data table, base table, revision table. Views would use ::getFieldTableName() to determine in which table the field should be listed so it would appear just once.

Working on ::getFieldTableName().
(just a warning since this is already assigned to me ;)

This implements #16 and should address #15. It also provides additional test coverage for the new method.

I added a @todo to revisit some code in ::countFieldData() because, after various attempts, I was not able to make every test pass without falling back to the data or base table in case no field information is available. We should be able to fix this in #2274017: Make SqlContentEntityStorage table mapping agnostic .

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -129,6 +140,53 @@ public function getFieldNames($table_name) {
+      $storage = \Drupal::entityManager()->getStorage($this->entityType->id());

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -294,7 +294,7 @@ public function getTableMapping(array $storage_definitions = NULL) {
+      $table_mapping = new DefaultTableMapping($this->entityType, $definitions);

Can we pass the storage controller in as an argument? It should be in scope as $this.

Other than that, I think this is good to go

Sorry, I should have clarified that: I did not inject the storage itentionally because in #2274017: Make SqlContentEntityStorage table mapping agnostic we are likely to be doing the opposite, that is making the storage rely on the table mapping, which will allow us to remove that line. Hence the current approach is likely to avoid API changes in the future.

works for me, I think I'm ok to rtbc this, I added the tests but the rest is largely re-done from scratch

A couple of small clean-ups.

unless bot says no

I don't think we need a CR for the changes to DefaultTableMapping's constructor since this is internal to SQL entity storage.

The issue summary does not outline the proposed resolution. Can be re-rtbc'd once it does.

Well I committed it by mistake. I'm not going to revert as this only needs an issue summary update and @plach promised me one so all good here.

And here it is :)

Moar

There's one thing which was forgotten here: we also need to provide a way to get the revision table name for a field if the SQL storage chooses to store default / all revisions in separate tables.

Opened a new issue to fix this: #2955442: Add a way to get all the tables in which a field is stored from TableMappingInterface