ContentEntityBase::hasTranslationChanges should exclude the "changed" fields from the comparison

Good catch. The fix is correct and the patch adds test coverage.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1173,10 +1173,20 @@ public function hasTranslationChanges() {
   +    $fields_do_not_compare = [
   

   Can we name this $skip_compare

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1173,10 +1173,20 @@ public function hasTranslationChanges() {
   +      if (isset($fields_do_not_compare[$field_name])) {
   

   I think this logic should be applied only between different revisions, can we have changes in revision metadata when updating the current revision instead of creating a new one? If so we should add test coverage for this change.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,99 @@
   +   * Tests the correct functionality of the hasTranslationChanges function.
   +   *
   +   */
   

   Surplus blank line

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,99 @@
   +    // revision metadata field revision_timestamp
   ...
   +    // revision metadata field revision_uid
   ...
   +    // revision metadata field revision_log
   

   The first letter should be capitalized and the comment should have a trailing dot.

Looks good now, only a typo to fix!

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,146 @@
+    // Check that the metadata fields are been skipped when comparing same

have been

I wonder if we should also skip changed and content_translation_changed, because these fields are usually set to a new value as a consequence of the return value of hasTranslationChanges(). In ChangedItem::presave() the call of hasTranslationChanges() is skipped if the value of this field (named changed or content_translation_changed) has been changed already.
But if you think of any re-usage of hasTranslationChanges() somewhere else as part of the API, it makes sense to skip this field like the revision specific fields.

Just a little bit of history:
hasTranslationChanges() was a part of ChangedItem unless @catch proposed to make it available as part of the API to be re-usable. I feel like we missed to explicit skip the "changed" field within in the isolated hasTranslationChanges() at this point. But I'm still thinking about this ...

The second usage of this method in core is in ContentEntityStorageBase::populateAffectedRevisionTranslations(). In this case it would be wrong to set the revision_translation_affected flag is set if only the value of the changed field itself changed. Otherwise for nodes we will list to revisions of a translation with no changes except their changed date. And this date is not revertable.
So I'm convinced that it's correct to skip "changed" and "content_translation_changed" as well.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1173,10 +1173,32 @@ public function hasTranslationChanges() {
       // @todo Avoid special-casing the following fields. See
       //    https://www.drupal.org/node/2329253.
-      if ($field_name == 'revision_translation_affected' || $field_name == 'revision_id') {

Can we also remove the @todo now?

Yep, waiting for the outcome of #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table sounds wise :)

This was discussed with @alexpott, @amateescu, @berdir, @cilefen, @fago and @xjm. We decided to defer triage because we were not sure about the steps to reproduce a significant problem. What issues does this cause? Can they be reproduced via the UI or only via some particular programmatic circumstances? It would be great if the IS provided more info about this.

(Sorry, it's been a long time since the last time I had a look to this code :)

Quick code review:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1173,10 +1173,32 @@ public function hasTranslationChanges() {
+        'content_translation_changed' => TRUE

The core Entity API should know nothing about the Content Translation module: we are introducing a circular dependency here. We probably need a way to hook into this process and provide a list of fields.

Marking this as postponed on #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table in the title, I still think we should continue iterating here in the meantime, so not setting the status to Postponed.

There is actually a separate bug that I have yet to open an issue for, that currently masks any real-world consequences as far as I can tell. This is still major to me, because it breaks a fundamental expectation when working with translations and revisions, but more on that later...

So as far as I can tell #2808335: Changed time not updated when only non-translatable fields are changed on a translated entity masks this bug because revision metadata fields are not marked translatable, so that they are actually ignored for changed time tracking currently because of this. @hchonov please correct me if I am incorrect on this (or anything else).

Assuming that that other bug were fixed, what this bug would lead to is that as soon as someone enters a revision log message, the changed timestamp will be updated even though nothing of the actual entity was changed. This is annoying for any code that actually relies on proper changed time tracking, especially across revisions.

Having written that, however, I agree that this is probably not major.

I would like @hchonov to validate my assessment, though, before downgrading it.

+1 Testing

#2808335: Changed time not updated when only non-translatable fields are changed on a translated entity is in, but the comment above said this blocks that, I guess it was meant the other way round?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1331,11 +1332,11 @@ public function hasTranslationChanges() {
          //    https://www.drupal.org/node/2329253.
   -      if (in_array($field_name, $skip_fields, TRUE)) {
   +      if (in_array($field_name, $skip_fields, TRUE) || (($field = $this->get($field_name)) && ($field instanceof ChangedFieldItemList))) {
            continue;
          }
          if (!$definition->isComputed()) {
   -        $items = $this->get($field_name)->filterEmptyItems();
   +        $items = $field->filterEmptyItems();
   

   I find the way $field is used here a bit hard to understand. We conditionally assign it in the first if but there's nothing conditional about it, and get() would actually throw an exception in that case.

   So, can you move the $field = $this->get($field_name) either outside the loop or not use a temporary variable for it? $this->get($field_name) instanceof .. would IMHO be perfectly fine here as well.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,116 @@
   +/**
   + * @file
   + * Contains \Drupal\KernelTests\Core\Entity\ContentEntityHasChangesTest.
   + */
   

   remove this.

I think the patch itself looks fine, but I think we should add a code-comment to the added condition. Something like this, maybe?:

When saving entities in the user interface, the changed timestamp is automatically incremented by ContentEntityForm::submitForm() even if nothing was actually changed. Thus, the changed time needs to be ignored when determining whether there are any actual changes in the entity.

I have some minor notes as well, but marking needs work mostly for the adding of a comment. Doesn't have to be my comment, but something that explains why we are doing this would be rather important to add, IMO.

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,110 @@
   + * Tests the hasTransationChanges function of content entities.
   

   Missing "l" in hasTranslationChanges. Also maybe "Tests ContentEntityBase::hasTranslationChanges()." would be clearer?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,110 @@
   +    // Create a new node-type.
   +    NodeType::create([
   +      'type' => $this->bundle,
   +      'name' => $this->randomString(),
   +    ])->save();
   

   Why are you using "node" here and not some test entity type?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,110 @@
   +  public function testhasTranslationChanges() {
   

   h -> H (for the first h ;-))

Documentation changes from #42.

Returning to "Needs Work" for someone to address:

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
   @@ -0,0 +1,110 @@
   +    // Create a new node-type.
   +    NodeType::create([
   +      'type' => $this->bundle,
   +      'name' => $this->randomString(),
   +    ])->save();

   Why are you using "node" here and not some test entity type?

I tried using EntityTest::create() in place of NodeType::create(), but couldn't get it to work.

Right, sorry that was a bit brief. Here is a more in detail description of what is needed.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+  public static $modules = ['system', 'user', 'node'];

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+    $this->installEntitySchema('node');
...
+      ->getStorage('node');

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+    // Create a new node-type.
+    NodeType::create([
+      'type' => $this->bundle,
+      'name' => $this->randomString(),
+    ])->save();

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+    /** @var \Drupal\node\NodeInterface $node */
+    $node = $storage->create([

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+      'type' => $this->bundle,

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityHasChangesTest.php
@@ -0,0 +1,110 @@
+      'title' => $this->randomString(),

Made changes as suggested in comment #45. Applying the patch, please review.

@tstoeckler I now see why we are using node rather than a test entity type: The tests rely on 3 functions that only exist on nodes - setRevisionAuthorId(), setRevisionCreationTime() and setRevisionLogMessage(). Theoretically we could remove the calls to these functions, but I think this would weaken the test (I have uploaded this patch as a proof of concept). I recommend returning to my patch in #43 as the solution.

We already have EntityTestWithRevisionLog however that is not translatable and does not have a changed field, so I guess we need to create a EntityTestMulRevChangedWithRevisionLog or something?

IMO the new entity type is quite straightforward.

Wow, that looks pretty sweet @Jo Fitzgerald!!

Thanks also for the two interdiffs, that was very helpful!

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevChangedWithRevisionLog.php
@@ -0,0 +1,46 @@
+ *   id = "entity_test_mulrev_chnged_revlog",

Missing an "a" in "chnged". I wanted to mark this "needs work" for that, but I checked and I guess you intentionally left that out, because otherwise the ID would be longer than EntityTypeInterface::ID_MAX_LENGTH. That's pretty funny, to be honest. But I think you chose the best resolution for this, as the ID is still pretty readable and still fits with out existing naming pattern.

....so, RTBC! Thanks again!

Aww, shucks, that comment has made my day. Thanks @tsoeckler!

With hindsight I should've mentioned the missing 'a' in my comment, but you read my mind anyway!

Hehe, well you gave yourself away a bit, because it was a bit too suspicious that you made that same mistake multiple times in different places ;-)

Committed and pushed 4dde9c8 to 8.4.x and eb5cb0f to 8.3.x. Thanks!

entity_test_mulrev_chnged_revlog is a bit weird but shouldn't hold up this patch.