block_content_revision loses columns due to incorrect entity definition

I had a look through the history of EntityDefinitionUpdateManager.php. It looks like getEntityUpdate() was introduced in this commit: http://cgit.drupalcode.org/drupal/commit/core/lib/Drupal/Core/Entity/Ent... for this issue: https://www.drupal.org/project/drupal/issues/2542748. Having a read through the issue solution.

Actually, I am not sure my previous theory makes sense. If the `revision_metadata_keys` were correct on the last installed definition, the `clone` operation should not affect that since during object cloning properties are shallow copied. So, it may be that the installed definitions are somehow getting corrupted? Will continue to investigate.

I wrote this small snippet to execute with `drush scr` and interestingly enough it looks like the `revision_metadata_keys` contributed by the annotation are not present in the last installed definition. The `node` entity type shows the same thing. Only the `revision_default shows` up in the last installed definition.

 echo "INSPECTING LAST INSTALLED SCHEMA DEFINITION...\n";
 $lastInstalledSchemaRepo = \Drupal::service('entity.last_installed_schema.repository');
 $bcDefn = $lastInstalledSchemaRepo->getLastInstalledDefinition('block_content');
 var_dump($bcDefn);

Is this expected behavior?

It is unclear to me that these are the same issues. Feel free to dup this one out if you think it's the same issue.

Also, that issue had many different patches and possible approaches and yours is not the most recent one anymore.

Not entirely sure what you mean here. Can you clarify?

@berdir it does happen without that patch from #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key. The reason we initially tried the patch from 2936511 is because it was suggested it might fix the issue.

Actually, as I stated in comment #7 and #8, I think my initial analysis was incorrect. I don't think it has anything to do with the `clone` and the fact that the revision_default metadata key was moved to the constructor in the patch. That wouldn't make sense. What seems to be more likely an issue is that the revision_user/etc keys are not part of the lastInstalledRevision. Maybe this is intentional? I saw some comments on #2936511 about this and not sure if they confirm this. However, if this *is* intentional, the code in the EntityDefinitionUpdateManager#getEntityType() somehow needs to fill in the actual revision metadata keys from the annotation definitions for the entity type before it passes it on down the line on the onEntityTypeUpdate call b/c as it is, this is not done and that is how we end up with the schema corruption.

@berdir Thanks for your continued help and attention on this one.

system_update_8501 is *absolutely* running before block_content_update_8400 every time on our installation. I have tried a lot of different things as I've been trying to debug this and I do believe that last week I did try manually running 8400 before 8501 and in that case everything was fine. Although, I did not check the last installed defn at that time but Drupal at least did operate normally. I did not save a copy of the output from running the updb but I did it about 20 times last week and I can guarantee you 8501 always ran before 8400.

I developed a small snippet I could run with drush scr to show me the result of using reflection to look at the internal revision_metadata_keys of the ContentEntityType returned from the getLastInstalledDefinition() and also to look at the getRevisionMetadataKeys() from the same ContentEntityType. I have attached the script to the issue. Because of your last comment about not just block_content being affected, I ran it for node, block_content, taxonomy_term, and user. The results before we run system_update_8501() show the following. NOTE: I did the below WITHOUT the patch https://www.drupal.org/project/drupal/issues/2936511 installed.

(The first array is the result of using reflection and the second the result of getRevisionMetadataKeys()).

INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR node...
/var/www/ibm/files-private/test-block-content.php:14:
array(0) {
}
/var/www/ibm/files-private/test-block-content.php:15:
array(3) {
  'revision_user' =>
  string(13) "revision_user"
  'revision_created' =>
  string(16) "revision_created"
  'revision_log_message' =>
  string(12) "revision_log"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR block_content...
/var/www/ibm/files-private/test-block-content.php:14:
array(0) {
}
/var/www/ibm/files-private/test-block-content.php:15:
array(3) {
  'revision_user' =>
  string(13) "revision_user"
  'revision_created' =>
  string(16) "revision_created"
  'revision_log_message' =>
  string(12) "revision_log"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR taxonomy_term...
/var/www/ibm/files-private/test-block-content.php:14:
array(0) {
}
/var/www/ibm/files-private/test-block-content.php:15:
array(3) {
  'revision_user' =>
  string(13) "revision_user"
  'revision_created' =>
  string(16) "revision_created"
  'revision_log_message' =>
  string(12) "revision_log"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR user...
/var/www/ibm/files-private/test-block-content.php:14:
array(0) {
}
/var/www/ibm/files-private/test-block-content.php:15:
array(3) {
  'revision_user' =>
  string(13) "revision_user"
  'revision_created' =>
  string(16) "revision_created"
  'revision_log_message' =>
  string(12) "revision_log"
}

Then I ran system_update_8501() and then again ran the script. Here is the output.

INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR node...
/var/www/ibm/files-private/test-block-content.php:14:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
/var/www/ibm/files-private/test-block-content.php:15:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR block_content...
/var/www/ibm/files-private/test-block-content.php:14:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
/var/www/ibm/files-private/test-block-content.php:15:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR taxonomy_term...
/var/www/ibm/files-private/test-block-content.php:14:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
/var/www/ibm/files-private/test-block-content.php:15:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
INSPECTING LAST INSTALLED SCHEMA DEFINITION FOR user...
/var/www/ibm/files-private/test-block-content.php:14:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}
/var/www/ibm/files-private/test-block-content.php:15:
array(1) {
  'revision_default' =>
  string(16) "revision_default"
}

So for all the types I queried, they do seem to have lost the revision_user, etc keys.

At this point, I have two questions.

1. I presume that if I ran EntityDefinitionUpdateManager#updateEntityType() on any of these entity types, I would end up losing the revision_user, etc columns in the respective revision tables (block_content_revision, node_revision, etc). Right?
2. Could this be that the state of our last installed definitions is the problem? For example, if I look at the 8.4.x definition of Drupal\node\Entity\Node, it does have the revision_user, etc keys in its revision_metadata_keys property of its @ContentEntityType annotation. Because of this, shouldn't the last installed definition at some point in 8.4 or earlier persisted the serialized ContentEntityType with these keys in revision_metadata_keys?

#67 on #2936511 ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key looks like it might resolve #2941733: block_content_revision loses columns due to incorrect entity definition with patch 2936511. Trying it now.

#67 on #2936511 ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key resolves it. We can close this as a dup.