ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key

Because #2891215: Add a way to track whether a revision was default when originally created passed all core tests it feels as though we don't have suitable test coverage for this issue. To prevent future regression of this issue I think we should add a new test, and a test-only patch in this issue.

Before #2891215-57: Add a way to track whether a revision was default when originally created I was adopting a similar approach to what this patch is doing (counting keys), however @gabesullice (IMO) correctly pointed out that that's not a particularly solid approach. Can't we just explicitly check the keys?

    if (
      $include_backwards_compatibility_field_names &&
      !isset($this->revision_metadata_keys['revision_user']) &&
      !isset($this->revision_metadata_keys['revision_created']) &&
      !isset($this->revision_metadata_keys['revision_log_message'])  
    ) {
      // ...
    }

The easiest way to do that would be to have at least one (test or non-test) entity type that relies on the BC layer on purpose. Then that would break. But that will also trigger BC warnings.

I was about to suggest exactly that: why are BC warnings a problem? Are you concerned about keeping "deprecated" code around?

I guess we could explicitly check the keys, yes. I kinda dislike long multi-line if statements though :)

Me too :)

A separate boolean variable to be checked in the if test would work for me.

[...] but turns out that we already have them there.

Yep, that's why I was confused by #6.

+1 on entity_test_revlog or a new dedicated entity test type.

RevisionableContentEntityBaseTest does not fail, it just happily creates the fields in the wrong table then and is consistently inconsistent and nothing explicitly fails.

