[PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work

1. +++ b/core/modules/rest/src/Serializer.php
   @@ -0,0 +1,121 @@
   +    $object = $this->serializer->denormalize($data, $class, $format, $context);
   ...
   +    $object->_restSubmittedFields = array_keys($data);
   

   This was only added in the ContentEntityNormalizer before, this will add this to every object (even though REST will not do that). I think it would be clearer about what is happening if all of that was inside the if()?

2. +++ b/core/modules/rest/src/Serializer.php
   @@ -0,0 +1,121 @@
   +  public function serialize($data, $format, array $context = array()) {
   

   The Serializer also has a final serialize() method, on top of deserialize which you already have as final.

I worked on a similar functionality as introduced by patch #6.
But for the use case of #2428795: Translatable entity 'changed' timestamps are not working at all this approach from #6 is too simple:

public function onChange($name) {
  $this->changedFields[$name] = TRUE;
}

The issue mentioned above needs to track changes per translation. Additionally a field should not be marked as changed until its value changed for real. (onChange seems to be called if you set a value again with its current value.)

Basically tracking of changed / dirty fields (changed values / additions / removals) is a good idea. The list of changed fields (per language) is essential to solve or ease the implementation of these issues at least:
#2297817: Do not attempt field storage write when field content did not change
#2465913: Write to the storage only affected entity field values
#2465901: [META] Make entity revision translation work
#2428795: Translatable entity 'changed' timestamps are not working at all

I integrated all the $changedFields stuff in my latest patch #42 for #2428795: Translatable entity 'changed' timestamps are not working at all and extended it to be "translation aware". If this one gets committed, the remaining patch here should look like the attached patch. I didn't break the API suggested here in #6.)

postponed for #2428795: Translatable entity 'changed' timestamps are not working at all

I found a way to "simplify" #2428795: Translatable entity 'changed' timestamps are not working at all, so that I don't need the tracking of changes anymore. Therefore the patch from #6 here is again the complete one ;-)

But during my tests I run into a different issue: #2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations.
That one is now a blocker for this issue as soon as the entity is translated!

I repeat my post from https://www.drupal.org/node/2474075#comment-9857739 to explain what happens.

Basically this doesn't work for translated entities:

public function postSave(EntityStorageInterface $storage, $update = TRUE) {
  parent::postSave($storage, $update);
  $this->resetChangedFieldList();
}

I'll try to explain why.
The patch by @alexpott introduces a new property for entities:

/**
  * The list of field names of any fields that have changed.
  *
  * @var string[]
  */
protected $changedFields = array();

If on purpose or not, this property is not one of the "synchronized" ones across the cloned translations in ContentEntityBase::initializeTranslation().
Therefor the tracking of changes in onChange() and the accessor getChangedFields() work separately on each translation (which is great BTW).

But as a consequence the resetChangedFieldList() method needs to called individually per translation!

If we keep the not obvious nature of methods like preSave() in combination with cloned translated entity objects we have two ways to fix the patch in #6:
1. synchronize the changedFields property across translations in initializeTranslation()
or
2. loop over all translations in postSave()

Nevertheless, many contrib developers might step into that trap when they add properties to custom content entities. Especially if they don't think about translation up front.

RE-rolled the patch from #6.

Test the re-roll.

@Jaesin: If you're working on this one, please read carefully through my comments in #2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations first.
Otherwise this solution might fail on translated entities.

Spoke to Alex, we agree this should be major now.

Rerolled #14

@mkalkbrenner I'm not sure how #2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations affects here. I was testing using your test but changing things:

  public function testOwner() {
    $user = $this->createUser();

    $container = \Drupal::getContainer();
    $container->get('current_user')->setAccount($user);

    // Create a test node.
    $english = Node::create(array(
      'type' => 'page',
      'title' => $this->randomMachineName(),
      'language' => 'en',
    ));
    $english->save();


    $italian = $english->addTranslation('it');
    $italian->setTitle('italian');
    $italian->setSticky(TRUE);
    $german = $english->addTranslation('de');
    $german->setTitle('german');
    $german->save();

    $this->assertEqual($user->id(), $german->getOwnerId());

  }

If I only save $german then here the results using debug() for the field keys.

Saving $german and then $italian from the test.

I'm not sure about the problem with postSave() for the current issue here. Maybe I'm missing something :)

Please can you add here a test or example about how #2474075: Fix Node::preSave() and document that preSave() and postSave() are not working with ContentEntity translations is affecting here?

Thanks!

The underlying design issue is that EntityResource::post() and ::patch() need to know which fields were present in the incoming request, which is known at an earlier point in the callstack, and in a separate API (EntityNormalizer::denormalize()). _restSubmittedFields is just a hacky way to pass the information between those two points in the callstack - which de-facto couples the Normalizer and the EntityResource.

As a possible fix : EntityResource::post() and ::patch() are called by \Drupal\rest\RequestHandler::handle(), which also passes the $unserialized data as a param (the post() and patch() methods currently just overlook the corresponding argument).
--> Could we deduce the "list of fields that were present in the incoming request" just by looking at the $unserialized data ?

Yes, I think I talked about something similar to this when the problem first came about. Handle this in rest only. I just want _restSubmittedFields out of serializtion module :) No way should it be anywhere near there.

