Automatically set revision user/log information/created time on entity revisions

The thing is that the owner thing is IMHO wrong, see #2376999: Non-existent node author user IDs are silently converted to 0 (?) (without validation errors). So I think that should not be done by default.

Added the revision log author and also the preSaveRevision() part, but that's bit tricky as well, as we need the method from the trait to get the field name (unless we want to duplicate that logic) but overriding methods like that from a trait is tricky, so I put it on EditorialContentEntityBase.

Moving an assertion for media from #2880199: Revision user not set when saving media.

@dawehner I'm not sure, I think fields having business logic that essentially spans across multiple fields is tricky

Reroll since the patch no longer applied, no changes..

This looks great to me.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -375,6 +376,12 @@ public function preSave(EntityStorageInterface $storage) {
+    // If no revision author has been set explicitly, make the node owner the

We shouldn't be referencing node in ContentEntityBase in my opinion

Can we get a change record here too

I'm not really sure we need a CR here because I don't think there's any action to take by a module developer after this change..

Fixed #10.

@dawehner, ok, do you think this helps? https://www.drupal.org/node/2908984

+++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
@@ -28,4 +28,19 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+  public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record) {
+    parent::preSaveRevision($storage, $record);
+    $revision_log_field_name = static::getRevisionMetadataKey($this->getEntityType(), 'revision_log_message');
+    if (!$this->isNewRevision() && isset($this->original) && (!isset($record->$revision_log_field_name) || $record->$revision_log_field_name === '')) {
+      // If we are updating an existing node without adding a new revision, we
+      // need to make sure $entity->revision_log is reset whenever it is empty.
+      // Therefore, this code allows us to avoid clobbering an existing log
+      // entry with an empty one.
+      $record->$revision_log_field_name = $this->original->$revision_log_field_name->value;
+    }
+  }

The issue summary is about revision user, not log messages - is this out of scope?

Also, I note BlockContent does the same dance, so if this is now part of its parent class, that dance can be removed too - assuming scope is correct.

BlockContent didn't extend from this yet when this was RTBC'd, but yes it can be included now as well :)

Also updating title to include revision log, now it's in scope I'd say ;)

Updated the patch.

Adding some of the changes from #2897026: \Drupal\media\Entity\Media::preSaveRevision() handles the revision timestamp compared to other entity types

Not sure if we need this change though

diff --git a/core/lib/Drupal/Core/Entity/ContentEntityForm.php b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
index 92bbbae..89e7f9f 100644
--- a/core/lib/Drupal/Core/Entity/ContentEntityForm.php
+++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
@@ -161,12 +161,6 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
     // Save as a new revision if requested to do so.
     if ($this->showRevisionUi() && !$form_state->isValueEmpty('revision')) {
       $entity->setNewRevision();
-      if ($entity instanceof RevisionLogInterface) {
-        // If a new revision is created, save the current user as
-        // revision author.
-        $entity->setRevisionUserId($this->currentUser()->id());
-        $entity->setRevisionCreationTime($this->time->getRequestTime());
-      }
     }

This should fix the tests.

I agree with #22 but that test was only added so we could close #2880199: Revision user not set when saving media. This issue fixes that one as well.

There is no base test for EditorialContentEntityBase as far as I could see, but I guess most of the things we test for in Node/Media are actually functionalities added by its base classes. We could/should probably refactor a lot of the content entity stuff (implementation details, forms, tests) into more usable base classes. While working on media I found that there is way to much duplicate code between content entities and sometimes this also leads to unexpected implementation differences. This is far beyond the scope of this issue though.

Almost, let's try again..

1. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +  public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record) {
   

   Entity types can implement RevisionLogInterface without extending EditorialContentEntityBase, so this needs to go into into ContentEntityBase::preSaveRevision().

2. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      $record->$revision_log_field_name = $this->original->$revision_log_field_name->value;
   

   We can use get|setRevisionLogMessage() here.

3. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      $record->$revision_created_field_name = \Drupal::time()->getRequestTime();
   

   And setRevisionCreationTime() here :)

