Add test coverage for uninstalling revisionable entity types whose code doesn't exist anymore

This looks great to me!

I was initially concerned about switching from live definitions to active ones under the hood, but then I checked all the invocations of the ::getRevisionMetadataKeys() method and it seems that the only places that would actually require the live definitions (EntityFieldManager::buildBaseFieldDefinitions() and RevisionLogEntityTrait::revisionLogBaseFieldDefinitions()) are using the flag to skip fetching definitions to avoid recursion. This seems a strong indicator that the change is definitely in the right direction.

This change might affect contrib/custom update code relying ::getRevisionMetadataKeys(), as we saw with system_update_8400(), but updates should always rely on active definitions to be predictable, so this change would actually allow to surface sneaky update path bugs.

I have only a minor remark:

+++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
@@ -60,6 +60,11 @@ public function testSystemUpdate8400() {
+      // Only run the following assertions for the test entity types.

I assume that the reason here is that we only have test data for test entity types, but let's make this comment more explicit.

Great, thanks!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -86,7 +86,7 @@ public function getRevisionMetadataKeys($include_backwards_compatibility_field_n
-      $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
+      $base_fields = \Drupal::service('entity_field.manager')->getActiveFieldStorageDefinitions($this->id());

This issue is currently assigned to 8.7.x, not 8.8.x. It's fixing a major bug, so it's reasonable to want that in 8.7.x, but are we sure this is a safe change to make in a patch release?

+++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
@@ -38,7 +38,7 @@ public function setDatabaseDumpFiles() {
-    foreach (['entity_test_revlog', 'entity_test_mul_revlog'] as $entity_type_id) {
+    foreach (['block_content', 'entity_test_revlog', 'entity_test_mul_revlog'] as $entity_type_id) {

@@ -60,6 +60,12 @@ public function testSystemUpdate8400() {
+      // The database dump contains data just for the test entity types, so the
+      // following assertions can only run for them.
+      if (!in_array($entity_type_id, ['entity_test_revlog', 'entity_test_mul_revlog'], TRUE)) {
+        continue;
+      }

By adding and later excluding block_content we only want to ensure, that its revision metadata keys are placed in the correct tables. This is a good addition, however it makes me think why don't we do that check for all entity types in the system instead of only for three of them?

--------

Found the problem! block_content_update_8003() must run before system_update_8400(), otherwise the revision_created and revision_user fields added by block_content_update_8003() are not moved to the revision table.

This means, that if in custom/contrib there is a similar update it should necessary run before system_update_8400(), right? Otherwise an inconsistent state might be the side effect of the change.

--------

This issue is currently assigned to 8.7.x, not 8.8.x. It's fixing a major bug, so it's reasonable to want that in 8.7.x, but are we sure this is a safe change to make in a patch release?

I've checked all the places in core, where we retrieve either a single revision metadata key (which always uses the BC layer) or all revision metadata keys by explicitly requesting the BC layer.

Now with the patch in system_update_8400() we will be retrieving the live base field definitions, but we will extract the revision metadata keys through the BC layer based on the active base field definitions. I don't see a problem with this beside that we might find a revision metadata key, which does not exist in the live base field definitions, but only in the active ones. However this does not represent an issue, because we do an intersect. If we are to change the update it will be only because of consistency. I've just wanted to share this with you, so that you can consider such use cases and think if there might be some tricky edge cases.

I, personally, support the change and cannot think of any problems that it could cause beside that updates adding revision metadata keys now should necessary run before system_update_8400(). Maybe we could add a post update, in which we check that everything went well and all revision metadata keys are placed in the correct tables? If not, then inform that something went wrong? Or as an alternative maybe we could check in \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::installFieldStorageDefinition() if we are installing a revision metadata key/field after system_update_8400() has been executed and check if the fields are placed in the correct tables?

--------

After having written all that, I don't really understand why if block_content_update_8003() runs after system_update_8400() the revision metadata fields will land in the wrong tables? What happens if somebody installs Drupal 8.8.0 and adds a similar update as block_content_update_8003()?

Is it because in \Drupal\Core\Entity\Sql\DefaultTableMapping::create() with the patch applied we retrieve the revision metadata keys from the active instead from the live base field definitions?

What about controlling by a flag or something similar whether to retrieve the live or the active field storage definitions for the BC layer? When an entity type is to be uninstalled then we don't care about the live field storage definitions. Maybe we could simply check if the entity class exist - not a beautiful solution, but a one that would work without any breaks I think.

We might have to do this fallback in the EntityFieldManager so that no matter how one is about to access the field storage definitions of a missing but not yet uninstalled entity type then the active field storage definitions will be returned. Wouldn't such a general solution solve even more potential problems for a missing entity type?

More testing the better.

Requeued tests against 10.x, if they pass I don't see why this can't go in.

Agreed with the extra assertions so +1 RTBC. Committed/pushed to 10.1.x and cherry-picked back to 9.5.x, thanks!