Add ability to track an entity object's dirty fields (and see if it has changed)

This is so important. We have half implemented this for translated fields in #2297817: Do not attempt field storage write when field content did not change

+++ b/core/lib/Drupal/Core/Entity/EntityWithDirtyFieldsInterface.php
@@ -0,0 +1,27 @@
+ * This interface extends the general fieldable entity interface. So that that

Word 'That' is mentioned twice in a sequence.

Fixed and attached new patch.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -567,6 +581,13 @@ public function getFields($include_computed = TRUE) {
+    return array_intersect_key($this->getFields(), $this->dirtyFields[$this->activeLangcode]);

Just asking here. According to this https://www.drupal.org/node/2112677 computed fields section says: "This tells the Field API that this field is computed so that it doesn't look for it in the database."

I'm not sure if the docs are accurate, but, would be the same use getFields(FALSE) here?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -157,6 +158,13 @@
+  protected $dirtyFields = [];

You are defining a new entity property, which should be unique per entity object, but when dealing with translations and entity cloning, which is really common in Drupal it will break and leak. To avoid this, you have to break the reference in CEB::__clone if not cloning because of initializing a translation and in CEB::initializeTranslation make the property shared between the entity translation objects.

For common mistakes regarding this you could take a look at

• #2828073: Cloning an entity with initialized translations leaves $entityKeys and $translatableEntityKeys pointing to the old entity object
• #2862463: Cloning an entity with initialized translations leaves $values pointing to the old entity object
• #2841311: Initialized fields of an entity clone have a reference to the original entity object instead to the cloned entity object
• #2675010: Cloned entity will point to the same field objects if the clone was created after an entity translation has been initialized

...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -701,6 +722,9 @@ protected function updateFieldLangcodes($langcode) {
   public function onChange($name) {
+    // Add the field to the dirty fields list for this translation.
+    $this->dirtyFields[$this->activeLangcode][$name] = TRUE;

You have to be aware of the current behavior of CEB::onChange, which will be fired even if you set the same value on a field e.g. $field->setValue($field->getValue()) and if you are ready to ignore this at the moment then probably it would be nice to document this in the code and open a follow up issue.

Removing credit for @Munavijayalakshmi. Thanks for your interest in contributing. Please make sure that your contributions are part of the current discussion on the issue. Read the issue carefully before posting new patches.

We just had a discussion about the new interface on IRC and personally at the DrupalDevDays Seville with tstockler and berdir and they both agree that we add new methods to content entities by adding them to ContentEntityInterface and in D9 they should be moved to the according interface - in this case to FieldableEntityInterface. We've done this for an example with the getLoadedRevisionId method.

This means we do not need the new interface and instead we have to add the new method to ContentEntityInterface.

I think that a @todo is fine.

Here are some thoughts on the patch. I still want to make a list/table of "possible things you can do to entities" and the expected return value of getDirtyFields() in each case for the issue summary.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -157,6 +158,13 @@
   +   * @var string[]
   

   Is this not "string[][]"? (is that allowed? Or should it be array[]?)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -157,6 +158,13 @@
   +  protected $dirtyFields = [];
   

   @hchonov pointed this out in person yesterday when we talked about this: Since this should be "synchronized" between all translations of an entity (as it already keys field names per translation), we should convert it to a reference in ContentEntityBase::initializeTranslation() - and then also break that reference in ContentEntityBase::__clone(). See the handling of the other properties in those methods, for inspiration.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -254,7 +262,11 @@ protected function getLanguages() {
   +    // @todo TEST COVERAGE FOR CHANGES IN THIS HUNK
   +    if ($this->isDefaultTranslation()) {
   +      $this->newRevision = TRUE;
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -135,6 +135,8 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +    // @todo TEST COVERAGE FOR CHANGES IN THIS HUNK
   +    $translation->postCreate($this);
   

   Ahh these hunks belong together, now I understand what is going on.

   That's interesting. On the one hand this is the minimal fix to resolve this, on the other hand this could arguably be seen as a BC break. I.e. if I see the current code in ContentEntityBase::postCreate() and want to provide a default revision message for my entity I would override postCreate() and unconditionally (!) call $this->setRevisionMessage('Just created a new entity!') there. That would now then also be called for new translations, which would be unfortunate.

   The other (slightly more invasive) way would be to add a postCreateTranslation() method or similar, although that also seems non-ideal as then one could argue we would also need equivalent hooks for translation updating and deleting.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -390,6 +402,8 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
   +    $this->dirtyFields[$this->activeLangcode] = [];
   

   As far as I can tell, this should clear the list of dirty fields in all languages, because we always save all translations of an entity.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -567,6 +581,13 @@ public function getFields($include_computed = TRUE) {
   +    return array_intersect_key($this->getFields(), $this->dirtyFields[$this->activeLangcode]);
   

   So this seems to account for the case that there is something in $this->dirtyFields that is not a field name? How can that happen? Or am I missing something?

6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -701,6 +722,9 @@ protected function updateFieldLangcodes($langcode) {
   +    $this->dirtyFields[$this->activeLangcode][$name] = TRUE;
   

   @hchonov brought up the interesting case of:

   $entity = $storage->load($entity_id);
   $entity->get('foo')->value === 'initial value';
   $entity->set('foo', 'some other value');
   $entity->set('foo', 'initial value');
   $entity->getDirtyFields();
   

   The question is whether 'foo' should be returned at that point. To fix this, we would somehow get the "original" item (not sure how, this could be tricky) and then compare with $items->equals($original_items). Since this would only result in "false negatives" (i.e. fields that are marked dirty that aren't actually dirty) and not "false positives" (i.e. fields that are not marked dirty even though they are) I personally think it would be fine to handle this case in a follow-up, but it would be nice to have confirmation that something like that would be accepted in a future patch. Not sure what your thoughts about this are.

7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -974,7 +998,9 @@ public function &__get($name) {
   +   * Always:
   +   * - uses default language
   +   * - notifies parent
   
   @@ -989,7 +1015,7 @@ public function __set($name, $value) {
   -        $this->fields[$name][$this->activeLangcode]->setValue($value);
   +        $this->fields[$name][$this->activeLangcode]->setValue($value, TRUE);
   

   This is the default value, so not sure why the setValue() call is changed.

8. +++ b/core/lib/Drupal/Core/Entity/EntityWithDirtyFieldsInterface.php
   @@ -0,0 +1,27 @@
   +interface EntityWithDirtyFieldsInterface extends FieldableEntityInterface {
   ...
   +  public function getDirtyFields();
   

   Discussed this with @Berdir, @hchonov and a head-shaking @klausi and @berdir, @hchonov and I agreed that instead of this new interface this should be added to ContentEntityInterface ;-)

Sorry, just realized that I totally missed comment #8, so I duplicated some points that @hchonov brought up when we personally talked about it. Just in case anyone is (rightly) confused.

Reading the reviews in #8 and #14 it looks like the reviewers think the patch can be updated to take into account their concerns. (though there is slight uncertainty about #14.3).

@WimLeers Looking at it in ignorance from the outside, it looks like the ball is in your court. Can you say more about in what way the next step is not clear to you?

About #14.3

The other (slightly more invasive) way would be to add a postCreateTranslation() method or similar, although that also seems non-ideal as then one could argue we would also need equivalent hooks for translation updating and deleting.

@tstoeckler, I would say it will be much better if we isolate this and introduce postCreateTranslation() instead of misusing the postCreate method which is intended to be called only after an entity is created. There is already a case in core where the concepts have been mixed-up causing some difficulties e.g. deleting an entity or only a translation will lead to calling the delete method on field items in both cases.

We already have hooks for creating and deleting translations - hook_entity_translation_insert and hook_entity_translation_delete :). For updating a translation the hook_entity_update is sufficient as there we get the entity in the translation in which the entity is being saved which over the FAPI is the entity in the language that has been edited.

Regarding entity creation we have:

• the create method in the storage
• the postCreate method on the entity
• the hook hook_entity_create

Regarding entity translation creation we have:

• the createTranslation method in the storage
• the hook hook_entity_translation_create

In order to make the Entity API consistent we just have to introduce the postCreateTranslation() method on the entity.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -254,7 +262,11 @@ protected function getLanguages() {
+    // @todo TEST COVERAGE FOR CHANGES IN THIS HUNK
+    if ($this->isDefaultTranslation()) {
+      $this->newRevision = TRUE;
+    }
+    $this->dirtyFields[$this->activeLangcode] = [];

Actually I am still not really sure what this is about. The dirty fields have to be emptied on the new translation object? Why would they not be empty on creating a new translation for that new language code?

Re #24: I don't think I have time to work on this patch myself at the moment. If I can allocate some time I will try to tackle the first part of #14 first to basically put this patch in a bit more context as that would be helpful in assessing the benefit and risk of this patch.

@WimLeers, could you please explain #22?

This is closely linked to the 'revision_translation_affected' functionality added by #2453153: Node revisions cannot be reverted per translation which I don't see mentioned here.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -157,6 +158,13 @@
+  protected $dirtyFields = [];

I don't like the term "dirtyFields", what's dirty about them? I had to ask what it meant. How about "changedFields" or "updatedFields"?

Ok, maybe I just need "get off the island" and learn new terminology.

Also, the problem with changed is that the fields might not have actually *changed*, maybe the same value was set again that was there before. So I think getChangedFields() would imply a functionality that doesn't actually exist.

(yes, onChange() is also not correct in that sense, but onDirty or onDirtyingField() is kinda... dirty ;))

This is a "touching" issue ;)

Also, the problem with changed is that the fields might not have actually *changed*, maybe the same value was set again that was there before.

Well that is not actually clear to me, to be honest. I had always thought that as part of tracking which fields are dirty we compare the to-be-set value with the current one and if they are equal, the field is not dirty? So basically my personal long-term plan was that once we have this API that at some point in the future that ContentEntityBase::hasTranslationChanges() could basically become super cheap, i.e.

public function hasTranslationChanges() {
  return !empty($this->dirtyFields[$this->getLangcode()]);
}

or something. And for that it would be important to track whether the field values actually changed. But maybe (apparently?) that's not what others are thinking? I would be interested to hear other ideas.

At least that's not what the patch is doing as far as I see and the only way to do that through onChange() would be to change all the field implementations to only call onChange() if they actually detect a change, which would be a BC change in theory (not sure if it would actually break something) and require changes in many field types.

So yes, that would be an interesting thing to have but I'm not sure how we could actually make that work :)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -990,7 +1016,7 @@ public function __set($name, $value) {
          if (isset($this->fields[$name][$this->activeLangcode])) {
   -        $this->fields[$name][$this->activeLangcode]->setValue($value);
   +        $this->fields[$name][$this->activeLangcode]->setValue($value, TRUE);
          }
          // If not, create one.
   

   $notify defaults to TRUE, why is this change necessary?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -135,6 +135,8 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
        $this->invokeHook('translation_create', $translation);
   +    // @todo TEST COVERAGE FOR CHANGES IN THIS HUNK
   +    $translation->postCreate($this);
   

   Interesting, we've had quite some discussions already about what should and shouldn't be called per-translation.

   See #2577609: Add per-entity-translation pre/post save methods (and hooks?) so that preSave() and postSave() implementations don't need to iterate all translations.

   There are bugs by not doing it, but I fear that adding this might also introduce new/different bugs.

   I only found two postCreate() implementations in all my entities and they're both not translatable, so I have no specific example of what could change.

3. +++ b/core/lib/Drupal/Core/Entity/EntityWithDirtyFieldsInterface.php
   @@ -0,0 +1,27 @@
   + * This interface extends the general fieldable entity interface. So that
   + * interface is not modified during the Drupal 8.x cycle.
   + *
   + * @todo Merge this interface with FieldableEntityInterface before Drupal 9.0.
   

   what we've done before is just add those methods to ContentEntityInterface with docs to move up in 9.x, but maybe only on issues that weren't committed yet because I can't find an example right now. (the argument is that fieldable interface is a generic interface that might have other implementations out there, while ContentEntityInterface is a 1:1 interface for ContentEntityBase)

#1926714: Track changes for entity property changes and make available to hooks is also related, maybe we can close that as a duplicate now. It clearly is very old :)

There is an issue about the problems with on change -
#2863818: Fix notification issues in the entity/field on-change system.

#38 seems to be suggesting 'touched' fields as opposed to 'dirty' fields, seems like a good suggestion: (https://en.wikipedia.org/wiki/Touch_(Unix))

@catch:

Yep, sorry for being cryptic :)

Btw, I share @tstoeckler's perplexity about the usefulness of this patch as it is currently. If I understand it correctly a regular entity form submission would mark every field for which we have a widget as dirty/touched/younameit, regardless of it being actually modified. Is this going to be used only for REST? I guess it couldn't be used to build a proper conflict management solution supporting merging of non-conflicting changes. If so, we will need another issue and a similar API to detect actual field changes. However I suppose that once we have the latter, that can address also the use cases for which we are introducing the former.

That said, I'm wondering whether the only reason to keep the current approach is that it's way easier to implement and it will allow to unblock/fix the issues in the OP way more quickly.

Great API addition. And: I share the point made in #37 / #39 / #46 that the current approach (back-and-forth changes that result in no change are still marked dirty) does not seem useful.

Why not simply do a sophisticated version of

array_keys(DiffArray::diffAssocRecursive($entity->toArray(), $entity->original->toArray()))

?

This would still be a really useful API addition. Let's begin with a reroll.
(comments triage)

• Remove the interface and add the method directly to ContentEntityInterface (#41.3, #14.8)
• Naming (touched vs dirty)
• Determine when a field is considered dirty, a initial change, current patch, or when the value is actually changed (#37, #39, #46)
• Adress translations, serialisation, cloning (#32)

This patch:

• Adresses #41.3, #14.8
• Adresses #14.2
• Adds tests for cloning

I did a small comment triage in #57, that I intend to work more on later.

Setting back to NW. This new patch adds more test coverage, ::testDirtyFields is expected to fail (due to suggested change in #37, #39, #46), so leaving at NW for now.

Actually I am still not really sure what this is about. The dirty fields have to be emptied on the new translation object? Why would they not be empty on creating a new translation for that new language code?

So while writing tests, I finally understood why ::postCreate was implemented here.

    $entity = EntityTestMul::create([
      'name' => $this->randomString(),
      'langcode' => 'en',
    ]);

    $entity->set('name', 'Test');
    $this->assertNotEmpty($entity->getDirtyFields());

    $translation = $entity->addTranslation('sv');

    $this->assertEmpty($translation->getDirtyFields());

::addTranslation calls ContentEntityStorageBase::createTranslation, which calls Entity::getTranslation() that returns the new translation object. However, ::createTranslation then sets new values on that newly created object which gets stored in dirtyFields array. So the current solution works around that by, emptying the array again.

(Needed a while to find this again, so added googlefood)