Manipulating the revision metadata keys before running the BC layer breaks the BC layer

+++ b/core/modules/workspaces/workspaces.install
@@ -139,8 +139,19 @@ function workspaces_update_8801() {
+        $required_revision_metadata_keys = $entity_type->get('requiredRevisionMetadataKeys');
+        $required_revision_metadata_keys['workspace'] = 'workspace';

The requiredRevisionMetadataKeys property was added in #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key for non-optional fields provided by the Entity API (like revision_default), but the new workspace revision metadata key is very much optional.

Silly me, thinking I could search for 8.8.x issues to monitor 8.8.0. :)

Bumping to critical.

@hchonov: Can you explain why your solution will work and can you add testing.

Short review and I have 2 points:

1. +++ b/core/modules/system/tests/src/Functional/Entity/Update/MoveRevisionMetadataFieldsUpdateTest.php
   @@ -202,6 +202,46 @@ public function testAddingRequiredRevisionMetadataKeys() {
   +    $definition = [
   +      'id' => 'entity_test_mul_revlog',
   +    ];
   +    $entity_type = new ContentEntityType($definition);
   +
   +    $revision_metadata_keys = $entity_type->get('revision_metadata_keys');
   +    $revision_metadata_keys['new_revision_metadata_key'] = 'new_revision_metadata_key';
   +    $entity_type->set('revision_metadata_keys', $revision_metadata_keys);
   +
   

   We can remove this part of the test. It is double.

2. The added test only test the setting and getting of metakeys. Can you add testing for when a BC layer get (not) triggered. I we now remove the fix from the patch the new tests will still pass.

The fix in this patch looks good to me.
The added tests look good too. Only they do not test the proposed fix.
The patch with only the added tests passes without the fix.
I have chatted with @hchonov and he states that:

it is not possible to add a failing test for the workspaces problem without changing the order of the updates, which can be achieved only by adding a new update. and this isn’t something that will be accepted.

I leave it to the committer if this is acceptable (possibly remove the added tests).

I'd like to take a look at this.

There are two things to discuss here:

1) If a fix is needed for the upgrade path tests, then the patch attached which switches that test to a newer db fixture should be enough.

The current update tests from the Workspaces module cover a situation that can not exist: a fixture of Drupal 8.0.0 with the Workspaces module installed, when the module was only added in 8.6.0.

The only theoretical update failure scenario would be if a site updates directly from 8.3.x to 8.8.x, but, in that case, the Workspaces module couldn't have been installed.

2) If a fix is needed to cover the current issue title ("Manipulating the revision metadata keys before running the BC layer breaks the BC layer"), then I don't think #22 is good for the following reasons:

#9 says

This is how new keys should be added. Otherwise the BC layer is broken.

This is not documented *anywhere* (the doxygen block for $requiredRevisionMetadataKeys only says something about new vs. cached instances of the entity type class), and it's a terrible DX if it's truly the only way to add a revision metadata key.

The current solution for providing the BC layer of revision metadata keys (using $requiredRevisionMetadataKeys) was committed in #2936511: ContentEntityType::getRevisionMetadataKeys() never considers the BC logic because of the new "revision_default" key and it is largely based on comment #18 from that issue:

Yes, but if a developer has defined one the keys, then it is her/his responsibility of providing all the 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.

This doesn't take into account the possibility of other core/contrib/custom modules providing revision metadata keys for entity types that they don't own/control (i.e. what Workspaces does in 8.8.0), and forces them to code specifically for the BC layer.

We just have never considered allowing additional revision metadata keys beside the ones defined by core.

Yes, that's the conclusion of #25 as well, see the last paragraph.

What workspaces has already done was to "hack" core to make that possible, which is why it was updating the property directly instead through a dedicated API and this isn't documented as supported anywhere.

What do you mean by "hacking" core? There is no dedicated API for adding a revision metadata key to an entity type definition, like there is no dedicated API for adding a regular entity key, it's all done through the generic \Drupal\Core\Entity\EntityType::set() method and it has been like that since 8.0.0.

I'd be ok with #22 only if we add a ContentEntityType::setRevisionMetadataKey() method, because Workspaces (or any other module) should not have to know or code around the internal implementation of a BC layer.

But, as explained in #25, that patch should be enough because a site needs to be on Drupal 8.6.0 in order to be able to install Workspaces, and at that point the BC layer has done its job already.

We can mark the new helper method as @internal for now, which keeps it "unsupported" as far as core's public API policy is concerned.

@hchonov asked me to think about whether #22 or #25 plus a new helper is the right way to go.

I'm inclined to agree with @hchonov that a new helper method (even marked @internal) to support a BC layer that's not going to be in Drupal 9 feels unnecessary.

But also if the changes to \Drupal\workspaces\EntityTypeInfo::entityTypeBuild() are necessary to fix the live definitions and the live definitions are currently wrong on 8.8.x then we should test this.

So I guess what I'm saying is that I think we should merge #22 and #25 to fix the update test so that it is updating from a more real point and fix the live definitions and the live workspace definition should tested top prove that we've fixed it.

