Add a way to get all the tables in which a field is stored from TableMappingInterface

\Drupal\Core\Entity\Sql\DefaultTableMapping gives me headache.

No way to get the dedicated revision table of a field from TableMappingInterface

Isn't \Drupal\Core\Entity\Sql\DefaultTableMapping::getDedicatedRevisionTableName let us do that?

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -99,9 +99,9 @@ public function addField($field, $type, $langcode) {
   -      // If there is revision support, only the current revisions are being
   ...
   +      // If there is revision support, all revisions are being queried, and the
   

   Just curious when did this change? Also, the updated comment doesn't make sense to me.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -349,21 +349,41 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
   -      if ($field->getCardinality() != 1) {
   ...
   +      if ($table_mapping->requiresDedicatedTableStorage($field_storage_definition)) {
   

   We need comment here to explain what is going on.

3. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -286,7 +285,7 @@ public function getViewsData() {
   -          $table = $table_mapping->getDedicatedDataTableName($field_definition->getFieldStorageDefinition());
   +          $table = $table_mapping->getFieldTableName($field_definition->getName());
   
   @@ -299,7 +298,7 @@ public function getViewsData() {
   -            $revision_table = $table_mapping->getDedicatedRevisionTableName($field_definition->getFieldStorageDefinition());
   +            $revision_table = $table_mapping->getFieldRevisionTableName($field_definition->getName());
   

   We'd need a change record for this.

Never mind my question it is already answered in IS.

which makes the interface kind of useless and prevents alternative SQL storage / table mapping implementations.

In general I'm onboard with the proposed solution and the end goal, however I'm not sure about a) this being a bug and not a task and b) this targeting 8.5.x. This does not seem the kind of change allowed in a patch release.

Some more specific thoughts:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -182,6 +182,59 @@ public function getFieldTableName($field_name) {
   +  public function getFieldRevisionTableName($field_name) {
   

   Since this and its data table counterpart are becoming the new canonical way to retrieve this info, can we add static caching to both?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -182,6 +182,59 @@ public function getFieldTableName($field_name) {
   +      // place where field revision data is stored, otherwise the base table is
   

   "... otherwise the revision table"

3. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -182,6 +182,59 @@ public function getFieldTableName($field_name) {
   +      // responsible for storing field data. Revision metadata is an exception
   +      // as it's stored only in the revision table.
   

   I think we can remove the last sentence in this case.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -182,6 +182,59 @@ public function getFieldTableName($field_name) {
   +      // Collect field columns.
   +      $field_columns = [];
   +      foreach (array_keys($storage_definition->getColumns()) as $property_name) {
   +        $field_columns[] = $this->getFieldColumnName($storage_definition, $property_name);
   +      }
   +
   +      foreach (array_filter($table_names) as $table_name) {
   +        $columns = $this->getAllColumns($table_name);
   +        // We assume finding one field column belonging to the mapping is enough
   +        // to identify the field table.
   +        if (array_intersect($columns, $field_columns)) {
   +          $result = $table_name;
   +          break;
   +        }
   +      }
   

   If I'm not mistaken this logic is common to both methods, can we move it into a separate protected helper method?

5. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
   @@ -120,4 +120,32 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
   +  public function requiresDedicatedTableStorage(FieldStorageDefinitionInterface $storage_definition);
   

   I can see why this could be moved to the interface, it's a generic enough concept, however I'm not sure it's necessary. If I'm not mistaken it'd be used only in the EQ backend: maybe knowing whether the field is not stored in the primary table would be enough?

   I think it would be useful to abstract the concept of primary table instead, i.e. where most field data is stored. In the core table layout, this primary table would be the data table for translatable entity types and the base table for non-translatable entity types.

I assume the usage you have in mind is for EQ and Views to do a check like: "if this field is not stored in the primary table, it uses a dedicated one so do this other logic".

I'm not sure about the Views code but the EQ code at first sight seems to distinguish between simple and complex query, where the complex case implies One (or more) join(s). My assumption was that it didn't to know whether the field lives in a non-primary shared table or dedicated table, any of the two would trigger the complex case. Of course I might be wrong, but I'd like to verify if that's the case, because it would be a shame to needlessly generalize the concept of dedicated table.

Looks good!

We still need CR and follow-up.

Also

+1

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -104,6 +104,16 @@ class DefaultTableMapping implements TableMappingInterface {
      /**
   
   @@ -276,22 +286,37 @@ public function getFieldNames($table_name) {
   +    $cache_key = $field_name
   +      . '.' . ($default_revision_only ? 'default_revision' : 'all_revisions')
   +      . '.' . ($default_language_only ? 'default_language' : 'all_languages');
   +
   +    if (isset($this->fieldStorageDefinitions[$field_name]) && !isset($this->fieldTableNames[$cache_key])) {
   

   Somehow not fully happy about the key, thought about a nested array instead but I guess a single key with a clear label is useful when debugging things.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
   @@ -108,16 +108,22 @@ public function getReservedColumns();
   -   * Gets the table name for a given column.
   +   * Gets the table name for a given field.
   

   Noticed this too yesterday ;)

3. +++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
   @@ -108,16 +108,22 @@ public function getReservedColumns();
   -   *   The name of the entity field to return the column mapping for.
   +   *   The name of the entity field to return the table name for.
   +   * @param bool $default_revision_only
   +   *   (optional) Whether to return the table which stores the default revisions
   +   *   (TRUE) or all revisions (FALSE). Defaults to TRUE.
   +   * @param bool $default_language_only
   +   *   (optional) Whether to return the table which stores the default languages
   +   *   (TRUE) or all translations (FALSE). Defaults to TRUE.
       *
   

   Writing this the second time because somehow I lost my previous comment.

   I'm rather confused by the variable names and descriptions.

   One part is the "only" suffix, that kind of sounds like setting it to FALSE returns more than one value to me.

   And while default revision makes sense as that's how the storage works, default_language IMHO does not. We don't store the default language (translation) in a different place than other others.

   It's also not really about translatable, as we actually store all fields in the data table, translatable or not.

   I'm not quite sure how to name it either. Internally we talk about base/data tables, but I suppose you'd prefer to consider that an implementation detail and different storages might handle things differently?

   Again revision is pretty clear, at least for configurable fields. But the second argument actually only makes sense for fields that are stored in multiple tables, which is for the most part only keys/metadata fields. Put different,y what is the expected return value of getFieldTableName('title', TRUE, TRUE) ? Right now in HEAD ,that returns 'node_field_data' because that is where it is stored. But didn't I ask for default translation only? :) One option might be to make it tristate, so that explicitly passing TRUE would allow us to say: no, this field is not stored in a non-translated way?. Not sure, just thinking out loud...

   Or we just drop that argument entirely, because we actually don't seem to be using it at all right now :) Which on the other side makes it impossible to get the field table name for the base/data table for those cases where you really want to know.

   But even for the revision argument is not fully clear. Base fields actually can be non-revisionable, in which case, unlike translation, they are *not* stored in revision_data. Given I have such a field, what is getFieldTableName('unrevisionable_field', FALSE) supposed to return? Right now it will return the data table. In which case the _only suffix kind of makes sense, but the description doesn't.

From that issue:

> I suggest that for any given field, table mapping implementations MAY provide additional tables for either the default revision only, or the default language only, or both

I get the theoretical idea, but languages and revisions are *not* the same in the storage and can IMHO not be treated equally.

You can load a specific revision but usually don't and just want the default , so it makes sense to optimize for that.

However, you can not and most likely never will be able to load just a single language. Our APIs are just not capable to do that and while it would be interesting for some use cases (sites with a large amount of languages), I don't see how it practically could work. Given that, it makes zero sense to support storing languages in separate tables? And if we treat the default language differently, why not store all translations in separate tables, where does it stop? ;)

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -289,22 +299,35 @@ public function getFieldNames($table_name) {
+  public function getFieldTableName($field_name, $all_revisions = FALSE) {

I like the idea of introducing the "all revisions" terminology also here, to be consistent with the Entity Query API, however I preferred the solution exposing a dedicated method for revisions.

AAMOF with that parameter we are still implying that a field may live in different tables depending on which revision we are dealing with.

IMHO the API would be cleaner if we got rid of the bool parameter and introduced a ::getAllRevisionsFieldTableName() method instead.

A dedicated method, besides making things more readable, allows subclassess to override only a specific behavior if they need to.

I think we switched to a single function here when you addded the translation argument?

I think that's also something we need to consider, two flags would then actually result in 4 combinations, so if we do add another concept in the future, then we would need to add two additional methods. .

Patch still applies and I have updated the core version in the change notice so RTBC.

Committed 329a3d9 and pushed to 9.1.x. Thanks!