Add the ability to use a prefix for all the tables in a table mapping

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -591,17 +605,17 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +    // Limit the string to 48 characters minus the prefix length, keeping a 16
   +    // characters margin for db prefixes.
   +    if (strlen($table_name) > 48 - strlen($this->prefix)) {
   

   Since $table_name contains the prefix at this point, I think this change is unnecessary.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
   @@ -160,11 +160,11 @@ public function convertToRevisionable(array &$sandbox, array $fields_to_update =
   -          $temp_table_name = TemporaryTableMapping::getTempTableName($new_table_name);
   +          $temp_table_name = static::getPrefixedTableName($new_table_name, 'tmp_');
   ...
              $this->database->schema()->renameTable($temp_table_name, $new_table_name);
   

   I guess this is a pre-existing issue (at least for dedicated field tables), but how this actually work? Since DefaultTableMapping::generateFieldTableName() will generate a different result than SqlContentEntityStorageSchemaConverter::getPrefixedTableName() I don't understand how $temp_table_name can exist at this point.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
   @@ -487,24 +487,43 @@ protected function createTemporaryDefinitions(array &$sandbox, array $fields_to_
   +    $updated_storage_definitions = $this->updateFieldStorageDefinitionsToRevisionable($temporary_entity_type, $sandbox['original_storage_definitions'], $fields_to_update, FALSE);
   ...
        $storage->setTemporary(TRUE);
   ...
   -    $updated_storage_definitions = $this->updateFieldStorageDefinitionsToRevisionable($temporary_entity_type, $sandbox['original_storage_definitions'], $fields_to_update, FALSE);
   

   Interesting that this is moved, especially with the setTemporary() call in between. Can you explain that?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
   @@ -487,24 +487,43 @@ protected function createTemporaryDefinitions(array &$sandbox, array $fields_to_
   +    // Limit the string to 48 characters minus the length of the prefix, keeping
   +    // a 16 characters margin for db prefixes.
   +    if (strlen($table_name) > 48 - strlen($prefix)) {
   

   This one is correct, I think, but wouldn't it be more intuitive to do strlen($prefixed_table_name) > 48?

FIrst of all, I forgot to point this out before: (Once again) Awesome work!! This is a really neat consolidation that removes a weirdness while introducing greater flexibility at the same time. What's not to love!

Not sure how that worked for longer dedicated table names, but we actually have a simpler way to map temporary table names to the ones that will be used after the update, since we have both table mapping objects available.

Awesome, you are really on a roll with removing weirdness from Drupal at the moment. Nice!

I moved that line above because I think it makes the code easier to follow if both lines for getting the entity type and field storage definitions stay together. The setTemporary() call does not impact the array of updated storage definitions, it's just needed before getting the table mapping :)

Well, if you put it that way... ...it totally makes sense! Thanks for the explanation.

Some minor points on the new patch/interdiff:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -591,17 +605,25 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +      $entity_type_id = $storage_definition->getTargetEntityTypeId();
   

   Let's move it outside of the if so we can use it when building $table_name above, as well.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -591,17 +605,25 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +      // Ensure that the combined length of the separator, the field hash and
   +      // the prefix does not exceed 16 characters.
   +      $prefix_length = $revision ? 2 : 4;
   

   The logic here is correct, it's a bit hard to understand, though. Can we expand the comment just a tad, to note that 16 - 10 - (4|2) = 2|4?

Leaving at "needs review", though, as more eyes wouldn't hurt and my points aren't very substantial.

Wow, I almost don't want to RTBC in order not to stop you from removing more messy stuff ;-)

It really wouldn't hurt to get another pair of eyes on this, though. So leaving at "needs review" again, but it really does look very, very nice.

My try at winning the nitpick of the month award:

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -3,6 +3,7 @@
+use Drupal\Core\Entity\EntityType;

@@ -604,22 +605,21 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
+    $entity_type_id = substr($storage_definition->getTargetEntityTypeId(), 0, EntityType::ID_MAX_LENGTH);

Super-nit: Let's use EntityTypeInterface instead.

All right, I guess no one else wants to look at this. Read through the patch again and it again made me very happy ;-)

Let's get this in.

I really like where this is going but there are a few choices that I don't understand (sorry if they were discussed above):

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -590,18 +605,25 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +    $entity_type_id = substr($storage_definition->getTargetEntityTypeId(), 0, EntityTypeInterface::ID_MAX_LENGTH);
   

   Why do we need this? Is there any chance the entity type ID is actually greater than 32?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -590,18 +605,25 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +      // Ensure that the combined length of the field hash (10), the separator
   +      // (4|2) and the prefix does not exceed 16 chars: 16 - 10 - (4|2) = 2|4.
   +      $prefix_length = $revision ? 2 : 4;
   +      $prefix = substr($this->prefix, 0, $prefix_length);
   

   I'm not sure about this logic: if I'm understanding correctly, this may end up mangling the prefix, which is not what I would expect, if I explicitly bothered to provide one. I think I'd rather be fine with limiting the prefix length to 38 (48 - hash length) and have a table name that may end up being only "prefix_hash" in the worst case scenarios. This would be a generalization of what TemporaryTableMapping::getTempTableName() is currently doing, so we'd also be BC (FWIW), I think.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -336,15 +336,19 @@ public function getTableMapping(array $storage_definitions = NULL) {
   +    $prefix = $prefix ?: ($this->temporary ? 'tmp_' : '');
   

   Why do we still need to the temporary property on the storage? If I'm reading SqlContentEntityStorageSchemaConverter's code correctly, we are already using custom table mappings for all the schema changes, we only need to inject a temp entity type definition and table mapping to store data in the temp tables, right? I'm looking at this code in particular:

         // Create the updated entity schema using temporary tables.
         /** @var \Drupal\Core\Entity\Sql\SqlContentEntityStorage $storage */
         $storage = $this->entityTypeManager->getStorage($this->entityTypeId);
         $storage->setTemporary(TRUE);
         $storage->setEntityType($sandbox['temporary_entity_type']);
         $storage->onEntityTypeCreate($sandbox['temporary_entity_type']);
   

   I might be wrong, but it seems to me that we can get rid of the temporary property, if we create a $temp_storage clone and inject the temp table mapping and temp entity type definition. If we do that we can keep the $storage object untouched and we no longer need to swap table mapping and definition back and forth, which is a pattern we are trying to get rid of.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
   @@ -150,22 +150,18 @@ public function convertToRevisionable(array &$sandbox, array $fields_to_update =
   -        $old_table_name = TemporaryTableMapping::getTempTableName($table_name, 'old_');
   +        $old_table_name = static::getPrefixedTableName($table_name, 'old_');
   

   Why do we need a dedicated method for this? Isn't a mapping with the old_ prefix enough? Is it because the old_ prefix might have been mangled by the table mapping (see bullet 1)?

5. +++ b/core/lib/Drupal/Core/Entity/Sql/TemporaryTableMapping.php
   @@ -4,8 +4,13 @@
   +@trigger_error(__NAMESPACE__ . '\TemporaryTableMapping is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Use the default table mapping with a prefix instead.', E_USER_DEPRECATED);
   

   Don't we need test coverage for this?

6. +++ b/core/modules/system/tests/modules/entity_test_update/src/EntityTestUpdateStorageSchema.php
   @@ -18,7 +18,7 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
   -      $schema[$entity_type->getBaseTable()]['indexes'] += \Drupal::state()->get('entity_test_update.additional_entity_indexes', []);
   +      $schema[$this->storage->getTableMapping()->getBaseTable()]['indexes'] += \Drupal::state()->get('entity_test_update.additional_entity_indexes', []);
   

   Why is this change needed? Is it because previously we used to temporarily update the entity type definition and now we don't need to?

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -336,15 +336,19 @@ public function getTableMapping(array $storage_definitions = NULL) {
+    $table_mapping_class = DefaultTableMapping::class;
...
+    return $table_mapping_class::create($entity_type, $storage_definitions, $prefix);

Minor nitpick since this was kicked back anyway:

Now that this has been simplified let's just use DefaultTableMapping::create() directly instead of storing the class name separately.

Looks way better now, thanks!

@amateescu, #18:

5. Not sure what we would actually test? That class is no longer used in core but we're also not changing anything in it..

I think a two lines test method in a unit test instantiating the temp table mapping and asserting the error would be enough. I’m not sure how much flexibility we have around test coverage for deprecations, but the docs don’t seem to imply it’s optional…

@amateescu, #19:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -590,18 +605,27 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +      if (strlen($table_name) > 48) {
   

   I tried hard not to suggest to store 48 in a variable, but I couldn't help myself ;)
   (they are only two, I know)

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -590,18 +605,27 @@ public function getDedicatedRevisionTableName(FieldStorageDefinitionInterface $s
   +        $prefix = substr($this->prefix, 0, 38);
   +        $table_name = $prefix . $field_hash;
   

   Unfortunately this logic will produce the same table name for data and revision tables. We need to add also the separator to the mix, which means we have a maximum length for prefixes of 34 chars. We should probably document this somewhere. We'll also need test coverage for this.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchemaConverter.php
   @@ -149,23 +149,19 @@ public function convertToRevisionable(array &$sandbox, array $fields_to_update =
   +      $backup_table_name_mapping = array_combine($sandbox['original_table_mapping']->getTableNames(), $sandbox['backup_table_mapping']->getTableNames());
   

   The name for this variable is correct, however at first sight it confused me because it has "table" and "mapping" in it. I'm wondering whether the code would be more readable if we just went with something like $backup_table_names, without stressing too much on the mapping concept. Your call, but I thought it would be good to point this out.

Looks like all review comments from #20 are addressed. Let's get it in!

Saving credits

Committed 76fdeba and pushed to 8.7.x.

Backport to 8.6.x will need to be discussed with release managers.

Discussed this with @catch: we prefer to address this problem space in 8.7.x.

+++ b/core/lib/Drupal/Core/Entity/Sql/TemporaryTableMapping.php
@@ -4,8 +4,13 @@
+@trigger_error(__NAMESPACE__ . '\TemporaryTableMapping is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Use the default table mapping with a prefix instead.', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php
@@ -445,6 +446,142 @@ public function testGetFieldTableNameInvalid() {
+   * @expectedDeprecation Drupal\Core\Entity\Sql\TemporaryTableMapping is deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0. Use the default table mapping with a prefix instead.

s/8.6.x/8.7.x