ChangedItem will not detect a change if another field item changes its value in field::preSave

Make sure ChangedItem::preSave is called as the last one of all other FieldItems::preSave.. This could be done by ensuring that EntityFieldManager::getFieldDefinitions will return an array of field definitions, where the changed field definition is at the end.

That sounds brittle to me. I'd rather convert the code to a differnt approach taking onChange() into account for figuring out changed fields - as discussed in https://www.drupal.org/node/2456257

@fago but like discussed in the other issue onChange is not a solution as well, because it fires even if you set the same value again ...

Well, in generally it does not solve the timing/order issue :/

Still, I'd favour some explicit instead of magic ordering of field definitions. E.g., why not let the storage update the changed field based upon EntityChangedInterface? It might be tricky to get this done in a BC manner though.

+++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
@@ -304,6 +304,19 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
+    if ($this->entityTypeManager->getDefinition($entity_type_id)->isSubclassOf(EntityChangedInterface::class) && isset($this->fieldDefinitions[$entity_type_id][$bundle]['changed'])) {

we deprecate isSubclassOf(), I suggest you just use getClass() check that directly.

This was discussed with @alexpott, @amateescu, @berdir, @cilefen, @fago and @xjm. We agreed this is a major issue because, even it may happen under not so common circumstances, the level of breakage it may imply is significant.

This patch is great. It's not the coolest thing in the world that we have to rely on the order of field definitions but it is certainly completely backwards compatible and I cannot see any other way to solve this issue generically without significantly more maintenance overhead. So I think we can get this in in its current form. Also, since it comes with tests, we an always improve the implementation if someone thinks of something better without regressing.

I do have some notes before RTBC, though.

1. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -304,6 +304,20 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
   +    // If the entity type is implementing the EntityChangedInterface and
   +    // has a changed field, then this field should be returned at the end
   +    // of the list, so that when iterating over the fields and calling
   +    // preSave on them the preSave of the changed item will be called as
   +    // the last one to ensure that if the other fields changed their values
   +    // in the preSave the changed item is going to detect this change.
   

   Here and below: Wraps too early.

   I also think the text is a bit hard to read, here is my suggestion (I initially had a bunch of individual notes, but I think that would have been harder to follow than this):

   If the entity type class implements EntityChangedInterface and has a changed field then this field should be at the end of the list so that when saving an entity the preSave() method will be called on the changed field last. This ensures that the changed field detects if other fields change their values in their own preSave() implementation.

   As always, feel free to improve on this further.

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -304,6 +304,20 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
   +    if (is_subclass_of($entity_class, EntityChangedInterface::class) && isset($this->fieldDefinitions[$entity_type_id][$bundle]['changed'])) {
   

   I was going to complain about hardcoding the name of the changed field here, but there's already a precedent for that in EntityChangedTrait so that's fine here. Re-purposed #2209971: Automatically provide a changed field definition to fix that.

3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedCustomTestItem.php
   @@ -0,0 +1,36 @@
   +/**
   + * @file
   + * Contains \Drupal\entity_test\Plugin\Field\FieldType\ChangedCustomTestItem.
   + */
   

   No longer necessary.

4. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedCustomTestItem.php
   @@ -0,0 +1,36 @@
   +class ChangedCustomTestItem extends ChangedItem {
   ...
   +  public function preSave() {
   

   ChangedItem only defines preSave() so if we're overriding that without calling the parent, it seems pointless to extend it, so extending either CreatedItem or TimestampItem directly would be better, I think.

Warrants a "needs work" I guess.

OK, thanks! Going to mark this RTBC.

As noted above this solution is not very "elegant", but solving it "properly" would need some sort of field dependency/weight system, which would be nice for a couple reasons, but we simply don't have and I don't think is something we are realistically going to get in D8. And while not being elegant, this still is a "minimally invasive" fix, with not much maintenance overhead from my point of view. Especially for fixing a major bug, this is really a very good tradeoff. Of course committers may disagree, but for me this is good to go.

I had one other remark namely that the test seems to be a bit brittle in that - as far as I can tell - it relies on the fact that between the first and second entity save one second or more of time have passed. However, that's a pre-existing issue with ContentEntityChangedTest all over the place so it doesn't make sense to hold this up on that, because apparently that hasn't been causing issues or we would know about it.

OK, awesome that it's green again. However, I do not feel comfortable RTBCing the REST test changes, I think someone from the REST team should have a look that. I.e. I'm not sure if we are intentionally sending multiple fields across the wire to make sure that the validation does not fail on missing fields or something like that.

I will try to summon @WimLeers.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
@@ -52,6 +54,42 @@ protected function setUp() {
+    $field_storage = FieldStorageConfig::create(array(
+      'field_name' => 'field_changed_custom',
+      'entity_type' => 'entity_test_mul_changed',
...
+      'cardinality' => 1,
+      'translatable' => FALSE,
+    ));
...
+    FieldConfig::create(array(
+      'field_storage' => $field_storage,
+      'bundle' => 'entity_test_mul_changed',
+    ))->save();
...
+    $entity = EntityTestMulChanged::create(array(
+      'name' => $this->randomString(),
+    ));

use short array syntax (new coding standard).

Fixed the short array syntax error and attached new patch.

+1

Just one nit, to clarify an otherwise kinda confusing comment:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -901,17 +901,20 @@ public function testPatch() {
+      // testing with in order to make the test independent of the order of the

s/order/validation order/

Looks good to me.

+1!

+++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
@@ -304,6 +304,19 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
+    // If the entity type class implements EntityChangedInterface and has a
+    // changed field then this field should be at the end of the list so that
+    // when saving an entity the preSave() method will be called on the changed
+    // field last. This ensures that the changed field detects if other fields
+    // change their values in their own preSave() implementation.
+    $entity_class = $this->entityTypeManager->getDefinition($entity_type_id)->getClass();
+    if (is_subclass_of($entity_class, EntityChangedInterface::class) && isset($this->fieldDefinitions[$entity_type_id][$bundle]['changed'])) {
+      $changed = $this->fieldDefinitions[$entity_type_id][$bundle]['changed'];
+      unset($this->fieldDefinitions[$entity_type_id][$bundle]['changed']);
+      $this->fieldDefinitions[$entity_type_id][$bundle]['changed'] = $changed;
+    }

This is clever fix but one that in the long run might not pay off. What happens when something else needs to come last in preSave? We have this problem all over the place especially in the Entity API. I think we need to consider that preSave's that change values are fundamentally different to preSave's that react to values being saved. What is super awkward here is that the changed field wants to be in both camps - reacting to other values changes and changing its own value. This makes me question the architecture.

The onChange() solution does seem like the proper fix but as @hchonov points out...

but like discussed in the other issue onChange is not a solution as well, because it fires even if you set the same value again

But isn't that a bug too? Perhaps a way forward here is to go with this fix for now but open followup to fix onChange firing when there is no change (if possible) and then refactor the changed field to use it instead of this?

Setting back to needs review because I think we should file the followups before committing this and probably add a @todo.

Looks good to me. I think it's a good idea to try to improve this in a follow-up, but would also nice to fix this for now at least.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
@@ -52,6 +54,42 @@ protected function setUp() {
+    $this->assertTrue($entity->field_changed_custom->value > $changed_custom_time, 'The custom changed field item updated its value');
+    $this->assertTrue($entity->getChangedTime() > $changed_time, 'The changed field item updated its value');

These greater thans make me nervous of random fails. What happens if the whole test is quicker that 1 second?

Read through the test again and it looks good to me... and still fails at the expected fails without the fix. Thanks!

1. Sorry I had some more thoughts on this patch whilst reading it again

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -304,6 +304,21 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
   +    // If the entity type class implements EntityChangedInterface and has a
   +    // changed field then this field should be at the end of the list so that
   +    // when saving an entity the preSave() method will be called on the changed
   +    // field last. This ensures that the changed field detects if other fields
   +    // change their values in their own preSave() implementation. A better
   +    // implementation where the updating of the changed field should not depend
   +    // on the field order will be fixed by https://www.drupal.org/node/2863844.
   

   Here is a suggested refactor for shorter sentences:

       // If the entity type class implements EntityChangedInterface and has a
       // changed field, the change field's definition should be last. This is so
       // that the changed field's preSave() method will be called last. This
       // ensures that, if other field values change as a result of preSave()
       // implementations, the changed field value is correct.
       // @todo https://www.drupal.org/node/2863844 Implement a solution where the
       //   field definition list does not need to be manipulated.
   

3. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -304,6 +304,21 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
   +    if (is_subclass_of($entity_class, EntityChangedInterface::class) && isset($this->fieldDefinitions[$entity_type_id][$bundle]['changed'])) {
   

   Reading this again it makes me ponder something. Whether or not EntityChangeInterface means there has to be a field called 'changed'. Or could it have another name. As far as I can see you could implement the interface and not call the field that. This might be a can of worms.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -901,17 +901,20 @@ public function testPatch() {
        // error message for the first PATCH-protected field. Remove that field from
        // the normalization, send another request, assert the next PATCH-protected
        // field error message. And so on.
   -    $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
   +    $max_normalization_complete = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
        for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
   -      $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i));
   -      $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
   +      // From all the protected fields leave only the field we are currently
   +      // testing with so that the test doesn't depend on the order in which the
   +      // entity fields are returned and validated.
   +      $max_normalization_reduced = $this->removeFieldsFromNormalization($max_normalization_complete, array_diff(static::$patchProtectedFieldNames, [static::$patchProtectedFieldNames[$i]]));
   +      $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization_reduced, static::$format);
          $response = $this->request('PATCH', $url, $request_options);
          $this->assertResourceErrorResponse(403, "Access denied on updating field '" . static::$patchProtectedFieldNames[$i] . "'.", $response);
        }
    
        // 200 for well-formed request that sends the maximum number of fields.
   -    $max_normalization = $this->removeFieldsFromNormalization($max_normalization, static::$patchProtectedFieldNames);
   -    $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
   +    $max_normalization_reduced = $this->removeFieldsFromNormalization($max_normalization_complete, static::$patchProtectedFieldNames);
   +    $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization_reduced, static::$format);
        $response = $this->request('PATCH', $url, $request_options);
        $this->assertResourceResponse(200, FALSE, $response);
   

   I think all of this needs a bit of work and hopefully @Wim Leers can provide a bit of backgroup of whether the changes are acceptable because we're changing the test quite fundamentally here.

   I think it should be something like:

       // DX: 403 when sending PATCH request with read-only fields.
       // Assert the expected error messages for each read-only field in the
       // normalization.
       $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
       foreach (static::$patchProtectedFieldNames as $field_name) {
         $normalization_to_test = $this->removeFieldsFromNormalization($max_normalization, array_diff(static::$patchProtectedFieldNames, [$field_name]));
         $request_options[RequestOptions::BODY] = $this->serializer->serialize($normalization_to_test, static::$format);
         $response = $this->request('PATCH', $url, $request_options);
         $this->assertResourceErrorResponse(403, "Access denied on updating field '" . $field_name . "'.", $response);
       }
   
       // 200 for well-formed request that sends the maximum number of fields.
       $max_normalization = $this->removeFieldsFromNormalization($max_normalization, static::$patchProtectedFieldNames);
       $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
       $response = $this->request('PATCH', $url, $request_options);
       $this->assertResourceResponse(200, FALSE, $response);
   

5. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedCustomTestItem.php
   @@ -0,0 +1,35 @@
   +    // If an initial value is provided when the entity is being created then
   +    // skip updating the field value.
   +    if ($this->getEntity()->isNew() && isset($this->value)) {
   +      return;
   +    }
   +    $this->value = \Drupal::time()->getCurrentTime() + 1;
   

   I think the fact that this is a time() makes the test harder to understand. The main point is that it always updates on save - it would be simpler to understand it is was just a counter that added one on every save. And then you don't even really need the isNew() complexity. You can just check what the count equals.

+++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
@@ -304,6 +304,21 @@ public function getFieldDefinitions($entity_type_id, $bundle) {
+    if (is_subclass_of($entity_class, EntityChangedInterface::class) && isset($this->fieldDefinitions[$entity_type_id][$bundle]['changed'])) {

Reading this again it makes me ponder something. Whether or not EntityChangeInterface means there has to be a field called 'changed'. Or could it have another name. As far as I can see you could implement the interface and not call the field that. This might be a can of worms.

The thing is that we actually do hardcode that the field is called changed in EntityChangedTrait. Of course people are not required to use that trait, but it is kind of an API that we provide and that does hardcode the field name. There is already the (not so aptly named) #2209971: Automatically provide a changed field definition for fixing that and making it into a proper entity key.

So my personal view is that we should leave it like it is in the patch currently and if people use a different field name they will not benefit from this optimization until #2209971: Automatically provide a changed field definition lands, but it's not that something explicitly breaks.

Thoughts?

#52.4: I was explicitly asked by @tstoeckler and @hchonov to review the REST changes. I did that in #37.
This is not changing the tests fundamentally; this is only ensuring that the validation order no longer matters.
That being said, your proposed changes have exactly the same effect (they're functionally equivalent) and are much easier to read/understand. So, +1 for adopting your proposed changes instead.

Needed a re-roll due to a change in EntityResourceTestBase since I had to merge the code manually anyway, I had a look whether I can simplify the diff and I think I found a way to not only simplify the diff, but actually simplify the test itself relative to the current code. The "interdiff" is relative to the code in current 8.7.x it's what git diff -w showed during the rebase of the re-roll.

Note that the method whose signature I am changing is internal and only used in that one place, so I think we are good in terms of BC.

I've tested the patch in #60 to see if it solves #2912318: Changing field value in an entity presave hook will not update the entity changed timestamp where the problem is that the timestamp is not updated for image fields with translatable title / alt attributes when they are edited concurrently. However, after I apply the patch I'm still not able to get the lock to work and it seem like it is not detected that the image field have been changed.

Steps to reproduce:

• Set up an image field. Make the title and alt attribute translatable, but make the image file non-tranlatable
• Edit the a node concurrently in 2 different languages: In the first language you change the image file, then after you simply save the other language without doing anything
• Result; There is no lock saying that the content has been changed. The new image added is gone as saving the second language overrides the new image added in the first language, and this happens without any notice.

Needed a rebase, but Git was able to handle it.

So this is one of those cases where Git will successfully rebase into a broken state. The variable I am removing here received another usage below, outside of the patch scope...

Let's see if this fixes it.

Just a re-roll.

This should fix the tests, but raises questions.

The main question was - why in the updated list of protected fields 'changed' field is not the last one? After some debugging I found out, that the fields that go after 'changed' in this updated list of protected fields are just missing from the request. Which, imho, is the issue for the test (as well as relying on fields order), but is outside of the scope of the current issue.

Rerolled for 9.4.x. Should also apply to 9.3.x.

Fixed all the errors from the patch in #77. Attached a diff file between #69 - #78 and between #77 - #78.

Thanks!