Add an API for converting entity type schemas to (non-)revisionable/(non-)translatable, with or without data

+1 I think this is a great idea!

Since this is an API addition, we'll need to add test coverage I assume?

This depends on #2960136: Add the ability to use a prefix for all the tables in a table mapping.

The blocker is in.

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeSchemaListenerInterface.php
   @@ -0,0 +1,28 @@
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $original
   ...
   +  public function onEntityTypeSchemaUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original, array $field_storage_definitions, array $original_field_storage_definitions, array &$sandbox = NULL);
   

   This should be named as $original_entity_type everywhere in the patch.

2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,167 @@
   +    // @todo Add any required safeguards.
   

   Do we want to finish it here or do we need a follow up?

3. +++ b/core/lib/Drupal/Core/Entity/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,167 @@
   +    // Copy data.
   

   s/Copy data/Copy data from original storage to temp storage/

4. +++ b/core/lib/Drupal/Core/Entity/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,167 @@
   +        // @todo Only set the fields needed based on the type of the conversion.
   +        // Set the revision ID to be same as the entity ID.
   

   Do we need a follow-up?

I think we need a change notice here referencing this and #2960136: Add the ability to use a prefix for all the tables in a table mapping because this API can be used to change a string basefield length or convert an int basefield to string.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +  public function onEntityTypeSchemaUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original, array $field_storage_definitions, array $original_field_storage_definitions, array &$sandbox = NULL) {
   

   I love the sanity checks this is running!

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
   @@ -0,0 +1,540 @@
   +  protected function updateEntityTypeDefinition($revisionable = FALSE, $translatable = FALSE) {
   

   I recall not having a data table a long while ago and that was preventing me from adding translations. If I read the tests correctly, this will make it possible for an entity type to go from translatable=FALSE/data_table=NULL to translatable=TRUE/data_table=foobar without data loss? If so, then hot damn I wish you worked on this 2.5 years ago before I wrote group_update_8002

Awesome work!

That is some really impressive work!

The code looks good to me, I also did a lot of manual tests to see how is this actually working and the results were good. Except one case (which is also pointed out in the tests): from Revisionable and Not Multilingual to Revisionable and Multilingual. The question is if we should add support for this case in this issue or not.

I'm done reviewing #22, tomorrow I'll post what I have. Meanwhile I launched a few additional test runs and it seems we have issues with PHP 5.6 and PgSQL.

Great stuff, btw :)

