Remove the BC layer for revision metadata keys

+++ b/core/modules/system/system.install
@@ -2495,3 +2495,35 @@ function system_update_8805() {
+      // @todo Figure out if this is enough for removing the protected property
+      //   when the entity type definition is serialized and stored in the last
+      //   installed definitions repository.
+      $entity_type->set('requiredRevisionMetadataKeys', NULL);

I think, that we should completely unset it from the serialized object instead of setting to NULL which is different.

----

Removing all the BC layers from the Entity API is already being taken care of in #3069696: Remove BC layers from the entity system and the patch there has far less failures. However it lacks an update like in the patch here for removing the BC layer property from the installed definitions. I would propose closing this issue as a duplicate and adding the update to the patch there.

Bumping this to critical since it's becoming the 9.0.x fix for #3098427: Manipulating the revision metadata keys before running the BC layer breaks the BC layer. It also blocks #3069696: Remove BC layers from the entity system which in turn blocks other deprecation removals.

The patch in #12 looks encouraging. Does it have to be a hook_update_N() or if it's only removing cruft could it be a hook_post_update_NAME()?

Asking because the longer we can avoid having to deal with #3108658: Handling update path divergence between 11.x and 10.x the better.

Needs a re-roll.

Tagging as a requirement for Drupal 9.0-beta1.

Here is a reroll against 9.0.x, including the stuff from #3098427: Manipulating the revision metadata keys before running the BC layer breaks the BC layer that is still needed. I also changed it to a post update. The fact that we are using post updates to update problems with update version numbers is an interesting fact considering that we're also thinking about switching back to numbers there to to make removing them again easier :)

Removing the expected deprecation messages as we no longer trigger them.

Note: A bit unfortunate that we're doing changes here against the untested workspaces_update_8003(), but we have an issue for that (#3108416: Remove workspace_update_8803()) and #3095333: Extend filled database dump with new stable modules and content for them already adds some test coverage to make sure that it at least doesn't fail in any way.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityType.php
   @@ -119,4 +76,16 @@ public function hasRevisionMetadataKey($key) {
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityTypeInterface.php
   @@ -52,4 +46,17 @@ public function getRevisionMetadataKey($key);
   +   * @return $this
   

   The return $this is missing in the actual implementation of setRevisionMetadataKey().

2. +++ b/core/lib/Drupal/Core/Entity/RevisionLogEntityTrait.php
   @@ -125,11 +125,8 @@ public function setRevisionLogMessage($revision_log_message) {
      protected static function getRevisionMetadataKey(EntityTypeInterface $entity_type, $key) {
   

   I think this method isn't needed anymore?

3. +++ b/core/modules/system/system.post_update.php
   @@ -267,3 +270,33 @@ function system_post_update_entity_reference_autocomplete_match_limit(&$sandbox
   +      $closure = function (ContentEntityTypeInterface $entity_type) {
   +        return get_object_vars($entity_type);
   +      };
   +      $closure = \Closure::bind($closure, NULL, $entity_type);
   ...
   +      $entity_type_definition = $closure($entity_type);
   +      unset($entity_type_definition['requiredRevisionMetadataKeys']);
   +      $entity_type = new ContentEntityType($entity_type_definition);
   

   This works and we can keep it, but it would've been simpler to define a closure which simply does unset($this->requiredRevisionMetadataKeys), instead of creating a new object.

protected static function getRevisionMetadataKey(EntityTypeInterface $entity_type, $key) {
I think this method isn't needed anymore?

What I mean is that this method offers default values for the revision metadata keys if they or some of them are not defined on the entity type.

This is however different than what we do in other similar traits. For example in - EntityOwnerTrait - there we first check whether the entity type has an "owner" entity key and only then return the base field for it.

Now that we remove the BC layer the revision metadata keys, they should be defined on the entity type. Therefore we should also remove the default values from the trait and instead only return revision metadata fields based on the revision metadata keys.

Thanks for the review.

1. Fixed.

2. Good point. I removed all usages of the method, deprecated it for D10 for the unlikely case that someone used it directly and added exceptions if you try to use it with an entity type without the necessary metadata keys.

3. I'm not sure how that would work, so I left it as-is.

Thank you for the changes.

+++ b/core/lib/Drupal/Core/Entity/RevisionLogEntityTrait.php
@@ -125,14 +136,17 @@ public function setRevisionLogMessage($revision_log_message) {
+  /**
+   * Gets the entity type definition.
+   *
+   * @return \Drupal\Core\Entity\ContentEntityTypeInterface
+   *   The content entity type definition.
+   */
+  abstract function getEntityType();

Why was this needed? Doesn't look like a practice of doing it in other traits providing entity fields.

P.S. I understand the idea of doing this, I am just not sure whether it is needed as entity traits are to be used inside entity classes, which always have this method. If this is a new standard for traits, then I am fine with it as it is more explicit anyway, but then we should be consistent and do that for all other traits (for sure in a different issue, if there isn't one already).

P.S.2. If we are about to keep it, then it should match the parent definition and therefore declare the visibility as public explicitly:
abstract public function getEntityType();.

It's there for the return type that is different from the parent. Instead of inline @var in the old method.

+++ b/core/lib/Drupal/Core/Entity/RevisionLogEntityTrait.php
@@ -125,14 +136,17 @@ public function setRevisionLogMessage($revision_log_message) {
+  abstract function getEntityType();

Ok, then let's just explicitly define the visibility modifier - public, in order to match the parent definition explicitly.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityTypeInterface.php
   @@ -52,4 +46,17 @@ public function getRevisionMetadataKey($key);
   +  /**
   +   * Sets a revision metadata key.
   +   *
   +   * @param string $key
   +   *   The name of the entity revision metadata key to set.
   +   * @param string|null $field_name
   +   *   The name of the entity field key to use for the revision metadata key. If
   +   *   NULL is passed, the value of the revision metadata key is unset.
   +   *
   +   * @return $this
   +   */
   +  public function setRevisionMetadataKey($key, $field_name);
   

   Feels very odd to be added this in 9.0.x and not in 8.9.x - ah I knew I was getting deja-vu - #3098427: Manipulating the revision metadata keys before running the BC layer breaks the BC layer adds this to 8.x

2. +++ b/core/modules/system/system.post_update.php
   @@ -285,3 +288,33 @@ function system_post_update_uninstall_simpletest() {
   +/**
   + * Remove backwards-compatibility leftovers from entity type definitions.
   + */
   +function system_post_update_entity_revision_metadata_bc_cleanup() {
   

   Is this tested anywhere?

Test fail was unrelated.

Yay tests and yay @alexpott :)

Could we maybe use property_exists() to check the property instead of the array conversion? According to https://3v4l.org/ugoKX, that works since 5.3 on protected properties.

Also not sure with the @group legacy on the test. Right now we shouldn't need it, but sooner or later, we'll need to add it.

Right, probably because we also remove the property, so it only exists as an dynamic property now, which property_exists() doesn't work with.

I guess this is as good as we can make it then.

Committed 5500590 and pushed to 9.0.x. Thanks!