4. +++ b/core/modules/media/src/Entity/Media.php
   @@ -467,7 +446,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setDefaultValueCallback(static::class . '::getRequestTime')
   
   @@ -501,11 +479,4 @@ public static function getCurrentUserId() {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public static function getRequestTime() {
   -    return \Drupal::time()->getRequestTime();
   -  }
   

   This is used for the 'created' base field, not for the 'revision_timestamp' field, so I don't think we can remove it.

5. +++ b/core/modules/media/tests/src/Kernel/MediaCreationTest.php
   @@ -53,6 +53,7 @@ public function testMediaEntityCreation() {
   +    $this->assertSame($media->getOwnerId(), $media->getRevisionUserId(), 'The media item was not created with the correct revision author.');
   

   Is the assertion message correct here (the "not" part)? Isn't the intention of the assert to check if the new revision owner is the same as the original entity owner ID?

@amateescu:

1. See my comment in #3 why I put that part there, we rely on methods that don't exist on ContentEntityBase and that are not o nan interface. I guess we could add an instanceof check for the trait or a method_exists but that's not very pretty.

5. assertion messages in phpunit are different from simpletest, they are shown only when an assertion fails and should explain what went wrong. See the other assertion messages in the context there.

Right, disregard #26.1 and .5 then :)

Thanks for the review! New patch is attached. Some remarks.

• #26.2 - We can use getRevisionLogMessage() on $this->original, but since $record is a stdClass, we can't use setRevisionLogMessage() right? At least added the change for getRevisionLogMessage().
• #26.3 - Again, we can't use setRevisionCreationTime() on the stdClass.
• #26.4 - Can't find where this is used for the created field? The created field gets a default value REQUEST_TIME from Drupal\Core\Field\Plugin\Field\FieldType\CreatedItem? But maybe I'm missing something.

Agreed with the remarks from #29, let's do this :)

First review with my new shiny framework manager hat on, so please bear with me :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -12,6 +12,7 @@
   +use Drupal\user\EntityOwnerInterface;
   

   Ouch, this is really not an option, sorry. We cannot depend on module code from a Core namespace (see http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/README.txt). This is a clear signal we need some decoupling here: UserInterface extends AccountInterface so the two systems are cleanly decoupled. We should do the same here by introducing \Drupal\Core\Entity\OwnedEntityInterface, to be extended by \Drupal\user\EntityOwnerInterface. We could even deprecate the latter and move all methods up to the former. Additionally we should introduce \Drupal\Core\Entity\OwnerInterface which \Drupal\user\UserInterface can then extend.

   It would be nice to also decouple RevisionLogInterface::setRevisionUser(), but that will require a follow-up because, to avoid BC breaks, we are basically forced to move every single method currently defined in UserInterface up to OwnerInterface, which kind of defeats the purpose of this decoupling. More discussion needed on this probably. A @todo would be enough for now, I can create the follow-up if none beats me to it.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -375,6 +376,12 @@ public function preSave(EntityStorageInterface $storage) {
   +    // If no revision author has been set explicitly, make the entity owner the
   +    // revision author.
   +    if ($this instanceof EntityOwnerInterface && $this instanceof RevisionLogInterface && !$this->getRevisionUser()) {
   +      $this->setRevisionUserId($this->getOwnerId());
   +    }
   

   ContentEntityBase has no concept at all of user/owner whatever, not a single line dealing with that. This strongly suggests this code actually belongs in EditorialContentEntityBase. If for whatever reason a subclass does not extend EditorialContentEntityBase, IMO it's fine for it to handle its own "owner" logic (pun non intended :).

3. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      $record->$revision_created_field_name = \Drupal::time()->getRequestTime();
   

   More coupling here :(

   Aside from that, we should really stop invoking \Drupal:: from non static contexts. Maybe we can just move the original static method onto EditorialContentEntityBase, mark it as @internal, and restore the default value call? We can then open a follow-up to explore the possibility to make dependencies available to default value initialization methods. Again, I can take care of that.

4. +++ b/core/modules/media/src/Entity/Media.php
   @@ -501,11 +479,4 @@ public static function getCurrentUserId() {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public static function getRequestTime() {
   -    return \Drupal::time()->getRequestTime();
   -  }
   -
   

   I was initially thinking this would be a BC break because of the {@inheritdoc} PHP doc, however that is actually a static method on the class, it doesn't appear on the interface, so it's not part of the public API. If we go the way I suggested above, we don't have to worry about this though.

Can we also get an updated issue summary, please? The issue title is clear, but the IS itself looks a bit outdated: it took me a while to understand what was going on here :)

Ah, I forgot: the CR seems outdated...

Crediting people from the issues closed as duplicate of/obsoleted by this.

Scratch #32.3: I didn't realize that's the revision created time...

I've created #2925682: [PP-1] Decouple RevisionLogInterface from the User module to discuss the follow-up work for #32.1.

1. Hm, depending on a module is indeed problematic, but..

a) There are several other such dependencies that already exist beside \Drupal\Core\Entity\RevisionLogInterface (which you mentioned) like \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget.
b) I'm not sure that an entity owner can be an account, I think it needs to be a user entity. To be honest, I'm not sure I fully understand the difference and point of AccountInterface anymore and I'm pretty sure I introduced that in the first place :) (Checking.. yep that was me :p). Also since \Drupal\Core\Session\UserSession also heavily depends on role entities from user module.
c) I don't think we can have a new OwnerInterface and extend the existing one from it with a different type hint: https://3v4l.org/js8rM (that's what I understood you suggested)