Most items are nitpicks but I have one serious concern about the current approach, see bullet 7.

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeListener.php
   @@ -115,4 +115,27 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +  public function onEntityTypeSchemaUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original, array $field_storage_definitions, array $original_field_storage_definitions, array &$sandbox = NULL) {
   

   When originally designing the entity storage API, we tried hard not to expose the concept of schema in the public API, given that some storage types don't rely on any schema. This is the main reason why the storage schema handler is instantiated by the storage handler. It's true that EntityDefinitionUpdateManager internally breaks this attempt, but at least the public API is "clean". Given that the new method is a replacement for ::onEntityTypeUpdate() in disguise (I think we even mentioned the possibility of deprecating the latter in favor of the former), I'm wondering whether we can find a more neutral name, e.g. something like ::onEntityTypeUpdateWithData(). This name might work better even for storage types that don't have a schema to adjust but may decide to store stuff differently, given the updated entity type/field definitions.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +trait EntityTypeSchemaListenerTrait {
   

   I didn't realize this was meant to be storage-agnostic, exactly for the reason pointed out in #27. But then I noticed this was (already) in the Sql namespace and I thought that for consistency it would be good to add a Sql prefix to the class name.

   I think it's ok to move it the upper namespace level, once it's actually storage-agnostic, but I think we should mark it as @internal for now, if that's the plan.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +      throw new EntityStorageException('Converting an entity type from revisionable to non-revisionable or from translatable to non-translatable is not supported.');
   

   Why would we forbid such changes? Just to prevent unintended data loss? Couldn't we make them available via an opt-in site setting?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +  abstract protected function preUpdateEntityTypeSchema(EntityTypeInterface $entity_type, EntityTypeInterface $original, array $field_storage_definitions, array $original_field_storage_definitions, array &$sandbox = NULL);
   ...
   +  abstract protected function postUpdateEntityTypeSchema(EntityTypeInterface $entity_type, EntityTypeInterface $original, array $field_storage_definitions, array $original_field_storage_definitions, array &$sandbox = NULL);
   

   I'm wondering whether it would make sense to have empty methods instead. We do this for exception handling. For instance a class might need to implement just one of the two.

5. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +        // Set the 'revision_translation_affected' flag to TRUE to match the
   +        // previous API return value: if the field was not defined the value
   +        // returned was always TRUE.
   

   Is this a copy/pasted comment? I'm not sure mentioning the previous API makes much sense here.

6. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +        if (!$original->isRevisionable() && !$original->isTranslatable() && $entity_type->isRevisionable() && $entity_type->isTranslatable()) {
   

   Wouldn't this be skipped when starting from a rev non_mul or a non_rev mul status? Shouldn't this be:

   if ((!$original->isRevisionable() || !$original->isTranslatable()) && $entity_type->isRevisionable() && $entity_type->isTranslatable()) {
   

7. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,239 @@
   +        // Treat the entity as new in order to make the storage do an INSERT
   +        // rather than an UPDATE.
   +        $entity->enforceIsNew(TRUE);
   +
   +        // Finally, save the entity in the temporary storage.
   +        $temporary_storage->save($entity);
   

   I know this logic was not introduced here, but this does not seem right to me after looking it with a fresh eye. We are not creating a new entity, we are just storing it again. Making this operation appear as an entity insertion could trigger all sorts of unwanted side effects, e.g. email notifications, statistics updates, integrity constraint violations (see the attached patch for an example), and so on. I really think this outlines the need for a new storage method (e.g ::restore($entity)), which would internally re-use the ::save() logic, and would internally trigger an insertion if needed, but would appear as an update. Alternative ideas are welcome, but I played with this problem in my mind for a while and couldn't come up with a better suggestion.

8. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +        throw new EntityStorageException("The entity update process failed while processing the entity type {$entity_type->id()}, ID: {$entity->id()}$error_revision_id.", $e->getCode(), $e);
   

   Aren't we missing a space between ID and revision ID?

9. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
   @@ -0,0 +1,246 @@
   +    $this->fieldStorageDefinitions = $field_storage_definitions;
   

   It's unfortunate we still have to mess with the internal state to get things right. Is there a plan to improve on this in later iterations? If not, would it make sense to backup/restore the original property values?

10. +++ b/core/lib/Drupal/Core/Entity/Sql/EntityTypeSchemaListenerTrait.php
    @@ -0,0 +1,246 @@
    +    $sandbox['max'] = (int) $this->database->select($table_name, 't')->countQuery()->execute()->fetchField();
    +    $sandbox['#finished'] = empty($sandbox['max']) ? 1 : ($sandbox['progress'] / $sandbox['max']);
    

    I'm not sure this logic is correct: if maintenance mode is off, entities could be added but also deleted, which would mess with the max value and potentially make the process never end. I think we should keep track of the latest seen ID and calculate the finished value as:

      $missing = $count_of_IDs_greater_than_last_seen_ID;
      $sandbox['#finished'] = $sandbox['progress'] / ($sandbox['progress'] + $missing);
    

11. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -408,6 +409,134 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +    $sandbox['current_table_mapping'] = $temporary_storage->getCustomTableMapping($entity_type, $field_storage_definitions);
    +    $sandbox['backup_table_mapping'] = $original_storage->getCustomTableMapping($original, $original_field_storage_definitions, 'old_');
    

    I had to read this code a few times to figure out what "current" was meaning. I'm wondering whether "new" would be easier to understand. Also, I'm wondering whether tmp_ and old_ are enough as prefixes, since they might clash with existing table names, e.g. manual backups. Couldn't we add a short hash to the prefix, e.g. the first 6 chars of the hash of the serialized table mapping, to reduce the likelihood of conflicts?

12. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -408,6 +409,134 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +    $schema = array_intersect_key($schema, $temporary_table_names);
    

    Can we add a comment to explain why these are needed?

13. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -408,6 +409,134 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +      $this->database->schema()->renameTable($original_table_name, $backup_table_name);
    

    Shouldn't we handle exceptions here and restore the original names if any occurred?

14. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -408,6 +409,134 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +      throw $e;
    

    Won't this pollute the stack trace? Wouldn't it be better to wrap it into a new exception?

15. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -408,6 +409,134 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    +    foreach ($backup_table_names as $original_table_name => $backup_table_name) {
    

    Could we add a site setting to preserve the backup tables? We could even make it opt-in, just be safe.

16. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -666,7 +793,9 @@ protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $res
    +      $table_mapping = $this->storage->getCustomTableMapping($entity_type, $this->fieldStorageDefinitions);
    

    Shouldn't we instantiate the storage class from the entity type as we do elsewhere in this patch?

17. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +    ConfigurableLanguage::createFromLangcode('ro')->save();
    

    <3

18. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +      $entity = $storage->create([
    +        'id' => $i,
    +        'name' => 'test entity - ' . $i . ' - en',
    +      ]);
    

    Can we add also some values to test dedicated field storage?

19. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +    $updated_entity_type = $this->updateEntityTypeDefinition($new_rev, $new_mul);
    +    $updated_field_storage_definitions = $this->updateFieldStorageDefinitions($new_rev, $new_mul);
    

    This feels like a logic that regular update function will need to repeat as-is every time. I'm wondering whether it would make sense to add it to the entity type and entity field managers and maybe invoke it automatically from the entity definition update manager.

20. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +          $translation = $revision_2->getTranslation('ro');
    

    Can we create a new revision for the translation, just to reinforce good practices? ;)

21. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +  protected function assertEntityData($revisionable, $translatable) {
    

    Argh! Not one but two boolean parameters! :D

22. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +   * Returns an entity type definition, possibly updated to be rev or mul.
    

    This PHP doc is a bit weird: it makes me think that either the method name should be get... or that we should change the PHP doc itself to describe the method's primary intent.

23. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +  protected function assertEntityTypeSchema($revisionable, $translatable) {
    +    if ($revisionable && $translatable) {
    +      $this->assertRevisionableAndTranslatable();
    +    }
    +    elseif ($revisionable) {
    +      $this->assertRevisionable();
    +    }
    +    elseif ($translatable) {
    +      $this->assertTranslatable();
    +    }
    +  }
    

    Would it make sense to add an else branch asserting that the entity type is neither revisionable nor translatable? We are assuming that no change is performed wrt the initial schema, but we aren't actually testing it AFAICT.

24. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionSchemaUpdateTest.php
    @@ -0,0 +1,540 @@
    +    foreach (['revision_log_message', 'revision_user', 'revision_created'] as $key) {
    

    Don't these checks belong to ::assertRevisionable()?

@amateescu, #26:

Ideally I'd like to release this API with the ability to support all flavors of "migrations" (including lossy ones, as mentioned above). That said I guess it's ok to fix entity revision in a follow-up, as long as we make sure it's done before 8.7. It would probably be good to have a release manager's opinion on this.

@amateescu, #29:

1: Definitely the name is not great, unfortunately the right one is already taken (::onEntityTypeUpdate()). I was hoping that we could leverage PHP docs to clarify that the method handles safely also no-data scenarios. Maybe we should just deprecate ::onEntityTypeUpdate() and make that even clearer through the deprecation note. Happy to pick another name, but I'd really like to avoid mentioning "schema" in the main API entry point also because, once we fix those @todos in the EntityTypeListener, storage and schema might definitely not be the only affected subsystems. With that in mind, I think you're correct in suggesting that we should just add a new method to EntityTypeListenerInterface rather than introducing a new interface.
3: I read that comment, but I wasn't clear on whether that meant unintended data loss or any kind of data loss. I think there's nothing wrong with intended data-loss. Let me provide an example: one of the main use cases I had in mind for this API, when originally thinking about it, was to switch the entity schema from "mul rev" to "non_mul rev", to remove the performance penalty introduced by the "mul" schema in monolingual sites. If the site only contains monolingual entities, switching to "non_mul" would indeed cause some metadata loss, but no relevant information would be actually lost. If we don't support these scenarios through this API, it's not clear to me how they would be supported otherwise. As for the other missing cases, I'm personally fine with a follow-up as long as it's done in 8.7. Again, I'll ping a release manager to chime in about this.
9: Good point, thanks
15: The reason why I'm suggesting "opt-in" is that I'm scared by the idea of people running schema updates on prod sites without backups. I know this is definitely against good practices, but people are doing this kind of stuff all the time and then are complaining about Drupal sucking as soon as something goes wrong. As part of our effort of making minor updates friendlier, I think it would be preferable for people to have weird tables lingering around rather than not being able to recover their data after a failed update. If we add the hashes suggested in 11 and add a timestamp to the serialized data, we should be able to even run multiple updates without ever risking a collision. A UI (or a cron job) to purge these backups would be a nice to have, not a blocker IMO. I'd feel better if we did this here instead of punting it to a follow-up, but I'm fine with leaving the final word to the committer that will take care of this issue.
16: Right, I forgot about the "static dance" we're doing now :)
19: Very good point: +1 for a follow-up to be addressed once we have more experience with these updates.
21: I swear I was joking :)

7:

I'm not sure whether presenting this operation as an update rather than insert is preferable every time.

Update is probably not always ideal as well, but at least it's assumed to be run multiple times, whereas insert is assumed to be run just once in the whole entity lifecycle.

There could be similar badly written code out there [...]

If I'm not mistaken, that's exactly what the core forum_node_insert() hook implementation is doing. I suspect that also the revisionable taxonomy update would break with the current patch, if forum were enabled. [wrong: we're saving terms not nodes :)] I guess you're saying it's badly written because it's not using fields/entities, but that was just an example.

[...] that acts differently for insert vs. update, and in the case of a conversion from non-rev to rev, that code could actually miss that the initial/default revision of an entity was created.

Yep, I guess this something that could happen as well.

One idea I had a while ago was to use the new synchronizable concept [...], and make this a "syncing" operation.

This is an interesting idea, the only drawback I can think of is that it shifts the responsibility of dealing with this special use case onto hook implementations. This wouldn't be that bad if no implementation existed yet, but I suspect quite a few would need to be adapted to accommodate this change. The reason why I was suggesting a ::restore() method is that this is just a storage reshuffle, after all. Ideally we would like no hook to be called at all, which is what would normally happen in an update function if we performed this "migration" manually. Maybe we could just run the "save" operation with a mock module handler?

Interdiff review:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlEntityTypeSchemaListenerTrait.php
   @@ -219,8 +222,13 @@ protected function copyData(EntityTypeInterface $entity_type, EntityTypeInterfac
        // If we're not in maintenance mode, the number of entities could change at
        // any time so make sure that we always use the latest record count.
   

   We need to update the comment to reflect the logic change.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlEntityTypeSchemaListenerTrait.php
   @@ -219,8 +222,13 @@ protected function copyData(EntityTypeInterface $entity_type, EntityTypeInterfac
   +    $missing = $this->database->select($table_name, 't')
   +      ->condition("t.$identifier_field", $sandbox['current_id'], '>')
   +      ->orderBy($identifier_field, 'ASC')
   

   It should be ok to assume a serial ID, right? This is just an helper after all.

::restore($entity)

So is this for only saving to the storage, and not triggering presave/insert/update hooks at all? If we can make that work it sounds the cleanest option to me.

@catch, #31:

Well, that was an alternative idea: I think we could manage to perform a ::save() with no hooks invoked without introducing a new method by instantiating a storage with a mock module handler.

Btw, do you think the following issues should be considered blockers for this or is it ok to address them as follow-ups?

• #3003935: Entity lifecycle hooks are executed during SqlContentEntityStorageSchemaConverter::copyData
• The issue mentioned in #26
• The issue mentioned in #28.3 (see also #29.3 and #30.3)

Setting back to needs work for the TODOs mentioned in #29 and for the minor update suggested in #30.

@amateescu, #34:

If we want this short hash to be deterministic, what we could do instead is somehow compute a hash of the entity type + field storage definitions used to instantiate the table mapping. @plach, what do you think?

Sounds like a plan. I think it would be good to add also the current request time, just in case for some reason we perform the same update multiple times.

Regarding #28.1 (and #29.1 + #30.1), I just realized that EntityTypeListenerInterface::onEntityTypeCreate() is also as broken as onEntityTypeUpdate() :( [...]

I think this means we need the ability to specify an array of storage definitions on create. The only case where it would be legit to retrieve/specify the live definitions would be on module installation. This would match how we handle the DB schema currently.

So two out of three methods from EntityTypeListenerInterface are broken at the moment. I don't really have a good proposal right now, just wanted to start the discussion that maybe we should deprecate that interface entirely.

Deprecating the interface itself would mean having to find another name. Why is it not ok to just add two new methods and deprecate the old ones? Do you have an alternative terminology in mind that could help with naming?

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -787,6 +787,55 @@ public function save(EntityInterface $entity) {
+  public function restore(EntityInterface $entity) {
+    $transaction = $this->database->startTransaction();
+    try {
+      /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
+
+      // Insert the entity data in the base table only for default revisions.

I don't think we can skip pre-save field methods, e.g. PasswordItem would brake.

Would it be possible to try and reuse the full save workflow with a mock module handler?

[...] my suggestion is to put "fieldable" somewhere, either in the name of the new interface (FieldableEntityTypeListenerInterface) or in the name of two new methods on the existing interface: onFieldableEntityTypeCreate() and onFieldableEntityTypeUpdate().

Good call, I'd go with the latter, as we might get conflicting method names between when implementing both interfaces otherwise.

On a second thought PasswordItem is probably fine, since we need to hash the password only when it changes. Can we apply the same line of thinking to the other field types? I.e. if stored values don't change, we don't need preSave field methods?

This is looking very good, however I think we still have a few "open" issues:

• How to deal with lossy migrations (#28.3 / #30.3): we mentioned a follow-up, but without the exception would they work? In that case we could still make them opt-in by not throwing the exception if a PHP setting disables it.
• Backup tables retention (#28.15 / #30.15): as I mentioned, I'd prefer to keep the backup tables around unless the user opts out via a PHP setting and file a follow-up to clean them up via cron or maybe a queue task "scheduled" a few weeks later.
• What about #34/#35?

  I think this means we need the ability to specify an array of storage definitions on create. The only case where it would be legit to retrieve/specify the live definitions would be on module installation. This would match how we handle the DB schema currently.

Quick code review of the latest changes:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -787,6 +787,59 @@ public function save(EntityInterface $entity) {
   +    // Allow code to run before saving.
   +    $entity->preSave($this);
   +    $this->invokeFieldMethod('preSave', $entity);
   

   These should be moved into the transaction scope to match the save behavior.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -787,6 +787,59 @@ public function save(EntityInterface $entity) {
   +    $transaction = $this->database->startTransaction();
   +    try {
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
   ...
   +      // Ignore replica server temporarily.
   +      \Drupal::service('database.replica_kill_switch')->trigger();
   +    }
   +    catch (\Exception $e) {
   +      $transaction->rollBack();
   +      watchdog_exception($this->entityTypeId, $e);
   +      throw new EntityStorageException($e->getMessage(), $e->getCode(), $e);
   +    }
   

   To reduce the amount of duplicated code, would it make sense to move this to a ::sqlStore() method (better name welcome :) taking a callable as parameter, similar to ::wrapSchemaException()?

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -787,6 +787,59 @@ public function save(EntityInterface $entity) {
   +      // Insert the entity data in the data table only for default revisions.
   +      if ($this->dataTable && $entity->isDefaultRevision()) {
   +        $this->saveToSharedTables($entity);
   +      }
   

   I'd move this into the base table's if branch...

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -787,6 +787,59 @@ public function save(EntityInterface $entity) {
   +      // Insert the entity data in the revision data table.
   +      if ($this->revisionDataTable) {
   +        $this->saveToSharedTables($entity, $this->revisionDataTable);
   +      }
   

   ... and this into the revision table's if branch.

I discussed #50 with @amateescu today in Slack:

• Regarding lossy migrations, we found a solution that we both find satisfying: we will address them in a follow-up and we will only allow them if there is no actual data loss involved in the migration. This means that to downgrade an entity type from translatable to non-translatable, we require that no translated entity is stored. Similarly, to downgrade an entity type from revisionable to non-revisionable, we require that no revision besides the default one is actually stored for each entity.
• Regarding keeping backup tables, we will leave the final word to committers.

Here is my (hopefully) last code review. No blockers besides the missing follow-up references, but some final touches would be welcome :)

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeListener.php
   @@ -115,4 +115,27 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +    if ($sandbox === NULL || (isset($sandbox['#finished']) && $sandbox['#finished'] === 1)) {
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlFieldableEntityTypeListenerTrait.php
   @@ -0,0 +1,241 @@
   +      // If there is no existing data, we still need to run the
   +      // post-schema-update tasks.
   +      $sandbox['#finished'] = 1;
   ...
   +    if ($sandbox['#finished'] === 1) {
   +      $this->postUpdateEntityTypeSchema($entity_type, $original, $field_storage_definitions, $original_field_storage_definitions, $sandbox);
   +    }
   ...
   +    $sandbox['#finished'] = $missing ? $sandbox['progress'] / ($sandbox['progress'] + (int) $missing) : 1;
   

   We are treating $sandbox['progress'] inconsistently: sometimes it's an int and sometimes it's a float. Given that we are using the strict equality operator, this is only working because of that ternary operator in the last quoted line. That isn't actually necessary if we always treat progress as a float. This feels fragile to me, at the moment, and it's a critical bit.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -408,6 +411,186 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +      if (!$schema_handler->tableExists($temp_table_name)) {
   +        $schema_handler->createTable($temp_table_name, $table_schema);
   +      }
   ...
   +          if (!$schema_handler->tableExists($temp_table_name)) {
   +            $schema_handler->createTable($temp_table_name, $table_schema);
   +          }
   

   I'm not sure about those ifs: I think that if temp tables already exist something is wrong, so maybe it would be better to get a schema exception while attempting to create the table. In general all these edge cases around schema creation and table renames don't seem to have test coverage, it would be great to create a follow-up to add that.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -408,6 +411,186 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +          // The shared tables are already fully created, but we need to save the
   +          // per-field schema definitions for later use.
   

   The comment doesn't wrap at 80 chars.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -408,6 +411,186 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +    // At this point the update process either finished successfully or any
   +    // error has been thrown already, so we can drop the backup entity tables.
   +    foreach ($backup_table_names as $original_table_name => $backup_table_name) {
   +      $this->database->schema()->dropTable($backup_table_name);
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlFieldableEntityTypeListenerTrait.php
   @@ -0,0 +1,241 @@
   +    // We cannot support updating the schema of an entity type from revisionable
   +    // to non-revisionable or translatable to non-translatable because that
   +    // implies data loss.
   +    $convert_rev_to_non_rev = $original->isRevisionable() && !$entity_type->isRevisionable();
   +    $convert_mul_to_non_mul = $original->isTranslatable() && !$entity_type->isTranslatable();
   +    if ($has_data && ($convert_rev_to_non_rev || $convert_mul_to_non_mul)) {
   +      throw new EntityStorageException('Converting an entity type from revisionable to non-revisionable or from translatable to non-translatable is not supported.');
   +    }
   

   We are missing the follow-up links here, setting to NW for these.

5. +++ b/core/modules/system/tests/src/Functional/Entity/Traits/EntityDefinitionTestTrait.php
   @@ -296,4 +297,127 @@ protected function deleteEntityType() {
   +   *   Defaults toFALSE.
   

   typo :)

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldableEntityDefinitionUpdateTest.php
   @@ -0,0 +1,480 @@
   +    $sandbox = [];
   +    $this->entityDefinitionUpdateManager->updateFieldableEntityType($updated_entity_type, $updated_field_storage_definitions, $sandbox);
   

   This is not actually testing multiple steps, even if we create multiple entities exactly to test that case.

Great job, thanks for the patience! :)

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -408,6 +411,182 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
+    // At this point the update process either finished successfully or any
+    // error has been thrown already, so we can drop the backup entity tables.
+    // @todo Decide whether we should keep these tables around.
+    //   @see https://www.drupal.org/project/drupal/issues/3024728
+    foreach ($backup_table_names as $original_table_name => $backup_table_name) {
+      $this->database->schema()->dropTable($backup_table_name);
+    }

As mentioned in #30.15, I'd feel better if by default we left the backup tables around. We have a follow-up to address this, but I'd prefer to have a minimal implementation from the very beginning, so that, if for any reason the follow-up is not ready in time, we err on the safe side. I originally suggested a PHP settings switch allowing to enable backup deletion on success, but I'd be fine even with just removing these lines and improving things in the follow-up. I'm literally terrified by the idea of an update being run on a live site with no backup and triggering a silent data loss issue like #2997960: Missing taxonomy hierarchy items in 8.6.0 after running taxonomy_update_8502. In this case exceptions wouldn't be triggered and thus backup tables would be deleted.

As discussed with @amateescu, we will leave the final word to committers.

Postgres is failing.

The blocker is in, back to RTBC.

Re-uploading #52.

Fixed deprecation errors.

The interdiff above was trivial, back to RTBC.

@committers:

Please, see #53.

Hi everyone,

I'm working on this patch #2829966: Support for Revisions on groups, and the only thing missing is the db upgrade path.

I don't know if I'm doing something wrong but when I try to convert the group entity to revisionable, I'm getting the following error:

Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'status' cannot be null: INSERT INTO {tmp_ec9121groups_field_data} (id, revision_id, type, langcode, status, label, uid, created, changed, default_langcode, revision_translation_affected) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 1
    [:db_insert_placeholder_2] => group_type_1
    [:db_insert_placeholder_3] => en
    [:db_insert_placeholder_4] => 
    [:db_insert_placeholder_5] => Group 1
    [:db_insert_placeholder_6] => 1
    [:db_insert_placeholder_7] => 1548251250
    [:db_insert_placeholder_8] => 1548855959
    [:db_insert_placeholder_9] => 1
    [:db_insert_placeholder_10] => 1
)
 in Drupal\Core\Database\Connection->handleQueryException() (line 689 of /app/webroot/core/lib/Drupal/Core/Database/Connection.php).

And when I add the status field definition, this is the error:

Exception thrown while performing a schema update. MySQL needs the 'status' field specification in order to normalize the 'group__status_type' index

Please, can some of you to provide an example of how to use this new API method.

Thanks.

OK I found it hard to find things to complain about, but managed to find two nits:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlFieldableEntityTypeListenerTrait.php
   @@ -0,0 +1,243 @@
   +
   +    // We cannot support updating the schema of an entity type from revisionable
   +    // to non-revisionable or translatable to non-translatable because that can
   +    // lead to unintended data loss.
   +    // @todo Add support for these conversions in case there is no data loss.
   +    //   @see https://www.drupal.org/project/drupal/issues/3024727
   +    $convert_rev_to_non_rev = $original->isRevisionable() && !$entity_type->isRevisionable();
   +    $convert_mul_to_non_mul = $original->isTranslatable() && !$entity_type->isTranslatable();
   +    if ($has_data && ($convert_rev_to_non_rev || $convert_mul_to_non_mul)) {
   +      throw new EntityStorageException('Converting an entity type from revisionable to non-revisionable or from translatable to non-translatable is not supported.');
   +    }
   +
   +    // Check that the fields required by a revisionable entity type exist.
   +    if ($entity_type->isRevisionable() && !isset($field_storage_definitions[$entity_type->getKey('revision')])) {
   +      throw new EntityStorageException('Missing revision field.');
   +    }
   +    if ($entity_type->isRevisionable() && !isset($field_storage_definitions[$entity_type->getRevisionMetadataKey('revision_default')])) {
   +      throw new EntityStorageException('Missing revision_default field.');
   +    }
   +
   +    // Check that the fields required by a translatable entity type exist.
   +    if ($entity_type->isTranslatable() && !isset($field_storage_definitions[$entity_type->getKey('langcode')])) {
   +      throw new EntityStorageException('Missing langcode field.');
   +    }
   +    if ($entity_type->isTranslatable() && !isset($field_storage_definitions[$entity_type->getKey('default_langcode')])) {
   +      throw new EntityStorageException('Missing default_langcode field.');
   +    }
   +
   +    // Check that the fields required by a revisionable and translatable entity
   +    // type exist.
   +    if ($entity_type->isRevisionable() && $entity_type->isTranslatable() && !isset($field_storage_definitions[$entity_type->getKey('revision_translation_affected')])) {
   +      throw new EntityStorageException('Missing revision_translation_affected field.');
   +    }
   +
   +    // If 'progress' is not set, then this will be the first run of the batch.
   +    if (!isset($sandbox['progress'])) {
   

   Could all the checks happen inside the $sandbox['progress'] check? I can't see a reason we'd need to check them every iteration. It's quite a lot of boilerplate to read when first looking at the function.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/FieldableEntityDefinitionUpdateTest.php
   @@ -0,0 +1,490 @@
   +    $this->assertEntityData($initial_rev, $initial_mul);
   

   Is it worth an extra line to create a new entity against the new storage in this test? i.e. we check the schema, we check the entities migrated, but we don't check that saving a new entity to the new schema is happy yet afaict.

Makes sense to link #2880154: Convert comments to be revisionable

Committed 3148601 and pushed to 8.7.x. Thanks!

Awesome, thanks!

Devel webprofiler busts with this kind of change adding a method to the interface that's not implemented. I'm wondering what that means in contrib for these changes generically, should the contrib maintainer create a 8.7 compatible version?

Class Drupal\webprofiler\Entity\Decorators\Config\ConfigEntityStorageDecorator contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Entity\EntityStorageInterface::restore) in devel/webprofiler/src/Entity/Decorators/Config/ConfigEntityStorageDecorator.php on line 18

That falls within the BC rule of 1-1 class/interface pairs:

Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

https://www.drupal.org/core/d8-bc-policy

Any real entity storage handler will be fine because they extend from EntityStorageBase.

special cases like webprofiler have to update, but adding a new method will not break 8.6 in any way, so just add the method and you're done.

@Berdir, this case it's a decorator, I guess that's the trouble with Decorators. Thanks for the response!

I've added the patch to devel here: #3030614: Drupal\Core\Entity\EntityStorageInterface::restore missing on ConfigEntityStorageDecorator

Would be good to expand the change record to give an example of how to use this to apply changes to field-type schemas