Also I'm not sure that this is critical bug anymore. Like is there a situation where the update can fail?

The new helper method would not be added specifically for the BC layer, it will be helpful even in 9.x and beyond for code that wants to add a revision metadata key to an entity type definition. It's just that in this case, it will help with not spilling the internal implementation of that BC layer to other places that have nothing to do with it.

I guess the proposal was not clear enough, so here's a pseudo-implementation of this helper:

8.8.x and 8.9.x:

public function setRevisionMetadataKey($key, $field_name) {
  $this->revision_metadata_keys[$key] = $field_name;
  // Only needed in 8.x because of the BC layer.
  $this->requiredRevisionMetadataKeys[$key] = $field_name;
}

9.x and beyond:

public function setRevisionMetadataKey($key, $field_name) {
  $this->revision_metadata_keys[$key] = $field_name;
}

\Drupal\Core\Entity\EntityType has many helper setters like this: setHandlerClass(), setStorageClass(), setFormClass(), setListBuilderClass(), setViewBuilderClass(), etc., so it's not like we would do something novel here..

#34 is quite compelling - how about we file a follow-up for that? At the moment the only other use case in core is system_update_8501() which we're about to delete and this code. So I think we'd need to envisage further usages or be able to point to contrib that's getting it current wrong to help provide a basis for adding the new API.