Drupal\user is a module but it is a required module and while we try to avoid it, Drupal\Core, unlike Drupal\Component is allowed to have "drupal dependencies". I think it's more like that we have to move in the opposite direction and move user entities to Drupal\Core?

2. Hm, good point, but technically, EditorialContentEntityBase doesn't actually have any existing owner logic either, so that's not really a better place for that :)

3. We can't inject dependencies into entity classes at the moment.

@Berdir:

1.a: I know we have other "backward" dependencies, but I don't think we should be adding more, that means increasing technical debt instead of reducing it, so I'd like to explore a more "correct" solution, if we can find one :)
1.b: Sorry for not being clear, that was just an example of how other Core code solved a similar issue, and I knew it would sound familiar to you ;)
By no means I meant that an entity owner has anything to do with an account. The whole point of what I was suggesting was exactly to decouple the concept of logged in user from the concept of entity owner, which theoretically might even not be a site user at all.
1.c: What I'd like to do is abstracting the generic bits that characterize an entity owner out of UserInterface and bring those to core. I don't think the full User module functionalities and the related concepts (registration, profile, roles, permissions and so on) belong to the entity system and those are all in some form represented in UserInterface. This is what I originally had in mind: https://3v4l.org/n7fa6. However the ::getOwner() method returns a UserInterface, so we cannot change that without breaking BC badly. Hence what I'd suggest we do is the following: https://3v4l.org/L5ApP. That way all module-provided entity classes are free to depend on the User module, but code living in Core, including future traits generalizing this logic, can deal with IDs only and don't need to know about UserInterface at all. This is not fully fixing the issue, as we still have an actual coupling, but at least interface relationships should be cleaner :)

2: Well, it implements RevisionLogInterface, so IMO that's a reasonable place for that hunk. At least the concept of revision "owner" (or "author") is definitely there :)

   // If no revision author has been set explicitly, make the entity owner the
   // revision author.
   if ($this instanceof EntityOwnerInterface && !$this->getRevisionUser()) {
     $this->setRevisionUserId($this->getOwnerId());
   }

3: Yep, that's why I was initially suggesting to restore the default value callback: I didn't realize that that code was concerning the entity created timestamp, not the revision one. Actually the following change was introduced in #23 and looks unrelated to me. On a second look the fact that it's needed to fix test failures is concerning, am I missing anything?

