Add a revisionable entity base class and log interface

+++ b/core/lib/Drupal/Core/Entity/EntityRevisionLogInterface.php
@@ -0,0 +1,90 @@
+interface EntityRevisionLogInterface {

If entity is part of it the interface name, shouldn't this extend EntityInterface?

Otherwise (if it's generic), shouldn't this be called RevisionableInterface?

Then again, we already have \Drupal\Core\Entity\RevisionableInterface. So, should this then be RevisionableWithMetadataInterface extends RevisionableInterface?

it is just one with additional features

That is exactly like saying NewInterface extends ExistingInterface.

This is consistent with EntityOwnerInterface, EntityChangedInterface and others. 1+ to not extend. extends EntityInterface actually wouldn't help much since most are content entities

I did not ask for extends EntityInterface (which I agree would not make much sense). I am asking for extends RevisionableInterface.

I'm asking for that because the interface being introduced here makes no sense on its own. It only makes sense in combination with the already existing RevisionableInterface.

interface EntityRevisionLogInterface {
  public function getRevisionCreationTime();
  public function setRevisionCreationTime($timestamp);
  public function getRevisionUser();
  public function setRevisionUser($account);
  public function getRevisionUserId();
  public function setRevisionUserId($user_id);
  public function getRevisionLogMessage();
  public function setRevisionLogMessage($revision_log_message);
}

interface RevisionableInterface {
  public function isNewRevision();
  public function setNewRevision($value = TRUE);
  public function getRevisionId();
  public function isDefaultRevision($new_value = NULL);
  public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record);
}

How does EntityRevisionLogInterface make sense without a getRevisionId() method (necessary to load a revision) or a setNewRevision() method (necessary to call before calling the new setRevisionLogMessage() or setRevisionUserId() can make sense)?

Sorry for not being more clear. I think "entity" shouldn't be part of the name. For the same reasons you and Berdir state: because it's not extending EntityInterface. Because RevisionableInterface is not extending EntityInterface. Because none of this is entity-specific.

1. +++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
   @@ -0,0 +1,91 @@
   +namespace Drupal\Core\Entity;
   +use Drupal\user\UserInterface;
   

   Super nit: don't we usually have a newline between these?

2. +++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
   @@ -0,0 +1,91 @@
   + * Defines an entity type with create time/author/log information for revisions.
   

   Comment doesn't really make sense.

3. +++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
   @@ -0,0 +1,91 @@
   +  public function getRevisionCreationTime();
   ...
   +  public function getRevisionUser();
   

   These aren't really about logging, right?

4. +++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
   @@ -0,0 +1,91 @@
   +  public function getRevisionLogMessage();
   

   Only this one is.

   I wonder if RichRevisionableInterface or something like that is better. But that feels even more weird. So I think "log" is ok. Would be great if others could chime in on that.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php
   @@ -0,0 +1,62 @@
   +  }
   +
   +
   +}
   

   Super nit: two newlines, should be one.

Hm, on second thought…

+++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
@@ -0,0 +1,91 @@
+  public function getRevisionUser();
...
+  public function setRevisionUser(UserInterface $account);
...
+  public function getRevisionUserId();
...
+  public function setRevisionUserId($user_id);

What if we split this off into RevisionOwnerInterface extends RevisionsableInterface.

(revision-level interface to match the entity-level \Drupal\user\EntityOwnerInterface)

Then you have two interfaces: one for a revision owner, and one for a log message + creation time. It feels like that's a step in the right direction.

Finally:

+++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
@@ -0,0 +1,91 @@
+  public function getRevisionCreationTime();
...
+  public function setRevisionCreationTime($timestamp);

"creation"

but NodeInterface uses "created".

Good points.

But doesn't that mean that all of this really should be merged into RevisionableInterface? I understand that's not possible for BC reasons. But if that's where we want to go eventually, then the docs should say that, and should document that as a @todo for Drupal 9. If there are good reasons for them to be separate, we should document that rationale on the interface.

That's all I care about: something that makes sense in the larger set of interfaces, and that has documentation explaining its rationale for posterity.

That's fine. Let's just document that rationale on this new interface. That's all I'm saying :)