+++ b/core/modules/workspaces/workspaces.post_update.php
@@ -141,6 +141,20 @@ function workspaces_post_update_move_association_data(&$sandbox) {
+    // Delete the entity and field schema data.
+    $keys = [
+      'workspace_association.entity_schema_data',
+      'workspace_association.field_schema_data.id',
+      'workspace_association.field_schema_data.revision_id',
+      'workspace_association.field_schema_data.uuid',
+      'workspace_association.field_schema_data.revision_default',
+      'workspace_association.field_schema_data.target_entity_id',
+      'workspace_association.field_schema_data.target_entity_revision_id',
+      'workspace_association.field_schema_data.target_entity_type_id',
+      'workspace_association.field_schema_data.workspace',
+    ];
+    \Drupal::keyValue('entity.storage_schema.sql')->deleteMultiple($keys);

AS this update is already in 8.8.0 I think this needs to be in a separate update no?

+++ b/core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php
@@ -25,7 +25,7 @@ class WorkspacesUpdateTest extends UpdatePathTestBase {
-      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.filled.standard.php.gz',
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',

Is the filled-ness important?

The new test doesn't fail because its namespace declaration is wrong. Here's the minimal test-only patch.

WorkspaceRevisionMetadataFieldTest fails because the drupal-8.2.0.bare.standard_with_entity_test_revlog_enabled.php.gz db fixture doesn't contain the installed definition for the new entity_test_mul_revlog_pub entity type.

@alexpott, re #36: we already have an additional use case in contrib, in the Revision Tree module, and another one coming up in the Trash module.

This means there are at least 7 places (3 in core - this patch, 4 in contrib) where we have to repeat BC-aware code, which in turn means that those contrib modules can not have a release that's compatible with both Drupal 8.x and 9.x at the same time (the whole point of having BC layers), so I really think that we need to add the helper setter method in this issue.

Yes, the move tests still fail. I didn't update the fixture to include the new entity type, I just pointed out why it fails.

The full patch from #39 wasn't generated with --binary, so here's a better one. Leaving at NW because the fixture needs to be updated.

@alexpott, what do you think about #39, should we do it here or in a new issue?

@amateescu you've convinced me...

we already have an additional use case in contrib, in the Revision Tree module, and another one coming up in the Trash module.

Let's do this here. It's a minimal thing and does make things easier for everyone. The fact we have all these other use-cases is the reason I've changed my mind.

Thanks, @alexpott :)

Updated the patch to add the helper method, fixed the failing tests by installing the new entity type in an update function and moved the workspace_association cleanup to workspaces_update_8803(), which was just introduced by #3099986: Move part of workspaces_post_update_move_association_data() to a hook_update_N and hasn't been part of a published release yet.

Patch does not apply.

@Meenakshi.g: Could you also post an interdiff.txt file. Such a file makes it easier for reviewers to see what you have changed in your new patch file. See: https://www.drupal.org/documentation/git/interdiff.

@daffie patch was not working because it cannot apply the binary patch to without full index line so there is no difference in interdiff

Oops, I forgot that this patch needs to be diffed with --binary :/ This patch is #41 with the interdiff from #44 and rolled properly.

Fixed those fails.

I think that all the reviews and open questions have been addressed, so the patch looks ready for another round of committer feedback.

Testbot failure.

Testbot is back to green now.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
@@ -119,4 +119,18 @@ public function hasRevisionMetadataKey($key) {
+    $this->requiredRevisionMetadataKeys[$key] = $field_name;

So the documentation in ContentEntityType directly contradicts what is going on here. It says:

   * This property should only be filled in the constructor. This ensures that
   * only new instances get newly added required revision metadata keys.
   * Unserialized objects will only retrieve the keys that they already have
   * been cached with.

So we need to update that documentation as to why this is okay.

Also all the revision metadata key logic could do with a unit test as we're about to remove update paths in 8.9.x and 9.0.x so we might endup with stuff being completely untested.

Thanks for taking another look @alexpott :)

Regarding the first point from #58, I think the documentation for that class property is bogus because #9 says that it can (and should) be filled from the outside when adding a new revision metadata key. Also, given that it's basically repeating a small part of the inline documentation from the constructor, let's remove it.

Regarding the unit test, keep in mind that this BC layer also goes away in 9.0.x (#3099789: Remove the BC layer for revision metadata keys), so, in that issue, the new method added here will become a simple setter, which shouldn't require any special testing IMO:

  public function setRevisionMetadataKey($key, $field_name) {
    $this->revision_metadata_keys[$key] = $field_name;
  }

Does system_modules_uninstalled()need to remove the required key too?

And what about \Drupal\workspaces\EventSubscriber\EntitySchemaSubscriber::onEntityTypeUpdate() and \Drupal\workspaces\EventSubscriber\EntitySchemaSubscriber::addRevisionMetadataField() do they need to deal with the required metadata key?

Great point! This should do it.

Fixed those fails and added test coverage for the problem that @alexpott discovered in #60.

This doesn't apply to 9.0.x anymore now, I'm not quite sure what we need to do now exactly as the upgrade path there has been removed, do we still need to commit some of this there? I suppose so...

NW for #64.

+++ b/core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php
@@ -25,7 +25,7 @@ class WorkspacesUpdateTest extends UpdatePathTestBase {
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',

We have to change this to 8.8.0 and then change drupal-8.6.0-workspaces_installed.php to drupal-8.8.0-workspaces_installed.php as well which we added in #3087644-50: Remove Drupal 8 updates up to and including 88**

I don't think it's necessary to update the test coverage as such, we can probably just do separate 9.0.x and 8.9.x patches:

9.0.x with the runtime changes and entity_test_revlog + associated test coverage.

8.9.x also including the workspaces update test.

I was thinking the same, maybe we could just directly use #3099789: Remove the BC layer for revision metadata keys for the 9.0.x issue, as that was then already removing a good chunk of this again I believe.

Tagging as a requirement for Drupal 9.0-beta1.

Setting this back to RTBC, as my understanding is that this is now only for D8 and #3099789: Remove the BC layer for revision metadata keys is the standalone companion issue for D9. Might want to wait until the D9 issue is also ready so we can commit them together though?

Random fail...

Would be good to commit this and #3099789: Remove the BC layer for revision metadata keys close to each other yes.

I looked at the patch again and noticed that we were missing a return statement for the new setRevisionMetadataKey() method. Fixed that quickly.

+++ b/core/modules/workspaces/workspaces.install
@@ -193,5 +196,19 @@ function workspaces_update_8803() {
+    // Delete the entity and field schema data.
+    $keys = [
+      'workspace_association.entity_schema_data',
+      'workspace_association.field_schema_data.id',
+      'workspace_association.field_schema_data.revision_id',
+      'workspace_association.field_schema_data.uuid',
+      'workspace_association.field_schema_data.revision_default',
+      'workspace_association.field_schema_data.target_entity_id',
+      'workspace_association.field_schema_data.target_entity_revision_id',
+      'workspace_association.field_schema_data.target_entity_type_id',
+      'workspace_association.field_schema_data.workspace',
+    ];
+    \Drupal::keyValue('entity.storage_schema.sql')->deleteMultiple($keys);

As we're changing updates that have already been released don't we need to add an 8804 update to do this?

That's an unfortunate crosspost.

I read back through the issue history:

@alexpott asked for a new update in #3098427-36: Manipulating the revision metadata keys before running the BC layer breaks the BC layer

@hchonov split it out in #3098427-37: Manipulating the revision metadata keys before running the BC layer breaks the BC layer

In #3098427-44: Manipulating the revision metadata keys before running the BC layer breaks the BC layer @amateescu correctly pointed out that the original update hadn't been in a tagged release yet, so moved it back inline.

However two months has passed, and there's now been a tagged release.

So... I think we need to add the hook_post_update_NAME() back, and commit that to 9.x, 8.9.x, and 8.8.x - the post update can stay in 9.x for workspaces users that already updated to Drupal 8.8.2.

Since this requires a 9.0.x commit, seems simplest to do it in a follow-up patch but remaining on this issue.

Really this was an unrelated problem with the workspaces update that we could have split out to a follow-up, although did not really hurt to include it here given it's touching the same code, except for update path/tagged release race condition.

Whoa, that's really unfortunate indeed. Let's do a quick followup patch here to fix 8.9.x and 8.8.x, and I'll open another issue to move this code to a post-update for 9.0.x as well.

The post update is missing from #78.

I think we can commit the 9.0.x patch here too.

Okay, let's do everything here then :)

Thanks patches look right.

Committed c02ff06 and pushed to 9.0.x. Thanks!

Committed and pushed f94693e842 to 8.9.x and 6fa0f86b39 to 8.8.x. Thanks!

#78 proves we have test coverage.