Do you mean without the fix?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -59,19 +59,25 @@ protected function checkStorageClass($class) {
   -    if (!$this->revision_metadata_keys && $include_backwards_compatibility_field_names) {
   +    if ($include_backwards_compatibility_field_names) {
          $base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());
   

   I think we were trying not to do this to avoid retrieving base fields. Not sure how much it buys, though.

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -224,7 +224,9 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
   -      $field_name = $entity_type->getRevisionMetadataKey('revision_default');
   +      // Disable the BC layer to prevent a recursion, this only needs the
   +      // revision_default key that is always set.
   +      $field_name = $entity_type->getRevisionMetadataKeys(FALSE)['revision_default'];
   

   Is this still needed?

Looks good, assuming testbots agree!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -59,7 +59,8 @@ protected function checkStorageClass($class) {
+    $needs_bc = !isset($this->revision_metadata_keys['revision_user']) && !isset($this->revision_metadata_keys['revision_timestamp']) && !isset($this->revision_metadata_keys['revision_log_message']);

With this condition we'll execute the BC logic on each method call for the case where an entity type doesn't have any of those fields. If we want to prevent this we would have to use another property e.g. $this->needsBC = TRUE; to store the information whether the BC logic has been already executed.

$needs_bc = $this->needsBC && !isset($this->revision_metadata_keys['revision_user']) && !isset($this->revision_metadata_keys['revision_timestamp']) && !isset($this->revision_metadata_keys['revision_log_message']);
and in the if statement we set that property to FALSE.

At the same time, the previous code would never have called been called if only the revision_user key would have been defined?

Yes, but if a developer has defined one the keys, then it is her/his responsibility of providing all the keys. We've added the BC layer only because we cannot force the developers to set the revision metadata keys in the entity annotation until D9. Therefore it is fine to skip running the BC logic if one of the keys has been defined in the entity annotation, because this means the developer is aware of those keys.

Actually it is even wrong to run the BC logic if at least one the revision metadata keys has been defined, because the intention might have been to not consider the other fields as revision metadata keys and skip all the internal storage logic that we already have for them. Changing this might really break sites, as the storage will suddenly return completely different structure. I am sorry, but we have to restore the previous behavior - putting back the status to "needs work" for this.

A bit worried about side effects of introducing more properties in those objects as they are persisted in entity definition manager and so on.

We are constantly introducing new properties on classes, the objects of which are persisted. I don't see any problems with doing this. But beside that I think that we persist only the definition i.e. the annotation of an entity, not the entity type object itself.

I think the easiest solution will be to not define the revision_default key in the constructor, but retrieve it through the BC layer like we are retrieving the other revision metadata keys.

I think it's highly unlikely that an entity type sets other revision metadata keys but not those three

That it is "unlikely" from our POV doesn't mean that it is unlikely from the POV of others. I think that it is not good, that we make decisions, because according to our opinion something is unlikely or really a rare use case. If we could solve it without having to break the BC then we should solve it like this, which means that we should handle the "revision_default" revision metadata key just like the other ones. The places where it is required have to check if it exist and if not hard code its name - until the update has been performed - and in the BC layer we just check if the field is provided in the base field definitions with the default name we use.

This might not be a beautiful solution, but it is how we currently deal with the other revision metadata keys and I think this is the only option we have until D9, where we'll require that the revision metadata keys are defined in the entity type annotation.

This is not possible as far as I see for a combination of reasons, it has be be more or less exactly as it is now:

In the patch I am posting I've implemented it with the BC layer to show exactly what I mean and it is possible :).

If we don't want that the places needing the field name have to hard code it themselves then we could introduce a new method name ContentEntityTypeInterface::getRevisionMetadataDefaultKeyName(), which will simply return the default field name.

In the patch that I've provided it is not specified by any entity type, but added automatically.

What is the problem with it being inside the BC logic? We can remove the error, it is actually not needed as it is us who add the metadata key.

@hchonov, @Berdir explained why it shouldn't be inside the BC logic in #23 and I agree with that comment.

It is not only in the BC logic but also in the constructor to cover the two cases where the revision metadata keys are provided / not provided in the entity type annotation. When the BC logic is removed then we'll just define it in the constructor without the condition that we currently have.

Sorry but I still don't understand what exactly is the problem with such an approach?

Is the warning that is causing the confusion? I am sorry about this, my bad for adding it. I've removed the warning. This patch doesn't force any entity type to define the "revision_default" key and when we remove the BC layer the key will be just defined in the constructor like now, but we will simply remove the condition that we currently have in the constructor.

The file "content_entity_type_d9.txt" shows how the entity type will look like in D9.

+++ b/core/modules/system/system.install
@@ -2078,7 +2078,7 @@ function system_update_8501() {
-    $field_name = $entity_type->getRevisionMetadataKey('revision_default');
+    $field_name = $entity_type->hasRevisionMetadataKey('revision_default') ? $entity_type->getRevisionMetadataKey('revision_default') : 'revision_default';

This is the problem with this approach. The system should *always* have a revision_default metadata key defined, either by default or specified in the entity annotation.

This is the problem with this approach. The system should *always* have a revision_default metadata key defined, either by default or specified in the entity annotation.

Like mentioned in Slack it is not possible because of the BC layer we've provided which relies on the revision_metadata_keys property being empty.

Here is the another approach where we need to make the BC layer a bit more complex but this way the "revision_default" key should always be returned by new instances of the class and never returned by unserialized objects prior to the update to cover the case where the entity type objects are cached in the EntityLastInstalledSchemaRepository.

@berdir will probably not be happy with this approach as he stated in #17:

A bit worried about side effects of introducing more properties in those objects as they are persisted in entity definition manager and so on.

FWIW this is what @Berdir was referring to in the last paragraph of the previous comment:

https://gist.github.com/plach79/dda896dad256d507939ebd31e1558b0a

Spent some more time discussing this with Berdir in Slack. I suggested an adjusted version of #33 (see https://gist.github.com/plach79/f22ceca27c8d071464f46e46d5769f24) while he suggested a simplified version that cannot be fixed the way I was suggesting (see https://gist.github.com/Berdir/410dc15cc4ff06aa2675503c7d500fa6).

I'm personally fine with both, but if we need to support these edge cases properly, we also need some test coverage, otherwise we will keep breaking stuff. I think that having a new test entity type defining only the revision_default key + some assertions around the generated schema should be enough.

Doesn't your own argument work against you here?

If I explicitly add revision_metadata_keys = ['revision_default' => 'revision_default'] to my entity type, then it will still call the BC logic.

Good catch... I had another solution which I tried to simplify and didn't notice that I've introduced that behavior for this key. But the fix for this is pretty simple - we should not add the key to the new property if it is already provided in the annotation :). This way the condition ($this->revision_metadata_keys === $this->requiredRevisionMetadataKeys) will evaluate to FALSE because the property $this->revision_metadata_keys will not be empty and will not equal the empty property $this->requiredRevisionMetadataKeys.

I'm OK with #33 too but I think the property for that is overkill for the single required property that we have no, we can always go back to that if we end up with different ones.

The property has an idea and it is to always store the state of the last version which covers cached entity type objects. If we remove the property the next time we introduce a new key the condition in the getter method will have to be defined like this:

$previous_required_revision_metadata_keys = [
  'revision_default' => 'revision_default'
];
$new_required_revision_metadata_keys = [
  'revision_default' => 'revision_default',
  'new_metadata_key' => 'new_metadata_key'
];
if ((!$this->revision_metadata_keys || ($this->revision_metadata_keys === $previous_required_revision_metadata_keys) || ($this->revision_metadata_keys === $new_required_revision_metadata_keys)) && $include_backwards_compatibility_field_names) {

where ($this->revision_metadata_keys === $previous_required_revision_metadata_keys) will match entity type objects that have been cached and ($this->revision_metadata_keys === $new_required_revision_metadata_keys) will match new instances.

To prevent such complex updates and additions to the required revision metadata keys we have to keep the property.

Should we merge the changes from this issue into the parent one, which has been reverted?

Let's keep things simple and keep working here, please :)

Did we reach consensus on how to move forward? It seems to me that #37 is an alternative to fix #32 that is similar in principle to what I was suggesting.

@hchonov:

Can we try that and add test coverage for the use cases it support?

Isn't this enough to fix the BC layer?

Can we try that and add test coverage for the use cases it support?

@plach, I'll write a test later today.

@amateescu, unfortunately this is not enough, because we don't want to execute the BC layer if the entity type definition ships with already set revision metadata keys.
@see #18:

Actually it is even wrong to run the BC logic if at least one the revision metadata keys has been defined, because the intention might have been to not consider the other fields as revision metadata keys and skip all the internal storage logic that we already have for them. Changing this might really break sites, as the storage will suddenly return completely different structure.

It was only reverted from 8.5.x so that it's not in the alpha. AFAIK it will be committed again as-is once that is released.

Just confirming that this is correct. This issue's "version" field is set to 8.6.x and so can proceed, since #2891215: Add a way to track whether a revision was default when originally created is already in 8.6, and so the patch in this issue can be committed to 8.6 regardless of when 8.5 alpha is tagged. Once 8.5 alpha is tagged, I'll cherry pick that issue back into 8.5, as well as this one when it's ready.

And here is the test and the fix so that we are able always to detect if the revision metadata fields have been defined in the annotation or not.

I'll have a look ASAP, thanks!

This looks great to me, thanks a lot for the expanded test coverage!

I have only a few minor nits:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -25,9 +36,23 @@ public function __construct($definition) {
   +    // have been provided by the entity type annotation therefore we add keys to
   
   +++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
   @@ -79,4 +80,128 @@ public function testSystemUpdate8400() {
   +    // have been provided by the entity type annotation therefore we add keys to
   

   Superminor: can we add a comma before "therefore" in both cases? :)

2. +++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
   @@ -79,4 +80,128 @@ public function testSystemUpdate8400() {
   +    // Object Type: \Drupal\Tests\system\Functional\Entity\Update\TestRevisionMetadataBcLayerEntityType
   

   These are a bit confusing at first sight: do we need them given that the objects are type-hinted below?

3. +++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
   @@ -79,4 +80,128 @@ public function testSystemUpdate8400() {
   +    $cached_with_no_metadata_keys = 'Tzo4MjoiRHJ1cGFsXFRlc3RzXHN5c3RlbVxGdW5jdGlvbmFsXEVudGl0eVxVcGRhdGVcVGVzdFJldmlzaW9uTWV0YWRhdGFCY0xheWVyRW50aXR5VHlwZSI6Mzk6e3M6MjU6IgAqAHJldmlzaW9uX21ldGFkYXRhX2tleXMiO2E6MDp7fXM6MzE6IgAqAHJlcXVpcmVkUmV2aXNpb25NZXRhZGF0YUtleXMiO2E6MDp7fXM6MTU6IgAqAHN0YXRpY19jYWNoZSI7YjoxO3M6MTU6IgAqAHJlbmRlcl9jYWNoZSI7YjoxO3M6MTk6IgAqAHBlcnNpc3RlbnRfY2FjaGUiO2I6MTtzOjE0OiIAKgBlbnRpdHlfa2V5cyI7YTo1OntzOjg6InJldmlzaW9uIjtzOjA6IiI7czo2OiJidW5kbGUiO3M6MDoiIjtzOjg6Imxhbmdjb2RlIjtzOjA6IiI7czoxNjoiZGVmYXVsdF9sYW5nY29kZSI7czoxNjoiZGVmYXVsdF9sYW5nY29kZSI7czoyOToicmV2aXNpb25fdHJhbnNsYXRpb25fYWZmZWN0ZWQiO3M6Mjk6InJldmlzaW9uX3RyYW5zbGF0aW9uX2FmZmVjdGVkIjt9czo1OiIAKgBpZCI7czo0OiJ0ZXN0IjtzOjE2OiIAKgBvcmlnaW5hbENsYXNzIjtOO3M6MTE6IgAqAGhhbmRsZXJzIjthOjM6e3M6NjoiYWNjZXNzIjtzOjQ1OiJEcnVwYWxcQ29yZVxFbnRpdHlcRW50aXR5QWNjZXNzQ29udHJvbEhhbmRsZXIiO3M6Nzoic3RvcmFnZSI7czo0NjoiRHJ1cGFsXENvcmVcRW50aXR5XFNxbFxTcWxDb250ZW50RW50aXR5U3RvcmFnZSI7czoxMjoidmlld19idWlsZGVyIjtzOjM2OiJEcnVwYWxcQ29yZVxFbnRpdHlcRW50aXR5Vmlld0J1aWxkZXIiO31zOjE5OiIAKgBhZG1pbl9wZXJtaXNzaW9uIjtOO3M6MjU6IgAqAHBlcm1pc3Npb25fZ3JhbnVsYXJpdHkiO3M6MTE6ImVudGl0eV90eXBlIjtzOjg6IgAqAGxpbmtzIjthOjA6e31zOjE3OiIAKgBsYWJlbF9jYWxsYmFjayI7TjtzOjIxOiIAKgBidW5kbGVfZW50aXR5X3R5cGUiO047czoxMjoiACoAYnVuZGxlX29mIjtOO3M6MTU6IgAqAGJ1bmRsZV9sYWJlbCI7TjtzOjEzOiIAKgBiYXNlX3RhYmxlIjtOO3M6MjI6IgAqAHJldmlzaW9uX2RhdGFfdGFibGUiO047czoxNzoiACoAcmV2aXNpb25fdGFibGUiO047czoxMzoiACoAZGF0YV90YWJsZSI7TjtzOjE1OiIAKgB0cmFuc2xhdGFibGUiO2I6MDtzOjE5OiIAKgBzaG93X3JldmlzaW9uX3VpIjtiOjA7czo4OiIAKgBsYWJlbCI7czowOiIiO3M6MTk6IgAqAGxhYmVsX2NvbGxlY3Rpb24iO3M6MDoiIjtzOjE3OiIAKgBsYWJlbF9zaW5ndWxhciI7czowOiIiO3M6MTU6IgAqAGxhYmVsX3BsdXJhbCI7czowOiIiO3M6MTQ6IgAqAGxhYmVsX2NvdW50IjthOjA6e31zOjE1OiIAKgB1cmlfY2FsbGJhY2siO047czo4OiIAKgBncm91cCI7TjtzOjE0OiIAKgBncm91cF9sYWJlbCI7TjtzOjIyOiIAKgBmaWVsZF91aV9iYXNlX3JvdXRlIjtOO3M6MjY6IgAqAGNvbW1vbl9yZWZlcmVuY2VfdGFyZ2V0IjtiOjA7czoyMjoiACoAbGlzdF9jYWNoZV9jb250ZXh0cyI7YTowOnt9czoxODoiACoAbGlzdF9jYWNoZV90YWdzIjthOjE6e2k6MDtzOjk6InRlc3RfbGlzdCI7fXM6MTQ6IgAqAGNvbnN0cmFpbnRzIjthOjA6e31zOjEzOiIAKgBhZGRpdGlvbmFsIjthOjA6e31zOjg6IgAqAGNsYXNzIjtOO3M6MTE6IgAqAHByb3ZpZGVyIjtOO3M6MjA6IgAqAHN0cmluZ1RyYW5zbGF0aW9uIjtOO30=';
   +    /** @var \Drupal\Tests\system\Functional\Entity\Update\TestRevisionMetadataBcLayerEntityType $entity_type */
   +    $entity_type = unserialize(base64_decode($cached_with_no_metadata_keys));
   +    $required_revision_metadata_keys = [];
   +    $this->assertEquals($required_revision_metadata_keys, $entity_type->getRevisionMetadataKeys(FALSE));
   ...
   +    $cached_with_metadata_key_revision_default = 'Tzo4MjoiRHJ1cGFsXFRlc3RzXHN5c3RlbVxGdW5jdGlvbmFsXEVudGl0eVxVcGRhdGVcVGVzdFJldmlzaW9uTWV0YWRhdGFCY0xheWVyRW50aXR5VHlwZSI6Mzk6e3M6MjU6IgAqAHJldmlzaW9uX21ldGFkYXRhX2tleXMiO2E6MTp7czoxNjoicmV2aXNpb25fZGVmYXVsdCI7czoxNjoicmV2aXNpb25fZGVmYXVsdCI7fXM6MzE6IgAqAHJlcXVpcmVkUmV2aXNpb25NZXRhZGF0YUtleXMiO2E6MTp7czoxNjoicmV2aXNpb25fZGVmYXVsdCI7czoxNjoicmV2aXNpb25fZGVmYXVsdCI7fXM6MTU6IgAqAHN0YXRpY19jYWNoZSI7YjoxO3M6MTU6IgAqAHJlbmRlcl9jYWNoZSI7YjoxO3M6MTk6IgAqAHBlcnNpc3RlbnRfY2FjaGUiO2I6MTtzOjE0OiIAKgBlbnRpdHlfa2V5cyI7YTo1OntzOjg6InJldmlzaW9uIjtzOjA6IiI7czo2OiJidW5kbGUiO3M6MDoiIjtzOjg6Imxhbmdjb2RlIjtzOjA6IiI7czoxNjoiZGVmYXVsdF9sYW5nY29kZSI7czoxNjoiZGVmYXVsdF9sYW5nY29kZSI7czoyOToicmV2aXNpb25fdHJhbnNsYXRpb25fYWZmZWN0ZWQiO3M6Mjk6InJldmlzaW9uX3RyYW5zbGF0aW9uX2FmZmVjdGVkIjt9czo1OiIAKgBpZCI7czo0OiJ0ZXN0IjtzOjE2OiIAKgBvcmlnaW5hbENsYXNzIjtOO3M6MTE6IgAqAGhhbmRsZXJzIjthOjM6e3M6NjoiYWNjZXNzIjtzOjQ1OiJEcnVwYWxcQ29yZVxFbnRpdHlcRW50aXR5QWNjZXNzQ29udHJvbEhhbmRsZXIiO3M6Nzoic3RvcmFnZSI7czo0NjoiRHJ1cGFsXENvcmVcRW50aXR5XFNxbFxTcWxDb250ZW50RW50aXR5U3RvcmFnZSI7czoxMjoidmlld19idWlsZGVyIjtzOjM2OiJEcnVwYWxcQ29yZVxFbnRpdHlcRW50aXR5Vmlld0J1aWxkZXIiO31zOjE5OiIAKgBhZG1pbl9wZXJtaXNzaW9uIjtOO3M6MjU6IgAqAHBlcm1pc3Npb25fZ3JhbnVsYXJpdHkiO3M6MTE6ImVudGl0eV90eXBlIjtzOjg6IgAqAGxpbmtzIjthOjA6e31zOjE3OiIAKgBsYWJlbF9jYWxsYmFjayI7TjtzOjIxOiIAKgBidW5kbGVfZW50aXR5X3R5cGUiO047czoxMjoiACoAYnVuZGxlX29mIjtOO3M6MTU6IgAqAGJ1bmRsZV9sYWJlbCI7TjtzOjEzOiIAKgBiYXNlX3RhYmxlIjtOO3M6MjI6IgAqAHJldmlzaW9uX2RhdGFfdGFibGUiO047czoxNzoiACoAcmV2aXNpb25fdGFibGUiO047czoxMzoiACoAZGF0YV90YWJsZSI7TjtzOjE1OiIAKgB0cmFuc2xhdGFibGUiO2I6MDtzOjE5OiIAKgBzaG93X3JldmlzaW9uX3VpIjtiOjA7czo4OiIAKgBsYWJlbCI7czowOiIiO3M6MTk6IgAqAGxhYmVsX2NvbGxlY3Rpb24iO3M6MDoiIjtzOjE3OiIAKgBsYWJlbF9zaW5ndWxhciI7czowOiIiO3M6MTU6IgAqAGxhYmVsX3BsdXJhbCI7czowOiIiO3M6MTQ6IgAqAGxhYmVsX2NvdW50IjthOjA6e31zOjE1OiIAKgB1cmlfY2FsbGJhY2siO047czo4OiIAKgBncm91cCI7TjtzOjE0OiIAKgBncm91cF9sYWJlbCI7TjtzOjIyOiIAKgBmaWVsZF91aV9iYXNlX3JvdXRlIjtOO3M6MjY6IgAqAGNvbW1vbl9yZWZlcmVuY2VfdGFyZ2V0IjtiOjA7czoyMjoiACoAbGlzdF9jYWNoZV9jb250ZXh0cyI7YTowOnt9czoxODoiACoAbGlzdF9jYWNoZV90YWdzIjthOjE6e2k6MDtzOjk6InRlc3RfbGlzdCI7fXM6MTQ6IgAqAGNvbnN0cmFpbnRzIjthOjA6e31zOjEzOiIAKgBhZGRpdGlvbmFsIjthOjA6e31zOjg6IgAqAGNsYXNzIjtOO3M6MTE6IgAqAHByb3ZpZGVyIjtOO3M6MjA6IgAqAHN0cmluZ1RyYW5zbGF0aW9uIjtOO30=';
   +    $entity_type = unserialize(base64_decode($cached_with_metadata_key_revision_default));
   +    $required_revision_metadata_keys = [
   +      'revision_default' => 'revision_default',
   +    ];
   +    $this->assertEquals($required_revision_metadata_keys, $entity_type->getRevisionMetadataKeys(FALSE));
   ...
   +    $entity_type = new TestRevisionMetadataBcLayerEntityType(['id' => 'test']);
   +    $required_revision_metadata_keys = [
   +      'revision_default' => 'revision_default',
   +      'second_required_key' => 'second_required_key',
   +    ];
   +    $this->assertEquals($required_revision_metadata_keys, $entity_type->getRevisionMetadataKeys(FALSE));
   

   Can we also add an assertion for the expected behavior with the BC-layer enabled?

   Btw, why are we base64-encoding the cached item? Wouldn't it be easier to troubleshoot this code in the future, if we just had the serialized string?

#49:

1. Done.
2. Done.

3. Can we also add an assertion for the expected behavior with the BC-layer enabled?

   I don't know this well enough to be sure what you're asking here. Can you clarify?

   Btw, why are we base64-encoding the cached item? Wouldn't it be easier to troubleshoot this code in the future, if we just had the serialized string?

   I tried, but we can't get rid of this AFAICT:

       $no_metadata_keys = new TestRevisionMetadataBcLayerEntityType(['id' => 'a', 'revision_metadata_keys' => []]);
       $this->assertEquals([], $no_metadata_keys->getRevisionMetadataKeys(FALSE));
   

   fails with:

   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
    Array (
   +    'second_required_key' => 'second_required_key'
   +    'revision_default' => 'revision_default'
    )
   

   and

       $with_metadata_key_revision_default = new TestRevisionMetadataBcLayerEntityType(['id' => 'a', 'revision_metadata_keys' => ['revision_default' => 'revision_default']]);
       $this->assertEquals(['revision_default' => 'revision_default'], $with_metadata_key_revision_default->getRevisionMetadataKeys(FALSE));
   

   fails with:

   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
    Array (
        'revision_default' => 'revision_default'
   +    'second_required_key' => 'second_required_key'
    )

#50's interdiff was wrong, and the filename even indicated that.

Thank you for everyone's diligent work on this. Just a reminder that 8.5 beta is next week, which will be the first tagged release since #2891215: Add a way to track whether a revision was default when originally created landed. Any help with final reviews and testing would be very much appreciated, so that ideally beta testers updating existing (alpha1 or earlier) sites can do so without getting those databases into a broken state. Thanks!

FYI: #2941733: block_content_revision loses columns due to incorrect entity definition was recently opened with additional information relevant to this.

Is this the same problem?

@Wim Leers, #50:

I don't know this well enough to be sure what you're asking here. Can you clarify?

I meant that we should add an invocation of ::getRevisionMetadataKeys(TRUE) and verify that the returned value is correct.

@mpdonadio, #56:

That test does not look right to me: it is not running updates. Loading an entity before updates have run may indeed lead to executing a broken query.

#57: I see. But we can't do that for \Drupal\Tests\system\Functional\Entity\Update\TestRevisionMetadataBcLayerEntityType, since it's not an actual entity type and hence this:

$base_fields = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($this->id());

in \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys() will fail. How do you propose we do that then?

Ok, makes sense, let's ignore my suggestion. #50 seems ready to go to me, I will RTBC it tomorrow morning, unless @Berdir has any concern.

RTBC patch is #50.

Sorry, I am currently working on an updated patch :). Will post it in a minute.

I am sorry for the delay.

Actually it is possible to add the test case where we check the BC layer of the serialized entity type. TestRevisionMetadataBcLayerEntityType is just the class, but the entity type it represents is defined through the "id" property. Therefore if the id represents a real entity type, then the entity field manager will be able to provide the base field definitions and so properly execute the BC layer.
When I've created that serialized entity type initially I used "test" for the id property, which doesn't represent any entity type. In the current patch I've exchanged it with the entity type "entity_test_mul_revlog", for which we've removed the revision metadata keys from the annotation so that we could test the BC layer.

@plach, now we have the additional test with the BC layer, that you wished :).

@plach, I had to use base64 encoding instead of simply providing the string output from serialize(), as I was unable of unserializing it. I've found the idea to use base64 encoding for such cases in https://stackoverflow.com/questions/10152904/unserialize-function-unseri... . Is there a better way of doing this?

@berdir pointed me to a new issue regarding this subject, but I was unable of reproducing it - #2941733: block_content_revision loses columns due to incorrect entity definition and I am not sure if it should prevent the current one from getting in?

I've just noticed, that I've uploaded the interdiff as a patch ... I'll be back on my computer in 40 minutes and will upload the whole patch. I am sorry for this ...

Thanks!

@berdir pointed me to a new issue regarding this subject, but I was unable of reproducing it - #2941733: block_content_revision loses columns due to incorrect entity definition with patch 2936511 and I am not sure if it should prevent the current one from getting in?

Well, unless it's directly causing that issue I think this shouldn't be blocked, I also tried to reproduce that issue without luck.

Here is the patch my comment #62 was about.

P.S. It looks like I should not upload any patches anymore today :). I have no idea how I've managed it to upload the patch and the interdiff twice.

After talking with @berdir about #2941733: block_content_revision loses columns due to incorrect entity definition, I've realized that with the new BC logic the update hook for adding the revision_default revision metadata key isn't complete. I've extended the patch and I am curious if somehow this is solving the referenced issue.

Tests are green and I also manually tested the upgrade from 8.4 to 8.5 once again and it seems to be working fine.

I get a couple of "The entity type block_content does not have a "published" entity key." notices, but it's the same in HEAD, so I think this is ready to go now.

Created #2942533: "The entity type block_content does not have a "published" entity key." notices when updating from 8.4 to 8.5 for the notices issue.

Yep, I agree those seem to be unrelated.

#67 looks like it might resolve #2941733: block_content_revision loses columns due to incorrect entity definition with patch 2936511. Trying it now.

@berdir @hchonov @plach #67 resolves the loss of the columns reported on #2941733: block_content_revision loses columns due to incorrect entity definition with patch 2936511. I apologize I did not realize that it was really the same issue and keep all the history here in this patch discussion. Thanks for all your hard work on this one. Please feel free to close out 2941733 as a dup.

Thanks for reporting, glad we were able to sort this out. It would be bad if we discovered it post commit ;)

Yay, I am happy that #67 is resolving your issue, @sdewitt.

We should add a test for the interdiff in #67, which is resolving the referenced issue. We'll certainly need the same test the next time we add a new required revision metadata key.

Good point

Any thoughts on how we could reproduce the problem in a test?

No idea, honestly, I was never able to reproduce that problem. We could try to break the logic we had in place before the latest iteration and see whether that helps reproducing the issue. Worst case we would get implicit test coverage.

Re #56 and #57, that wasn't a real test. It was just the minimum amount of code to show the problem (the query barfing). The test I was working on was an update test that

- use the filled fixture (drupal-8.filled.standard.php.gz)
- loaded one of the test nodes
- checked the field value
- ran updates (which does a field value change)
- reloaded the node
- checked the field value to see if the update worked.

The problem (for me at least) means that drupal-8.filled.standard.php.gz won't work (others may have this too, didn't check). I can make my own fixture; just wanted to use what was there. Rebuilding it may (or may not) be worthy of a followup.

Committers: please remove my credit on this; my file on this issue didn't add any value to the patch.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -59,7 +84,7 @@ protected function checkStorageClass($class) {
   -    if (!$this->revision_metadata_keys && $include_backwards_compatibility_field_names) {
   +    if ((!$this->revision_metadata_keys || ($this->revision_metadata_keys === $this->requiredRevisionMetadataKeys)) && $include_backwards_compatibility_field_names) {
   

   Are we sure we want a strict (===) comparison here? Aren't there pathways by which they might not be in the same order? For example, if there ends up being a 2nd required one, added in a child class and therefore defined before 'revision_default', but an update function adds it to the last installed definition after 'revision_default' was already added by system_update_8501()?

2. +++ b/core/modules/system/system.install
   @@ -2092,9 +2092,22 @@ function system_update_8501() {
   +        // Update the revision metadata keys to add the new required revision
   +        // metadata key "revision_default".
   +        $revision_metadata_keys += $required_revision_metadata_keys;
   

   This line of code is doing more than the comment says, depending on what else happens to be in $required_revision_metadata_keys besides 'revision_default'. I think it would be best to limit the scope of this update function to only 'revision_default', and leave future required keys to the update functions of the patches/modules that introduce them. Or is there a reason we need/want to merge in the entire required array here?

3. +++ b/core/modules/system/system.install
   @@ -2092,9 +2092,22 @@ function system_update_8501() {
   +      if (!isset($revision_metadata_keys['revision_default'])) {
   +        // Update the property holding the required revision metadata keys,
   +        // which is used by the BC layer for retrieving the revision metadata
   +        // keys.
   +        // @see \Drupal\Core\Entity\ContentEntityType::getRevisionMetadataKeys().
   +        $required_revision_metadata_keys = $installed_entity_type->get('requiredRevisionMetadataKeys') + ['revision_default' => $field_name];
   +        $installed_entity_type->set('requiredRevisionMetadataKeys', $required_revision_metadata_keys);
   +
   +        // Update the revision metadata keys to add the new required revision
   +        // metadata key "revision_default".
   +        $revision_metadata_keys += $required_revision_metadata_keys;
   +        $installed_entity_type->set('revision_metadata_keys', $revision_metadata_keys);
   +
   +        $definition_update_manager->updateEntityType($installed_entity_type);
   +      }
   

   This entire block only runs when there isn't already an installed field that happens to be named 'revision_default'. But then we're in a situation where in such a case, the metadata keys end up being different between the last installed definition and the runtime code one. I think it would make more sense to run this block outside of the if (!$definition_update_manager->getFieldStorageDefinition($field_name, $entity_type_id)) { statement that's above it, since nothing in here needs to distinguish between whether that field already exists or not.

4. We should add a test for the interdiff in #67...Any thoughts on how we could reproduce the problem in a test?

   If I understand correctly (please correct me if I don't), the problem prior to #67 was that system_update_8501() ran in such a way that following it, $installed_entity_type->getRevisionMetadataKeys() would not support the BC of 'revision_user' etc. And in #2941733: block_content_revision loses columns due to incorrect entity definition that manifested when block_content_update_8400() ran after system_update_8501(). In which case, can we expand EntityUpdateAddRevisionDefaultTest::testAddingTheRevisionDefaultField() to check that following the updates, that $installed_entity_type->get('requiredRevisionMetadataKeys') and $installed_entity_type->get('revision_metadata_keys') are what we expect and/or that $installed_entity_type->getRevisionMetadataKeys() returns what we expect? I think that would be sufficient test coverage, or am I missing something?

I found several issues with the comments. This patch doesn't address #82, but it addresses my comment nits (seemed easier to directly make the changes than post a review).

While I wouldn't block this patch going in on it, because we already have the data loss in HEAD, I think we should have actual upgrade path tests that expose this issue (i.e. with an entity type that requires this bc layer, with some content in the db for it).

What about committing this as soon as #82 is addressed and trying to improve test coverage as a follow-up?

Working on #82.

#84/#10 is what I asked for indeed, I just didn't realise it was already covered, so fine with any additional coverage we can add being a follow-up.

@effulgentsia, #82:

1: Seems a good fix to me. I manually checked the update once again and requiredRevisionMetadataKeys is initialized with an array even when the parent object is being unserialized, so I don't think we are at risk of matching a NULL with an empty array, which would be the only unintended scenario allowed by this change.
2: Fixed
3: I think the current logic is correct, imagine the following scenario:

1. We are updating an entity type that does have an existing (unrelated) revision_default field;
2. if system_update_8501() runs while the codebase has not been updated to deal with the new revision_default revision metadata key, the latter will get a default value of revision_default, thus matching the existing field;
3. at this point the current logic will skip processing for this entity type id entirely;
4. at the end of the process you get a warning in the status report telling you you need to update your code or things will not work correctly;
5. you install an updated version of the provider module, which implements an update function as suggested at https://www.drupal.org/node/2936349 (just updated it), and everything works fine;
6. if the updated module were already installed, system_update_8501() would do all the work and the provider module update would return early, but the end result would be correct also in this case.

If instead we go the way you suggest, we will be missing the warning in step 4, which I don't think would be correct.
4: Yep, that's what I meant in #79. Implemented that. Attaching also a failing patch.

@effulgentsia, #82.3

This entire block only runs when there isn't already an installed field that happens to be named 'revision_default'.

If this happens then this is bad, really bad, as we rely on the field not being there. If an entity type already defines a field with such a name, we cannot simply reuse it and pretend it is the core field. How do we handle this? We cannot simply throw an exception, but probably log an error? I think we should start defining core reserved field names and not allow custom/contrib using them.

P.S. I've just seen the response of @plach about this in #88. Then I guess everything is correct.

P.S.2 @plach, but I think that it would be good to add an else branch and log an error pointing to the CR.

This should address #89.

Looks good to me as well. Thanks!

Removing credit from @mpdonadio per request in #80. Adding credit to @sdewitt for the testing in #2941733: block_content_revision loses columns due to incorrect entity definition that led to this patch's improvement.

Thank you to everyone for helping resolve this critical regression prior to beta! Pushed to 8.6.x and 8.5.x!

Hi, I updated from 8.4.4 to 8.5.0beta and i'm getting this error, i checked the patch it is present, i've been struggling trying to find how to resolve this.

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Database\DatabaseExceptionWrapper</em>: SQLSTATE[42S22]: Column not found: 1054 Unknown column &#039;revision.revision_default&#039; in &#039;field list&#039;: SELECT revision.vid AS vid, revision.langcode AS langcode, revision.revision_user AS revision_user, revision.revision_created AS revision_created, revision.revision_log_message AS revision_log_message, revision.revision_default AS revision_default, base.mid AS mid, base.bundle AS bundle, base.uuid AS uuid, CASE base.vid WHEN revision.vid THEN 1 ELSE 0 END AS isDefaultRevision
FROM 
{media} base
INNER JOIN {media_revision} revision ON revision.vid = base.vid
WHERE base.mid IN (:db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] =&gt; 1
)
 in <em class="placeholder">Drupal\Core\Entity\Sql\SqlContentEntityStorage-&gt;getFromStorage()</em> (line <em class="placeholder">455</em> of <em class="placeholder">core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php</em>).

Hm even if the update hasn't been executed and the field isn't present then why would the storage assume it is there?

@Berdir Yes I did check all the updates and even tried to restor databse to 8.4.4 and update again to 8.5.0beta.
I fixed this by adding a column "revision_default" in table 'media_revision'.

But now i'm getting this error after I followed upgrade path for Media which is now implemented in core. But I get this error when i try to add a Media. I tried recreating my own media type but i still get the same error:

<em class="placeholder">Drupal\Component\Plugin\Exception\PluginNotFoundException</em>: The &quot;media_bundle&quot; entity type does not exist. in <em class="placeholder">Drupal\Core\Entity\EntityTypeManager-&gt;getDefinition()</em> (line <em class="placeholder">133</em> of <em class="placeholder">core/lib/Drupal/Core/Entity/EntityTypeManager.php</em>

Any idea if this might come from the previous error i mentioned above ?

@KarimBou could you open a new, critical, issue for this report?

Referring to my problem as described in

https://www.drupal.org/project/drupal/issues/2956936

I am still trying to fix this.

At this moment I get these errors:

An existing "Default revision" field was found for the Aangepast blok[error]
entity type, but no "revision_default" revision metadata key was
found in its definition.
An existing "Default revision" field was found for the Inhoud entity [error]
type, but no "revision_default" revision metadata key was found in
its definition.

Since I see this is mentioned (solved!?) in this thread , can anyone tell me what to do (I am lost...) Thanks!

@BlinX, these errors occurred during the update, right? Did you update your site through drush updb or through the Drupal UI?

@hchonov, I first attempted a few times by Drupal UI, then after reading severall related posts I learned to install/use Drush and tryed drush updb and drush entup also few times.
Although this happened last time:
Site is in D8.4.8, drush updb to D8.5.3 did not work.
So I used Installatron to upgrade to D8.5.3 and after that did drush updb and drush entup.
I saw a lot (good things I believe) happening in Drush, but also the Errors I mentioned before...