+++ b/core/modules/media/src/Entity/Media.php
@@ -467,7 +446,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setRevisionable(TRUE)
-      ->setDefaultValueCallback(static::class . '::getRequestTime')
       ->setDisplayOptions('form', [

Aside from that, it's a shame that we need to couple the entity system with the request time component, since again we had no concept of request in the (Content)Entity class so far. Every time we decide to provide a default value inside a method, we are at risk of (needlessly) introducing a new dependency, and even if we may be able to inject these sooner or later, I'm not sure an entity is the right place to do that. Anyway, at least that's a Component, so it's not as bad as depending on a module, I'd just wish we could revert this trend, instead of adding more and more of these \Drupal:: occurrences, that imply more technical debt and (often) a more coupled system. It would be better if there were a way to set this value from outside the entity class, but I don't have a good suggestion, so I guess this will do for now.

A couple of minor things I found while reviewing the code again:

1. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    if (!$new_revision && isset($this->original) && (!isset($record->$revision_log_field_name) || $record->$revision_log_field_name === '')) {
   +      // If we are updating an existing node without adding a new revision, we
   

   Can't this be simplified to if (!$new_revision && isset($this->original) && empty($record->$revision_log_field_name)) {?

2. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +
   +    if ($new_revision && (!isset($record->$revision_created_field_name) || empty($record->$revision_created_field_name))) {
   +      $record->$revision_created_field_name = \Drupal::time()->getRequestTime();
   +    }
   

   This can be simplified to if ($new_revision && empty($record->$revision_created_field_name)) {. I guess 0 is not a valid value for this field, right?

Hi @plach

Just some thoughts, need to think more about this stuff. I understand your reserveration, but I'm not sure how to address that properly.

One problem with this issue is that while it is classified as a task, it also actually *adds* this mandatory functionality to media entities which are missing it right now and that is a bug. So as this seems to be more complicated, we could re-open the media bug report and fix it with copy & paste there. But the fact that this logic is mandatory and easy to forget is IMHO also an argument to find a generic way to solve it.

1b/c) I'm not sure that just moving the ID methods to OwnedEntityInterface would really help. For this request yes, but an ID *without* having any meaning whether it is a user entity or not is not enough, getOwner() is nice for DX when working with the owner (e.g. loading the name or some other information from it to display it). We couldn't deprecate EntityOwnerInterface then. And sure, we definitely shouldn't move the whole of user module into Drupal\Core, if anything then just the user entity.

Aside from that, it's a shame that we need to couple the entity system with the request time component, since again we had no concept of request in the (Content)Entity class so far.

The dependency on the current time (which itself is depending on the request) is not new. We're just making it more explicit since we deprecated REQUEST_TIME, which we've been using for a long time in ChangedItem/CreatedItem for example. That's what moving things to components does, it makes dependencies more explicit :)

One random idea would be to find a way to move this logic to user module, at least the parts that depend on an owner. Then we don't introduce a dependency on it. But I don't think we have a hook/event right now for preSaveRevision() and not sure we want to introduce that. It would also result in this functionality being more hidden.

Maybe the User module could define a \Drupal\user\OwnedEntityBase class extending EditorialContentEntityBase and implementing EntityOwnerInterface? That way all module-defined classes could safely depend on the User module without "polluting" the Core entity system?

Possibly, but the problem with base classes is always that you can only have one. So if we add OwnedEntityBase then you can't extend from something else anymore. EditorialContentEntityBase is a bit different because it combined quite a few things together which is helpful.

Well, this would be just a User-flavored version of EditorialContentEntityBase, so if you're fine with the latter, you should be fine also with the former IMO. A User-defined trait would have the opposite advantages/drawbacks...

I created this issue that might be a good solution for this issue #2978701: Replace the revision log message with a comment field

One possible solution to avoid depending on EntityOwnerInterface would be to use $this->getEntityType()->hasKey('owner') to determine if the Entity supports having an owner, you can use a similar abstracted approach for getting and setting the actual value. See \Drupal\user\EntityOwnerTrait for more details.

Having said that there already seems to be president in the core Entity API for depending on the User module, see \Drupal\Core\Entity\RevisionLogInterface and \Drupal\Core\Entity\RevisionLogEntityTrait for example. So given that either we could go with my suggestions above to abstract it out a bit, or we could just use the existing president I just linked and continue on, then later open a meta issue or a task issue to fully decouple the Entity API from the User module.

Hey @Berdir @plach - just wondering what the way forward is here. A legitimate bug was closed as a duplicate of this Task and the conversation has essentially been dead since 2017. I just came across this bug with a different entity type so agree 100% that it should be fixed in some generic way.

I've rerolled #29 against HEAD, setting to NR to make sure things still pass.

Fixes deprecated calls.

Gah...fixing whitespace.

This also seems related.

1. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $revision_log_field_name = $this->getEntityType()->getRevisionMetadataKey('revision_log_message');
   +    $revision_created_field_name = $this->getEntityType()->getRevisionMetadataKey('revision_created');
   

   micro-optimisation: should we store the result of ::getEntityType in a variable instead of calling the same method twice?

2. +++ b/core/lib/Drupal/Core/Entity/EditorialContentEntityBase.php
   @@ -28,4 +28,25 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    if ($new_revision && (!isset($record->$revision_created_field_name) || empty($record->$revision_created_field_name))) {
   

   do we need !isset and empty? I think empty covers both cases

3. +++ b/core/modules/media/src/Entity/Media.php
   @@ -511,11 +489,4 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -  public static function getRequestTime() {
   

   I don't think we can delete this, we'll need to trigger a deprecation and keep it until D11.

   Public methods are technically an API

Love the cleanup here

Fixes #63, the interdiff is pretty scuffed since #57 applied with quite a bit of fuzz. I'm not sure if more work needs to be done to decouple stuff as per #32?

CR drafted - https://www.drupal.org/node/3349765

I've already discussed #32 with @plach then and still think that Drupal\Core doesn't have to be free of Drupal\user references, would be nice, but Drupal\user is a required module and the difference between a Drupal\Core component and a required module like user and system is IMHO minor and mostly historical/due limitations what exactly Drupal\Core components can contain.

I can see that other parts of that comment have been addressed, the logic was moved to EditorialContentEntityBase. That said, wouldn't RevisionLogEntityTrait be a better fit for this logic? EditorialContentEntityBase is just a handy aggregation of several traits and currently doesn't have its own logic.

Wonder if this could still get an issue summary update please per #32

Definitely agree that we shouldn't be referencing module code from Drupal\Core and it's a policy that has been documented in https://git.drupalcode.org/project/drupal/blob/HEAD/core/lib/Drupal/Core.... However, I think it would be fair to ask for an exception from the framework managers based on the fact that there are several pre-existing uses of the same interface in the Drupal\Core namespace. We could open a follow-up for implementing a solution for #41. It doesn't look like implementing that becomes any harder if we fix this issue separately first.

Could we move Drupal\user\EntityOwnerInterface to Drupal\Core\Entity\EntityOwnerInterface, and then have it use Drupal\Core\Session\AccountInterface rather than Drupal\user\UserInterface?

Technically it might still work, as AccountInterface has an id() method and UserInterface extends AccountInterface so it wouldn't be far removed from the current implementation.

Doing that would allow us to decouple from the user module.

> Could we move Drupal\user\EntityOwnerInterface to Drupal\Core\Entity\EntityOwnerInterface, and then have it use Drupal\Core\Session\AccountInterface rather than Drupal\user\UserInterface?

An "account" is currently an abstract concept that might or might not be a user entity, in theory it could be something else. Yes, UserInterface extends AccountInterface, but that means that a user is an account, not necessarily the opposite. Of course, most places we assume it is and every User::load($currentUser->id()) would fall apart badly if it weren't, but technically, that is how it is defined. So we'd first need to redefine that an account id does in fact need need to be a user id. At that point, having two different interfaces wouldn't actually make too much sense anymore.

We'd also definitely need to to #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries as well.

So that would have pretty big implications, and would set us back at least one major release cycle before we could rely on that.

Of course, most places we assume it is and every User::load($currentUser->id()) would fall apart badly if it weren't, but technically, that is how it is defined. So we'd first need to redefine that an account id does in fact need need to be a user id.

I was thinking about that in the back of my head, but I was also thinking that, but in theory a site could have its own "user" implementation which extends AccountInterface, and in theory not even use the user module. Again, this is just a theory, I've never practically tested that, and it's very likely that it just wouldn't work, but in theory we could support that scenario by purposefully not coupling to the user module.

Re-rolled #64 for 11.x and converted to MR. Hiding patches. Have not addressed any recent comments.

Updated version to 10.2.0.
Thanks!

Pushed an updated 11.x branch to the fork to fix the spellcheck error, see https://www.drupal.org/project/drupal/issues/3401988#comment-15400828

Updated IS, added framework manager tag based on #68. FWIW - I really hope we can use the interface there because the magic getters/setters are not great in the current state.

Trying the patch for Drupal 10 in comment #64 and when creating a new media, the revision user is now correctly set.

But when editing the same media with another user, the revision is still set to the owner user id and not the user that is editing the media.

In media_form_alter, i have to manually set it to finally have the current user in the revision log.

  $media_entity = $form_state->getFormObject()->getEntity();
  $media_entity->setRevisionUserId(\Drupal::currentUser()->id());

NW based on #81 - great find @Tomefa

The weird part is, that code looks to have come from Node (setting the revision author to the author if it's not set) so I wonder why that same issue doesn't already apply to Node.

Will try to dig into it today.

Marking as Needs tests to cover that scenario (editing with a different user)

To answer #82 - it comes from here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Manually testing Media works fine for me.

However, it doesn't make sense to set the revision author in 2 places, and it especially doesn't make sense to set it to the author. I think we should remove it from the form and fix the entity base implementation.

Pushed test coverage for #81, still unsure how to reproduce it via the UI since Media goes through that same form base.

The !$this->getRevisionUser() check in ContentEntityBase also fails on API based updates so we may need to remove that and always hardset to the current user.