The changed fields addition seems generally nice, and something most ORMs support, for example. @yched, is there something you don't like about that approach?

@yched $unserialized is the Entity. So, we can detect fields when denormalizing but not directly from $unserilized.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -166,6 +167,13 @@
   +  protected $changedFields = [];
   

   Note: this property is not synchronized across all translations of an entity.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1197,4 +1227,11 @@ public function hasTranslationChanges() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +    $this->resetChangedFieldList();
   +  }
   

   This implementation only resets $changedFields for the last translation edited of an entity while all other translations keep their list of changed fields after save. Due to the fact that an entity save always saves every translation, you need to iterate over all translations here to reset all lists.

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -145,18 +149,24 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      foreach ($entity->getChangedFields() as $field_name => $field) {
   

   I wonder if this tracking of changed fields is still required at all.
   The property ContentEntityBase::values "holds the original, unchanged values of the entity".
   Can't we just iterate over all fields and use FieldItemListInterface::equals() to detect the changed fields?

@marthinal : Oh, right, crap. We could do something with the denormalized *array* data, but the fully unserialized Entity is useless here.

The changed fields addition seems generally nice, and something most ORMs support, for example. @yched, is there something you don't like about that approach?

Just tricky debugging sessions with @fago a couple month ago which left me under the impression that the onChange() notification chain was complicated and brittle, so a general preference for solutions that avoid relying on it for critical stuff...

But that qualifies as FUD, probably. And we have no alternative approach here, so I'll shut up :-)

The changed fields addition seems generally nice, and something most ORMs support, for example. @yched, is there something you don't like about that approach?

I repeat my question from comment #24:
I wonder if this tracking of changed fields is still required at all.
The property ContentEntityBase::values "holds the original, unchanged values of the entity".
Can't we just iterate over all fields and use FieldItemListInterface::equals() to detect the changed fields?

@mkalkbrenner

The property ContentEntityBase::values "holds the original, unchanged values of the entity".

Hm - I'm not sure if that's a reliable, designed-that-way feature (I doubt it's guaranteed by non-regression tests, for example ?), nor if this is 100% guaranteed fo the entire lifecycle of an entity.

ContentEntityBase::values is protected, that's just internal to how content entities optimize the instanciation of the FieldItemList / FieldItem objects. I don't think it's intended for use by the outside world ?

Hm - I'm not sure if that's a reliable, designed-that-way feature (I doubt it's guaranteed by non-regression tests, for example ?), nor if this is 100% guaranteed fo the entire lifecycle of an entity.

But is FieldableEntityInterface::onChange() reliable?
If you set a field value on an entity, it triggers a setter which finally calls Map::onChange():

  /**
   * {@inheritdoc}
   *
   * @param bool $notify
   *   (optional) Whether to forward the notification to the parent. Defaults to
   *   TRUE. By passing FALSE, overrides of this method can re-use the logic
   *   of parent classes without triggering notification.
   */
  public function onChange($property_name, $notify = TRUE) {
    // Notify the parent of changes.
    if ($notify && isset($this->parent)) {
      $this->parent->onChange($this->name);
    }
  }

This method finally calls onChange() on the entity (parent). But the API allows to explicit prevent that notification.

On the other hand there are already todos in ContentEntityBase:

  /**
   * The plain data values of the contained fields.
   *
   * This always holds the original, unchanged values of the entity. The values
   * are keyed by language code, whereas LanguageInterface::LANGCODE_DEFAULT
   * is used for values in default language.
   *
   * @todo: Add methods for getting original fields and for determining
   * changes.
   * @todo: Provide a better way for defining default values.
   *
   * @var array
   */
  protected $values = array();

Isn't it better to implement @todo: Add methods for getting original fields and for determining changes instead of implementing another change tracking in parallel?

#29 Yep, TypedData's ::setValue($value, $notify = TRUE) allows the data to "react to the change but do not notify upstream". IIRC, that is intended for internal stuff like "modifying ERItem->entity needs to modify ER->target_id accordingly, and reversedly", where you don't want to fire the notification twice and end up in a loop. When used wrong, this can break the notification chain, yes. Then again, it's our official API for this kind of stuff :-)

Regarding the @todo about ContentEntityBase::$values :

   * @todo: Add methods for getting original fields and for determining
   * changes.

Yes, but It dates from the very first TypedData / EntityNG commit (heh, exactly 3 years ago), so not sure it's still relevant after all the code changes since then :-)
Also, $this->values is not fully immutable :
- __sleep() writes the actual FieldItemList values back into $this->values. Not sure if that affects the $entity after it's been serialized, or if that just affects the serialized data.
- removeTranslation() removes stuff from $this->values
- updateOriginalValues() writes to $this->values (intended to be called only after save())

So, between the two approaches... would be good to hear @fago's thoughts :-)

Wow, quite a significant WTF!

Assigning to @fago per #30.

Pinged @fago to get his feedback: https://twitter.com/wimleers/status/727145004804792324

Thanks for the ping wim!

The property ContentEntityBase::values "holds the original, unchanged values of the entity".

hm, in most case - yes. But I'm not sure that's always the case, nor was it designed or documented to work like that. It could very well be that some entity type implements logic - e.g. onChange() - that causes the values to be updated. And then there are cases like sleep() which write to it - as yched mentions. That said, I do not think we should rely on that.

Just tricky debugging sessions with @fago a couple month ago which left me under the impression that the onChange() notification chain was complicated and brittle, so a general preference for solutions that avoid relying on it for critical stuff...

But that qualifies as FUD, probably. And we have no alternative approach here, so I'll shut up :-)

hehe. We already use it some critical cases afaik, like for clearing static caches. It should be fine to use it for that also + we managed to solve those complicated cases. So time to take advantage of it now?

I generally like the approach take of the patch - although there are probably more special cases and docs we'd have to think about and figure out. E.g. - what are the changed fields of a created entity? (Probably all). Yeah, and as mentioned we need take care of updating all translations.

* @todo: Add methods for getting original fields and for determining
* changes.

Yes, onChange() was intended to allow for that - but we never got to implementing it.

A few remarks regarding the patch:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -532,6 +540,25 @@ public function getFields($include_computed = TRUE) {
   +    return array_filter($this->getFields(), function ($field) {
   

   No need to instantiate all field objects - better use getFieldDefinitions()

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -241,4 +241,19 @@ public function isValidationRequired();
   +   * Gets an array of changed field item lists.
   

   nitpick: a list - that the list is representated as array is an implementation detail.

3. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -241,4 +241,19 @@ public function isValidationRequired();
   +   *   An array of field item lists implementing, keyed by field name.
   

   docs need work

4. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -241,4 +241,19 @@ public function isValidationRequired();
   +   * Resets the list an array of changed field names.
   

   Needs work + explanation when it's valid to call that.

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -145,18 +149,24 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +        if ($field->isEmpty() && !$original_entity->get($field_name)->access('delete')) {
   ...
   +        if (!$original_entity->get($field_name)->access('update')) {
   

   I do not know why the patch changes the access calls here - that seems out of scope for this patch and is wrong. There is no such thing as delete or update access on fields, field access only knows view and edit operations.

oh, then the patches introduces an API change by adding those methods to the interface. I think we can avoid that by moving it into a new,optional interface, implemented by ContentEntityBase.

We really need to get this going again. We need this to be fixed.

Since #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, we have thorough test coverage that should catch BC breaks in this area. This is also closely related to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

For my own understanding, I rolled a debugging patch. With this applied, you can do something like:

$ php vendor/bin/phpunit -c core core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonCookieTest.php
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

Fstring(618) "string(15) "--- CURRENT ---"
string(5) "title"
string(15) "---------------"
string(11) "--- NEW ---"
string(4) "uuid"
string(36) "139ff7e7-9e6a-4c94-9b90-a772452f7710"
string(36) "5eebe165-30af-4e60-81e3-9cca51f7232c"
string(5) "title"
string(10) "Dramallama"
string(5) "Llama"
string(7) "created"
int(1487183557)
string(9) "123456789"
string(7) "changed"
int(1487183557)
string(9) "123456789"
string(18) "revision_timestamp"
int(1487183557)
string(9) "123456789"
string(45) "uuid,title,created,changed,revision_timestamp"
string(15) "---------------"
{"message":"Access denied on updating field \u0027created\u0027."}"


Time: 4.89 seconds, Memory: 6.00Mb

There was 1 failure:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonCookieTest::testPatch
Failed asserting that 403 is identical to 422.

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:342
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:366
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:836

FAILURES!
Tests: 1, Assertions: 27, Failures: 1.
wim.leers at acquia in ~/Work/d8 on 8.3.x*

and hence get clear output about the difference between the current algorithm in HEAD (the _restSubmittedFields hack we want to get rid of) versus what a sane implementation (using FieldItemList::isEmpty() and FieldItemList::equals()) would look like. My implementation is super naïve, but I'm kind of scared by the complexity in the prior patches.

Any takers?

#38 conflicted with #2751325: All serialized values are strings, should be integers/booleans when appropriate. Rerolled.

I've re-read this entire issue again.

Given that @fago wrote this in #34:

@todo: Add methods for getting original fields and for determining
* changes.

Yes, onChange() was intended to allow for that - but we never got to implementing it.

… I'm now thinking that that approach in the latest patch (#19) actually is okay. Despite me having written I'm kind of scared by the complexity in the prior patches in #38.

First: a straight rebase of #19. A lot has changed since then.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -259,37 +259,35 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-
-      if (!$original_entity->get($field_name)->access('edit')) {
-        throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
-      }
-      $original_entity->set($field_name, $field->getValue());

Oops, I didn't mean to delete this. I already fixed that locally but failed to add it to the patch.

The problem with the patch in #41 (aside from dealing with translations etc, that still needs to be done!) is that it even marks initial field values as "changed". The reason is two-fold:

1. $entity->$name = $values[$name]; in ContentEntityStorageBase::initFieldValues() uses the magical ContentEntityBase::__set setter, which sets a value with $notify = TRUE.
2. $entity->get($name)->applyDefaultValue(); in ContentEntityStorageBase::initFieldValues() uses \Drupal\Core\Field\FieldItemList::applyDefaultValue(), without setting the $notify parameter, so the default of $notify = TRUE is used

The solution is therefore:

1. Do not use the magical setter, but instead use the non-magical setValue(), so we can pass $notify = FALSE
2. Call applyDefaultValue() with $notify = FALSE
3. … and update the docs of the magical ContentEntityBase::_set() to state explicitly that it will use $notify = TRUE (optional change, but makes this less confusing going forward)

With this change… all REST tests seem to be passing :) (REST does not yet have entity translation support, nor entity translation test coverage.)

Forgot patch. Let's see how many Entity/Field API tests fail.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -259,37 +259,40 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +        // identify the entity. Therefore it does not make sense to modify any of
   

   Line exceeding 80 characters

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -259,37 +259,40 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +          // It is not possible to set the language to NULL as it is automatically
   

   Line exceeding 80 characters

Made changes as per the coding standard, please review.

@Pavan: If you see that an issue is clearly still in an early, experimental, researching phase… please do not reroll it in this incredibly unhelpful way. This is very annoying. Because it's very distracting.

1. #41 had 29 fails. 100% of EntityResourceTestBase tests, plus RestRegisterUserTest. Because it's checking field edit access for more fields than it should.
2. #44 has 52 fails.
   1. All the EntityResourceTestBase tests in the rest module now pass. YAY!
   2. But all of those in the hal module fail.
   3. And many other tests now fail

This should fix the failing EntityResourceTestBase tests in the hal module. Next: fixing the other failing tests — if at all possible.

(Interdiff relative to #44.)

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -156,6 +157,13 @@
   /**
+   * The list of field names of any fields that have changed.
+   *
+   * @var string[]
+   */
+  protected $changedFields = [];

This is useful for all kinds of things, not just Rest. I think there are also issues about this already. In any case, if this is needed for this to land, please extract it into a separate patch/issue and mark this as postponed, so it can be discussed/reviewed properly in its own right. It is quite an important change, but also a very risky one, so it needs a lot of thought.

20 HAL failures in #44. 5 in #49. That's a good step in the right direction.

Root cause analysis: part 1

While awaiting that, I was working on determining the root cause for the failing ContentEntityFieldMethodInvocationOrderTest test.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -102,14 +102,17 @@ protected function doCreate(array $values) {
   protected function initFieldValues(ContentEntityInterface $entity, array $values = [], array $field_names = []) {
+    // Do not notify when setting values, because these are initial values.
+    $notify = FALSE;
+
     // Populate field values.
     foreach ($entity as $name => $field) {
       if (!$field_names || isset($field_names[$name])) {
         if (isset($values[$name])) {
-          $entity->$name = $values[$name];
+          $entity->$name->setValue($values[$name], $notify);
         }
         elseif (!array_key_exists($name, $values)) {
-          $entity->get($name)->applyDefaultValue();
+          $entity->get($name)->applyDefaultValue($notify);
         }
       }

This is what's causing all those entity translation test failures, including the one for ContentEntityFieldMethodInvocationOrderTest: almost all those new test failures in #44 (and almost all remaining test failures in #49).

initFieldValues() is called from two places:

1. \Drupal\Core\Entity\ContentEntityStorageBase::doCreate()
2. \Drupal\Core\Entity\ContentEntityStorageBase::createTranslation

Root cause analysis: part 2

I had hoped that letting doCreate() call initFieldValues() with $notify = TRUE and letting createTranslation() call initFieldValues() with $notify = FALSE would solve the problem. But it did not.

Turns out that ContentEntityBase::onChange() does magic with translations. For example when you do:

  \Drupal\node\Entity\Node::create([
    'title' => 'yar',
    'type' => 'article',
    'langcode' => 'de',
  ]);
  $node->save();

create() creates a default translation (x-default) and assigns the three given values to the respective fields. Upon assigning langcode, ContentEntityBase::onChange() is called. The first case in its switch statement then detects that this is indeed the default translation (x-default) and then sets the default langcode to the value we passed: de.
From that point onward, all translation-related code will now correctly handle this default translation (x-default) as the de translation. So ContentEntityBase::getTranslation('de') will return it as expected.

All of this only works as long as ContentEntityBase::onChange() is called when setting the langcode field value, i.e. only if $notify = TRUE while setting that value.

Therefore we have no choice but to let $notify = TRUE. But that will again mean EntityResource::patch() will explicitly handle many fields as "updated"/"edited"/"modified"/"changed", despite them not having changed at all!

Solution

1. revert changes in ContentEntityStorageBase made by #44
2. call resetChangedFieldList() at the end of both doCreate() and createTranslation()

Caveats

Visibility of & caller of resetChangedFieldList()

Ideally, this would not be a public function, and it'd be called by ContentEntityBase() itself. And in fact, this is possible when calling Entity::create()! We can let ContentEntityStorageBase::postCreate() call the reset method :)

However, it becomes a bit more difficult when considering ContentEntityStorageBase::addTranslation(). But… ContentEntityBase::addTranslation() calls ContentEntityStorageBase::createTranslation(). createTranslation() is much like doCreate(), because both call initFieldValues(). Also, createTranslation() invokes hook_translation_create(). So… I think the solution in the end is quite obvious: createTranslation() should also call Entity::postCreate()!

What does changed mean?

Now, this seems to work, but I'm not sure that point 2 is entirely correct: a field that is created but differs from its default value technically is also "changed". At least, when you interpret "changed" with the semantic meaning defined in \Drupal\Core\Entity\FieldableEntityInterface::onChange() and \Drupal\Core\TypedData\TraversableTypedDataInterface::onChange().

However, what we need for the purpose of REST is not change to the field value relative to its default value, what we need is change to the field value relative to the stored value. So perhaps the "changed" in getChangedFields() and resetChangedFieldList() is not narrow enough. Perhaps what we want is "modified", which we then define as modified compared to the stored value.

Because "changed" is used for all sorts of TypedData-related magic, with automatic calculation of property values etc. See for example \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::onChange(). That's specifically not what we need.

Further evidence that "changed" in getChangedFields() is different: we're calling resetChangedFieldList() after we've called initFieldValues(), and hence after having called hook_entity_field_values_init() + hook_ENTITY_TYPE_field_values_init(). i.e. it's about changes compared to the initial values.

Does this belong on ContentEntityStorageBase or Entity?

I suspect this actually belongs on Entity… but we can be pragmatic, and first only add it to ContentEntityBase, and then perhaps move it up a level in the abstraction chain later.

Only the first caveat is addressed in this issue. This means I was able to remove FieldableEntityInterace::resetChangedFieldList(). That means disruption is smaller, and more importantly: we reduce the error surface area, because we prevent accidental or incorrect calls to this method!

Leaving caveats 2 and 3 for future rerolls. Now at least the thinking/concerns are documented while they're still fresh.

Note that the number of failing tests should remain the same. This is pure clean-up.

D'oh, forgot one hunk! Canceled the test of #54.

This apparently causes fields to be validated in a slightly different order. Easy fix.

Should bring it down to 6 failures.

Hm, let's revert #54 + #56 for now. Then we should really be at 6. I can revisit that clean-up later. Getting to 0 fails is more important right now.

One more small change to HAL tests, similar to the changes made previously in #57. Now really down to 6 failures.

Turns out all remaining REST failures (3 of the 6 remaining failures) have the same root cause as #2768651-85: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX! Importing the fix from there. Not importing the test coverage. Because I expect #2768651 to land long before this.

And the final 3 failures all have the same hilarious root cause!

\Drupal\user\Plugin\rest\resource\UserRegistrationResource::post() has this:

    // Only activate new users if visitors are allowed to register and no email
    // verification required.
    if ($this->userSettings->get('register') == USER_REGISTER_VISITORS && !$this->userSettings->get('verify_mail')) {
      $account->activate();
    }
    else {
      $account->block();
    }

    $this->checkEditFieldAccess($account);

This used to work fine because we were relying on _restSubmittedFields in checkEditFieldAccess().

But now we're relying on "changed field" tracking inside the entity object itself. Which means that $account->block() or $account->activate() cause the status field to be changed!

So the solution is simple: first call checkEditFieldAccess(), then block/activate the account!

Note that this is blocking #2824851 — see #2824851-61: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

#64 has a new/unexpected failing test: Drupal\Tests\serialization\Unit\Normalizer\EntityNormalizerTest. I wrote this in #64:

Not importing the test coverage. Because I expect #2768651 to land long before this.

Turns out I need to import those test changes too. Doing that here then.

Woot, green! All of the feedback from @fago in #34 still needs to be addressed. But now we at least now this is a workable solution.

Looking forward to some initial reviews.

Overall I think this is looking pretty good. It is so good to have this moving again, and to finally have a generic solution for 'changed' fields. Not needing to rely on the serializer - coupling REST, serialization, and entities more than we need to.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -566,6 +576,25 @@ public function getFields($include_computed = TRUE) {
   +  public function getChangedFields() {
   

   We use the word 'changed' in quite a few places in Drupal. I wonder if there are some better alternatives we could use here... A common terminology is to use 'dirty'. So 'getDirtyFields' or 'hasDirtyFields' etc.. That's just the first thing the sprang to mind, as a few other frameworks, like Laravel use similar. Other suggestions totally welcome!

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -566,6 +576,25 @@ public function getFields($include_computed = TRUE) {
   +  public function resetChangedFieldList() {
   

   I wonder if we need this method to be public? I see it is needed directly after an entity is saved and it gets persisted to storage.

3. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -171,10 +173,6 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   -    $entity->_restSubmittedFields = array_keys($data);
   

   YES PLEASE.

4. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Comment/CommentHalJsonTestBase.php
   @@ -37,13 +37,13 @@
   +    'entity_id',
   

   Ordering issues in the tests?

5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -259,37 +259,40 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if ($entity instanceof FieldableEntityInterface) {
   

   Yes, I think I noticed this before. That REST module was assuming all entities would be able to use this fieldable behaviour. Totally +1 on this.

   While we're here we could move this logic into another method to improve the readability a bit?

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   @@ -22,12 +23,14 @@
   -    foreach ($entity->_restSubmittedFields as $key => $field_name) {
   ...
   +      foreach (array_keys($entity->getChangedFields()) as $field_name) {
   

   This is just horrible... and this is waaaay nicer.

Regading @fago's comments in #34, It seems like a good idea, and correct to have all fields on a newly created entity as 'changed'. I guess our definition of changed is 'different to the persisted value'? Do we need to store the original values in another property on content entities maybe? Otherwise, we would have to load the original all the time? If we cannot rely on the $values property that is..

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -566,6 +576,25 @@ public function getFields($include_computed = TRUE) {
+  public function getChangedFields() {
+    // Filter the all fields to only fields that have changed.
+    return array_filter($this->getFields(), function ($field) {
+      /** @var FieldItemListInterface $field */
+      return isset($this->changedFields[$field->getName()]);
+    });
+  }

Not sure if it's simpler (to me it is), but this could just use an array_intersect_key() instead?

#72 :)

1. I agree getDirtyFields() is a much better name! Done.
2. I agree this should not be public.
   However, when I first tried to fix that, in #54, things failed horribly, so I reverted that in #60.
   Trying again now. (AFAICT #54 failed because it caused ->newRevision = TRUE to be set on every translation of the entity too, rather than only doing it for the default translation.
3. :)
4. Yes, these tests were actually meant to be very picky about the order, so that we'd know if the order changes. There's another issue that's making it insensitive to the order. In this issue, we should just reflect what the new order is.
5. Hm, I'm not sure how moving this check to a separate method would improve legibility? Isn't this check already as simple and clear as it possibly can be?
6. :)

#73: done.

Still to do: #34 + #72.5. Doing that next.

Apparently besides #34, there was also #35. Doing that first.

(Rebased this one. So patch should apply again.)

#34:
Per your recommendations, the patch is already not relying on ContentEntityBase::$values, and is instead relying on onChange().

1. But that means you won't be able to do things like:
   

   foreach ($entity->getDirtyFields() as $field_name => $field) {
     …
     if ($field->isEmpty()) {
   

   … so I'm not sure that what you're suggesting is actually better?

2. Fixed. This was c/p'ed from \Drupal\Core\Entity\FieldableEntityInterface::getFields(), so also fixed this there.
3. Fixed. This was c/p'ed from \Drupal\Core\Entity\FieldableEntityInterface::getFields(), so also fixed this there.
4. As of #74, this no longer exists :)
5. That change also is no longer in the patch, so nothing to change there either.

That still leaves the "track dirty fields per translation" stuff that was mentioned by @mkalkbrenner in #11 + #24 — I thought @fago also talked about it in #34, but he does not. Doing that next.

And this then makes the dirty field tracking per-translation.

Just read the patch, have only followed the last couple of comments, not read the whole issue:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -157,6 +158,13 @@
      /**
   +   * The list of field names of any fields that have changed.
   +   *
   +   * @var string[]
   +   */
   +  protected $dirtyFields = [];
   

   This should now mention the "per-translation" part, also the typehint is now no longer valid.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -254,7 +262,10 @@ protected function getLanguages() {
   -    $this->newRevision = TRUE;
   +    if ($this->isDefaultTranslation()) {
   +      $this->newRevision = TRUE;
   +    }
   

   If this is in fact necessary, it should be its own issue including dedicated test coverage.

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

   Why is the intersect needed? What would be in $this->dirtyFields that's not a valid field name?

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -989,7 +1014,7 @@ public function __set($name, $value) {
   -        $this->fields[$name][$this->activeLangcode]->setValue($value);
   +        $this->fields[$name][$this->activeLangcode]->setValue($value, TRUE);
   

   Again this needs dedicated test coverage.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -135,6 +135,7 @@ public function createTranslation(ContentEntityInterface $entity, $langcode, arr
   +    $translation->postCreate($this);
   

   Here, as well.

6. +++ b/core/lib/Drupal/Core/Entity/EntityWithDirtyFieldsInterface.php
   @@ -0,0 +1,27 @@
   +interface EntityWithDirtyFieldsInterface extends FieldableEntityInterface {
   

   I already mentioned this above, but this must definitely be its own issue. It is needed for a bunch of things that have nothing to do with Rest. And as you already found out with a bunch of the changes above this really is a tricky change, that needs a lot of thought and a lot of tests.

   Maybe we can tackle some of the things that I said should be split off above together with this, not sure.

7. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -259,37 +259,40 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if ($entity instanceof FieldableEntityInterface) {
   
   +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceAccessTrait.php
   @@ -22,12 +23,14 @@
   +    if ($entity instanceof FieldableEntityInterface) {
   

   This should be EntityWithDirtyFieldsInterface now?

#80

1. Done.
2. Yeah… that's for that new/separate issue. Added @todo.
3. Because $this->dirtyFields only contains booleans:
   

       // Add the field to the dirty fields list for this translation.
       $this->dirtyFields[$this->activeLangcode][$name] = TRUE;
   

4. This is not changing anything. TRUE is the default value for that parameter. I'm just making it explicit.
5. This is definitely a big change that needs test coverage. Added @todo
6. Yep, this definitely something that needs its own issue.
7. Yep, fixed!

Currently creating a new entity system issue for this.

This is blocked on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed).

Since this is also blocking #2824851, that issue is now blocked on two issues: #2824851-62: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

This is not a functional bug, this is cleaning up architectural/technical debt.

#2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX landed, so this is now unblocked!

This is the same patch as #81, minus the hunks in EntityNormalizer (added in #64) and in EntityNormalizerTest (added in #69). The changes to those files were imported from #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, and since that now landed, those hunks are effectively already committed :) The bugfix this was blocked on is committed!

Hurray, let's see if this is still green :)

3 failures. Just like some changes to CommentHalJson*Test were necessary, the same is true for NodeHalJson*Test.

#2863336: Default revision flag doesn't propagate to all entity translation objects added a new test (\Drupal\KernelTests\Core\Entity\ContentEntityCloneTest::testEntityPropertiesModifications()), which is failing due to the changes in this issue. Perhaps this is the test coverage that @tstoeckler was asking for in #80.2?

Ugh, I forgot to bump this from PP-1 to PP-2 in #82. This is still blocked on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed).

Unless I misread something, unassigning per #90.

Yep, sorry, I should've done that!

IS revamped.

Attempt to improve the title.

Adding a link to support one particular example in the IS.

I thought for a moment that #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior might help solve this without needing #2862574: Add ability to track an entity object's dirty fields (and see if it has changed), but AFAICT that's not actually possible.

I thought this might work:

     // Overwrite the received fields.
-    foreach ($entity->_restSubmittedFields as $field_name) {
-      $field = $entity->get($field_name);
+    foreach ($entity->getFields() as $field_name => $field) {
       $original_field = $original_entity->get($field_name);

… but it can't. Because that will even trigger for fields that weren't submitted. And I thought I could work around that by then doing

      if (!$original_field->equals($field)) {
        continue;
      }

in the loop, but that would effectively introduce the information disclosure security vulnerability that #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior so carefully avoided.

Therefore, this is still blocked on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed).

However, this definitely doesn't block #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior anymore. Since that wasn't the reason this was made Major (this was marked major in #18, 25 months ago), keeping this major for now.

Just to prove or unprove the reasoning in #98, I tried doing what I described I first thought could work and then thought could not work. Because #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior landed yesterday, so if I'm right, the approach in #98 should result in a failing test.

The change:

-    foreach ($entity->_restSubmittedFields as $field_name) {
-      $field = $entity->get($field_name);
+    foreach ($entity->getFields() as $field_name => $field) {

And it did fail (that's the original field value, the new field value, the response body, and the PHPUnit output, respectively):

Fstring(147) "array(1) {
  [0]=>
  array(1) {
    ["value"]=>
    string(1) "1"
  }
}
array(0) {
}
{"message":"Access denied on updating field \u0027nid\u0027."}"


Time: 5.98 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonBasicAuthTest::testPatch
Failed asserting that 403 is identical to 422.

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1035

But that's just for the nid field, which we're not sending. What if we do the naïve thing and ignore any fields whose new value is empty and original value is non-empty?

      if (!$original_field->isEmpty() && $field->isEmpty()) {
        continue;
      }

Then we fail on UUID, which is auto-generated:

string(282) "array(1) {
  [0]=>
  array(1) {
    ["value"]=>
    string(36) "24348f40-0970-40fd-8f37-f3792c191f97"
  }
}
array(1) {
  [0]=>
  array(1) {
    ["value"]=>
    string(36) "e13bebc3-7c20-419d-a58a-f48a6b31fb0f"
  }
}
{"message":"Access denied on updating field \u0027uuid\u0027. 2 "}"


Time: 5.82 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonBasicAuthTest::testPatch
Failed asserting that 403 is identical to 422.

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1035

What if we also explicitly ignore changes to the UUID field, because that can never be changed? Then we fail on the revision timestamp, which is also auto-generated:

string(234) "array(1) {
  [0]=>
  array(1) {
    ["value"]=>
    string(9) "123456789"
  }
}
array(1) {
  [0]=>
  array(1) {
    ["value"]=>
    int(1503484047)
  }
}
{"message":"Access denied on updating field \u0027revision_timestamp\u0027. 2 "}"


Time: 5.82 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonBasicAuthTest::testPatch
Failed asserting that 403 is identical to 422.

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1035

Conclusion: we really need to have some way to know which fields have been sent/changed.