+++ b/core/lib/Drupal/Core/Entity/RevisionableContentEntityBase.php
@@ -0,0 +1,113 @@
+abstract class RevisionableContentEntityBase extends ContentEntityBase implements RevisionLogInterface {

This new content entity base class is not very helpful when you want to add revision log capability to an existing entity class.

How about providing a trait instead?

Sorry, didn't mean to change the status.

@amateescu
You extend the new base class, instead of adding the trait? You are changing your class either way.

The reason why we went with a base class is because "revisionable" is one of those fundamental behaviors that really deserves one, instead of multiple traits.

My point was that you can not extend both the new base class *and* the original entity class :)

Edit: for example when the original entity class contains a lot of specific logic/code that you don't want to duplicate.

I still don't see the use case. You extended ContentEntityBase before, now you extend RevisionableContentEntityBase.

Swapping an existing entity type's class to add additional methods is probably not a use case we need to concern ourselves with?

Swapping an existing entity type's class to add additional methods is probably not a use case we need to concern ourselves with?

That's exactly what Multiversion has to do, swap all content entity classes in order to add extra revision functionality, so my opinion is that core can and should be a bit more helpful in this regard.

Here's what I had in mind.

How is the normal case harder? IMO both cases are covered by default with that approach.

I'd agree with amateescu - adding revision support to some other module seems to be something very realistic. The trait needs to document it's "implementing" the interface though and cannot use @inheritdoc but must point
to the interface methods the "old-way" with a FQDN method reference.

I wanted to point you to an example, but I did not find any. There were all removed by #2502621: Replace implement notes with inheritdoc tag - but that was wrong imo, so I re-opened that issue.

I still think this is a wrong workaround, but sure people can disagree. It seems to be that we are trying to make the normal case harder and the special case easier. Not sure this is the right strategy.

We can have both. Just make a base class which uses the trait.
Edit: i.e, add RevisionableContentEntityBase that has the trait + just adds the field definitions in via a regular method override. That's not worse than having it directly in the base class imo.

Note that the patch in #33 still has the new RevisionableContentEntityBase class, but it's empty and just uses the trait. However, overriding baseFieldDefinitions() in this class is not needed if we keep the code added to ContentEntityBase::baseFieldDefinitions().

+++ b/core/lib/Drupal/Core/Entity/RevisionLogInterface.php
@@ -0,0 +1,92 @@
+   * Gets the entity revision creation timestamp.
+   *
+   * @return int|NULL
+   *   The UNIX timestamp of when this revision was created. Return NULL if the
+   *   entity type does not support revision create time.
+   */
+  public function getRevisionCreationTime();
...
+   * Gets the entity revision author.
+   *
+   * @return \Drupal\user\UserInterface|NULL
+   *   The user entity for the revision author. Return NULL if the entity type
+   *   doesn't support revision authors.
+   */
+  public function getRevisionUser();

I was also wondering about these two. Since our base field definitions include revision_created and revision_user by default, is there really a way for these methods to return NULL?

If so, then the code in the trait has to be adjusted in that regard and check for the existence of the fields before trying to get their value.

Also, should it rather be "revision owner" instead of "revision user", in order to be consistent with EntityOwnerInterface?

+    // Add the base fields provided by the revisionable log entity trait.
+    $traits_used = class_uses(get_called_class());
+    if ($traits_used && isset($traits_used['Drupal\Core\Entity\RevisionLogEntityTrait'])) {
+      $fields += static::revisionLogBaseFieldDefinitions($entity_type);
+    }
+

This is a hack.
Let our new base class call the trait method, instead of doing black magic.

Well, it seemed to be that this hack even never worked.

It is working, I tested it manually with the Term entity class.

the point was that we have the interface but don't require people to implement every method.

I don't see how that would work, every method from an interface needs to have an implementation. And the current implementation of getRevisionUser() and getRevisionCreationTime() would not return NULL but throw an exception if those fields are not there.

This is a hack.
Let our new base class call the trait method, instead of doing black magic.

I disagree :) Any other opinions?

Discussed with @bojanz and @dawehner on IRC and I'll drop my argument for the "hack". In this regard, #45 is fine, but we still need to fix #41..

Ok then, fixed those return types and the docs for the trait as well. The patch looks ready to me.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Should we make Node and BlockContent implement the new interface? Maybe in #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table.

#55: Closely related: #2678492: Convert the node revision converter to the generic entity one.

Should we rename the issue to clarify that we introduced an entity base class as well (RevisionableContentEntityBase)? That's the important part for most people.

This issue is missing a change record. (We should add change records for noteworthy API additions.)