Deep serialization for content entities to make an exact snapshot of an entity object's structure based on its current state

Chatted with @hchonov about this a bit yesterday. Some notes...

• Since ContentEntityBase::__sleep() "backs up" whatever is returned from FieldItem(List)::getValue() I looked into EntityReferenceItem::getValue() as I thought if that returned the entity property this problem would be solved. It actually does, but only if the entity is new, so this doesn't help as of now
• I think I remember some issue trying to change EntityReferenceItem::getValue() so that it returns the entity property under some other condition (or always?). Will try to find that
• It is probably too much of a BC break to always return the entity property unconditionally in EntityReferenceItem::getValue()
• @hchonov brought up that something like a $has_computed flag in getValue() would help here. Funnily, FieldItemList::getValue() already declares such a parameter, even though TypedDataInterface::getValue() does not. So it should perhaps be possible to add such a parameter to EntityReferenceItem::getValue() if it helps
• @hchonov mentioned that even if we had a $include_computed flag, we would have to explicitly call that from ContentEntityBase::__sleep() which could be considered a behavior change

Some archeology...

Ahh, somehow missed the last patch. That looks pretty sweet!

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -189,7 +189,7 @@ public function setValue($values, $notify = TRUE) {
-        if (!$this->entity->isNew() && $values['target_id'] !== NULL && ($entity_id !== $values['target_id'])) {
+        if (!$this->entity->isNew() && $values['target_id'] !== NULL && ($entity_id != $values['target_id'])) {

Why this change?

I think that the form changes should be moved into #2824293: Inconsistent form build between initial form generation and the first ajax request. I think it's unfortunate that the patch introduces knowledge about the entity system. Maybe we can try setting the $serializeComputedFieldValues flag somewhere in ContentEntityForm. But let's figure that out independently of the base issue that is being solved here.

Edit: Fix issue link

So I was thinking about whether we could make the $include_computed = TRUE case the default and what implications that would have. I couldn't come up with a concrete case of anything breaking, but that's obviously not saying much in the diverse wonderland that the Entity Field API is in core and contrib.

In any case, sending a patch that does that to the bot to see if anything breaks. If it's red, we'll know for sure that things break. If it's green that obviously doesn't mean nothing breaks, but that if anything breaks it's either not tested or in contrib.

Meh, that's annoying. But OK. Let's see.

So I guess what is happening with the patch is that the node type object ends up in the cached response instead of just its ID and on a page cache hit, the response gets unserialized, but system.module is not loaded. So when the NodeType instance gets constructed it fails because NodeType::$preview_mode defaults to DRUPAL_OPTIONAL.

That means two things:
1. I think it is a big problem that node type objects (or any objects whose classes rely on *.module files) cannot be part of response objects due to page cache. That has nothing to do with this issue, though.
2. This is not a viable approach, so we should go back to something like #10 but probably document some of the problems described above.

Uploading a patch just to test/prove my theory.

So I had opened #2825801: Node type objects (and other objects that depend on *.module files) cannot be part of response objects due to Page Cache: Document or fix!, but closed that as duplicate in the meantime, mainly on #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. Thinking about that more, I think the test failure does not actually point to a super valid issue, it's just a very unfortunate combination of edge cases and code debt. So if we wanted to go with this approach we could postpone this on (either) #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty (or #2807961: Replace usages of deprecated global constants with the new interface constants).

What do others think? Is this something we can get away with in Drupal 8?

My motivation for pursuing a non-flag approach is that it simply removes one more (albeit tiny) layer of complexity. Although the patch itself looks great and I can't think of any concrete problems, the combination of entity references, translations, entity reference revisions, ... and this flag getting lost (or turning up unexpectedly) somewhere in the mix if only due to a bug in contrib, kind of scares me a bit. I think it is absolutely necessary to fix this for modules like Inline Entity Form/Paragraphs and others to retain/reclaim some of their sanity, so if we can't get away with the behavior change, then I'm all for a flag, but I'd like to make sure that we actually need it.

Re #25:
1. Yes, that is a very good point and we need to update ContentEntityStorageBase::setPersistentCache() to account for that.
2. That also is a pretty strong case for not making this the default behavior, but somehow optional, like with the flag in the previous approaches. If you are using entity references for "embedding" entities - like the use-case of the Paragraphs module - then this is not a problem at all, you will just get a lot of cache tags if you heavily nest your entities. But in non-embedded use-cases of entity reference this could be a problem. If you have a setup of Music Genre -> Artist -> Album -> Song and you have a "listen_count" field on the Song, this would end up invalidating the entire Music Genre every time someone listens to a song in that genre. That's definitely not what you want in that case.

+1 Testing

Talked a lot with @hchonov about this last week. Some thoughts

• After thinking and talking about #26 a fair bit I realized that a single entity might actually have both "types" of references. I.e. the "Music genre" entity in #26 might also have a Paragraphs-based "Description" field, with embedded entities. So semantically the differentiation of whether or not to embed entities is not correct on the entity-level. It is actually a property of the reference itself. That doesn't necessarily mean that the entity-level flag is completely out of the question, in my opinion, but we should keep that in mind
• Advancing on the previous point we considered making the "embed or not" flag a setting on the entity reference field. That way InlineParagraphsWidget and InlineEntityFormWidget in contrib could check whether a field definition has that flag and only expose themselves to "embedabble" entities. That would allow #2824293: Inconsistent form build between initial form generation and the first ajax request to be solved fairly cleanly, I guess. The challenge would be to somehow expose this flag in the UI without completely freaking people out, but I'm leaving that aside for now.
• #27 brings up a good point, that serializing entities together with their "embedded" entities would mean that we have to invalidate the cache of entities when their embedded entities change. Thus, ContentEntityStorageBase::setPersistentCache() would have to add cache tags for all embedded entities. I was thinking something like the following pseudo-code:
  

  interface EmbeddableEntityReferenceFieldItemListInterface extends EntityReferenceFieldItemListInterface {
  
     public function embeddedEntities();
  
  }
  
  // In EntityReferenceFieldItemList
  class EntityReferenceFieldItemList extends FieldItemList implements EmbeddableEntityReferenceFieldItemListInterface {
  
   ...
  
    public function embeddedEntities() {
      if ($this->getSetting('embed')) {
        return $this->referencedEntities();
      }
      return [];
    }
  
  }
  
  // In ContentEntityBase::__sleep()
          if ($field instanceof EmbeddedEntityReferenceFieldItemListInterface) {
            foreach ($field->embeddedEntities() as $delta => $embeded_entity) {
              $this->values[$name][$langcode][$delta]['entity'] = $embedded_entity;
            }
          }
  
  // In ContentEntityStorageBase::setPersistentCache()
        foreach ($entity as $field_name => $items) {
          if ($items instanceof EmbeddedEntityReferenceFieldItemListInterface) {
            foreach ($items->getEmbeddedEntities() as $embedded_entity) {
              $cache_tags += $this->buildCacheTags($embedded_entity);
            }
          }
        }
  
  

• While the performance benefit mentioned in #27 is nice, that's not the reason for this issue. We really need this to solve #2824293: Inconsistent form build between initial form generation and the first ajax request. And that's not even talking about #2820235: Add support for inline entity forms. The issue title is precisely correct: We need a way to store the state of an entity across different requests. That is currently utterly broken for embedded entities.

Yes, this can be major in light of that.

So the idea here is to always rebuild the form with the entity that has been used the first time the form has been generated, but if we do not serialize computed values (entity references) then on wakeup the entity might differ from the initially used entity in case e.g. referenced entities have changed - this is for the case when dealing with nested entity forms really important. This means we have to cache the whole entity structure within the form state as well.

Right, we need to be able to preserve nested entities containing changes. Right now, we also loose those changes during rendering as I described in #2831727: [PP-1] Prepare-view of entity references silently drops changes contained in referenced entities.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -407,9 +425,74 @@ protected function clearTranslationCache() {
   +  protected function getFieldDefinitionsFilteredBySetting($setting, $value) {
   

   Nice abstraction!

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -407,9 +425,74 @@ protected function clearTranslationCache() {
   +    return $this->fieldDefinitionsFilteredBySetting[$setting][$value];
   

   I'm wondering whether we really have to cache the result. Serializing an entity at least is a rare and just one-time operation, isn't it?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -407,9 +425,74 @@ protected function clearTranslationCache() {
   +      foreach ($this->getFieldDefinitionsFilteredBySetting('serialize_embedded_entities', TRUE) as $name => $definition) {
   

   This flag makes much more sense ... better than what was discussed in IRC before, sorry. I just read the issue summary.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -407,9 +425,74 @@ protected function clearTranslationCache() {
   +          // Get the values together with the referenced entities and flag the
   +          // referenced entities for deep serialization as well.
   +          $this->values[$name][$langcode] = $field->getValue(TRUE);
   +          foreach ($this->values[$name][$langcode] as $delta => $values) {
   +            // We can assign protected properties of other objects only if they
   +            // have the same class as ancestor.
   +            $referenced_entities = array_filter($values, function ($val) {return is_object($val) && is_subclass_of($val, ContentEntityBase::class);});
   +            foreach ($referenced_entities as $referenced_entity) {
   +              $referenced_entity->serializeCompleteEntityStructure = TRUE;
   +            }
   +          }
   

   I'm wondering whether we could move some of this logic onto the field item list level etc., so its overridable on a field type level? This would allow us to probably get rid of this specific check for entity reference fields.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -427,6 +510,24 @@ public function __sleep() {
   +    $this->serializeCompleteEntityStructure = FALSE;
   

   Can we document this line? At least for me this is not entirely obvious of why this has to happen here.

6. +++ b/core/modules/field/field.install
   @@ -104,3 +105,39 @@ function field_update_8003() {
   +        $settings['serialize_embedded_entities'] = is_subclass_of($class, FileItem::class) ? TRUE : FALSE;
   
   +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -49,6 +49,7 @@ public static function defaultFieldSettings() {
   +      'serialize_embedded_entities' => TRUE,
   

   Can we document why we need a different behaviour for file items?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -546,8 +474,14 @@ public function __wakeup() {
       */
      public function serializeWithCompleteStructure() {
   +    // When running a deep serialization the flag
   +    // "serializeCompleteEntityStructure" has to be set in order to serialize
   +    // referenced entities as well, which are referenced by field having the
   +    // setting "serialize_embedded_entities" set to TRUE.
        $this->serializeCompleteEntityStructure = TRUE;
        $serialized = serialize($this);
   +    // After the deep serialization is ready we have to unset the flag in order
   +    // to allow for normal serialization afterwards.
        $this->serializeCompleteEntityStructure = FALSE;
        return $serialized;
      }
   

   Here is a question, do we need some recursive reference protection? I know there is a module for that: https://www.drupal.org/project/entity_reference_validators but I'm wondering whether we need that kind of production for serialize as well

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -109,7 +109,8 @@ protected function doTestSerialization($deep_serialization, $field_name) {
   -    $entity_level_zero = unserialize($entity_level_zero->serializeWithCompleteStructure());
   +    $serialized_entity = $entity_level_zero->serializeWithCompleteStructure();
   +    $entity_level_zero = unserialize($serialized_entity);
   

   This change seems unnecessary :)

I have one architectural issue with the patch:

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -156,6 +156,13 @@
   /**
+   * Whether to serialize the complete entity structure.
+   *
+   * @var bool
+   */
+  public $serializeCompleteEntityStructure = FALSE;

@@ -454,6 +461,34 @@ public function __sleep() {
+  /**
+   * {@inheritdoc}
+   */
+  public function serializeWithCompleteStructure() {
+    // When running a deep serialization the flag
+    // "serializeCompleteEntityStructure" has to be set in order to serialize
+    // referenced entities as well, which are referenced by field having the
+    // setting "serialize_embedded_entities" set to TRUE.
+    $this->serializeCompleteEntityStructure = TRUE;
+    $serialized = serialize($this);
+    // After the deep serialization is ready we have to unset the flag in order
+    // to allow for normal serialization afterwards.
+    $this->serializeCompleteEntityStructure = FALSE;
+    return $serialized;
+  }

I don't like the fact that for the top-level entity we have a dedicated method and then for nested entities we just set the flag ourselves and rely on the recursive nature of serialize(). I was thinking of how to avoid this, and I thought that maybe we can provide a dedicated method on the field item list. See this example code for what I mean:

class ContentEntityBase {

...

  public function __sleep() {
    ...
    if ($this->serializeCompleteEntityStructure) {
      $this->values[$name][$langcode] = $field->getSerializationValue();
    }
    else {
      $this->values[$name][$langcode] = $field->getValue();
    }
    ...
  }

}

class EntityReferenceFieldItemList {

...

  public function getValue($include_computed) { ... }

  public function getSerializationValue() {
    $values = $this->getValue($this->getFieldDefinition()->getSetting('serialize_embedded_entities'));
    foreach ($values as $delta => $item_values) {
      $values[$delta]['entity']->serializeCompleteEntityStructure = TRUE;
  }

}

This way the property value would be read exclusively by ContentEntityBase::__sleep().

We could then consider making it protected again by providing a setter for it. No getter would be needed.

I have a couple more notes, but I think this would be the most important issue to discuss. Here the rest of the notes, though for reference:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -156,6 +156,13 @@
   +  public $serializeCompleteEntityStructure = FALSE;
   
   @@ -454,6 +461,34 @@ public function __sleep() {
   +  public function serializeWithCompleteStructure() {
   

   I think the name of the property and method is not ideal. It implies that not using this method yields something that has an incomplete structure, or something. I think serializeWithEmbeddedReferences would be more explicit, although that's also very long, if not even longer.

2. +++ b/core/lib/Drupal/Core/Field/EntityReferenceInlineWidgetBase.php
   @@ -0,0 +1,19 @@
   +abstract class EntityReferenceInlineWidgetBase extends WidgetBase {
   
   +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -27,7 +27,7 @@
   -class FileWidget extends WidgetBase implements ContainerFactoryPluginInterface {
   +class FileWidget extends EntityReferenceInlineWidgetBase implements ContainerFactoryPluginInterface {
   

   I absolutely think this makes sense. The file widget is a form of an inline entity form. When we talked about all this we realized depending on how this embedded serialization will be used in form_state / entity cache / ... the one thing that could become problematic is if "embedded" entities are re-used. This is not possible e.g. with Paragraphs, but it can be possible with the more generic Inline Entity Form. With the FileWidget in core, though, this is also not possible, so it makes it a perfect candidate in my view.

   However, I think this change significantly increases the complexity of the patch. Especially, since the method is unused as of now, it really doesn't provide any value yet to do this change. So I would propose moving these changes to a follow-up.

I really like the new name. Just like a deep copy, you do a deep serialization here.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -156,6 +156,13 @@
    
      /**
   +   * Whether to serialize the complete entity structure.
   +   *
   +   * @var bool
   

   ... I think it would be nice to mention that this also contains referenced entities.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -74,4 +74,23 @@ public function getLoadedRevisionId();
   +  /**
   +   * Returns a deeply serialized entity.
   +   *
   +   * @return string
   +   *   The deeply serialized entity.
   +   */
   +  public function deepSerialize();
   +
   +  /**
   +   * Flags the entity for deep serialization.
   +   *
   +   * After calling this method and serializing the entity e.g.
   +   * "serialize($entity);" a deep serialization will be performed and the flag
   +   * will be automatically reset.
   +   *
   +   * @return $this
   +   */
   +  public function setDeepSerialization();
   

   I'm wondering whether these methods actually belong onto the FieldableEntityInterface

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -340,6 +341,14 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +    $form['serialize_embedded_entities'] = array(
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Serialize entity references together with the parent entity when performing deep serialization of the parent entity.'),
   +      '#description' => $this->t('If the field widget to be used for this field is an "entity reference inline widget" then the setting must be set.'),
   +      '#default_value' => $field->getSetting('serialize_embedded_entities'),
   +    );
   

   I'm really wondering whether its a good idea to expose this as a UI element. For me this reads like something you should be able to just handle in YML files directly

EDIT: Crosspost. Not much duplication though, except that I agree with #63.3

Thanks @hchonov, I agree with you, this is looking quite nice now. I like it a lot! Thank you very much for your efforts on this!

I only have some minor points now, nothing architectural:

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceInlineWidgetBase.php
   @@ -0,0 +1,19 @@
   +abstract class EntityReferenceInlineWidgetBase extends WidgetBase {
   

   I guess this is no longer needed either, right?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -340,6 +341,14 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   +    $form['serialize_embedded_entities'] = array(
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Serialize entity references together with the parent entity when performing deep serialization of the parent entity.'),
   +      '#description' => $this->t('If the field widget to be used for this field is an "entity reference inline widget" then the setting must be set.'),
   +      '#default_value' => $field->getSetting('serialize_embedded_entities'),
   +    );
   

   Forgot to mention this earlier, but what do you think about making this a "hidden" setting for now? and introducing a UI setting in a follow-up? I think it would make sense to introduce this together with an actual use-case (i.e. FileWidget or something). Also, it's kind of "unfair" ;-) right now to potentially confuse people with these details if it currently doesn't actually do anything.

   As far as I know contrib/custom module could alter this in, as well, and it should be properly saved to the field settings, so not doing this (for now) shouldn't block anything.

3. +++ b/core/modules/field/field.install
   @@ -104,3 +104,39 @@ function field_update_8003() {
   +  $config = \Drupal::configFactory();
   

   Let's call this $config_factory to avoid confusion.

4. +++ b/core/modules/field/field.install
   @@ -104,3 +104,39 @@ function field_update_8003() {
   +    $field = $config->getEditable($field_id);
   

   For clarity I think this should be called $field_config.

5. +++ b/core/modules/field/field.install
   @@ -104,3 +104,39 @@ function field_update_8003() {
   +    if ($class == EntityReferenceItem::class || is_subclass_of($class, EntityReferenceItem::class)) {
   

   Can we not simply check $field['field_type'] === 'entityreference'?

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -0,0 +1,119 @@
   +  protected $entityTypeId;
   ...
   +    $this->entityTypeId = 'entity_test_mul';
   

   Minor, could be initialized as the default value directly.

7. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -0,0 +1,119 @@
   +    $this->entityTestMulStorage = $this->entityManager->getStorage($this->entityTypeId);
   

   Since we abstract away the entity type ID in a variable, I think can just be called entityStorage

8. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -0,0 +1,119 @@
   +  public function testSerialization() {
   +    $field_name = 'field_test_entity_reference';
   +    // Create the test entity reference field.
   +    FieldStorageConfig::create([
   +      'field_name' => $field_name,
   +      'entity_type' => $this->entityTypeId,
   +      'type' => 'entity_reference',
   +      'settings' => [
   +        'target_type' => $this->entityTypeId,
   +      ],
   +    ])->save();
   +    FieldConfig::create([
   +      'entity_type' => $this->entityTypeId,
   +      'field_name' => 'field_test_entity_reference',
   +      'bundle' => $this->entityTypeId,
   +      'translatable' => TRUE,
   +    ])->save();
   +
   +    $this->doTestSerialization(FALSE, $field_name);
   +    $this->doTestSerialization(TRUE, $field_name);
   +  }
   ...
   +    $this->assertEquals($deep_serialization ? 'entity level one' : $initial_entity_name, $entity_level_zero->$field_name->entity->label());
   +    $this->assertEquals($deep_serialization ? 'entity level two' : $initial_entity_name, $entity_level_zero->$field_name->entity->$field_name->entity->label());
   

   Also pretty minor, and also arguable (so feel free to leave as is), but this feels a bit like the creation of the field config + storage should be in setUp() and the two doTestSerialization() should in fact be two separate test methods, i.e. testSerialization() and testDeepSerialization(). ($field_name could be another property on the test, next to entityTypeId then.) You could then put the entity setup in a separate createNestedEntities() method (or similar) to avoid duplication. In my personal opinion that would make the test a bit more readable, but again, feel free to ignore, it's not important.

9. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -0,0 +1,119 @@
   +    $entity_level_zero->name = 'entity level zero';
   +    $entity_level_zero->$field_name->entity->name = 'entity level one';
   +    $entity_level_zero->$field_name->entity->$field_name->entity->name = 'entity level two';
   

   As this seems to be "the heart" of the test, I think it could use a comment. Generally it's best to describe why something is happening, not what is happening, but this might be an exception. Because it's very easy to miss that there is no ->save() call below I think simply saying "Change the entity values in memory but do not persist them to storage." or something like that would be helpful.

10. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
    @@ -0,0 +1,119 @@
    +    $this->entityTestMulStorage->resetCache();
    

    Very interesting. Can you explain why this is needed? (Preferably with a code comment instead of in this issue ;-))

#64.5 What about fields of type EntityReferenceRevisions and others extending EntityReference?

Hmm... that's a good point, I didn't think about this. So this would in go update all image and file fields as well. But it would also update e.g. entity reference revisions fields. I'm not sure that's a good thing, though. Maybe so, because otherwise it would kind of be unfair, that Entity Reference Revisions has to create its own update path just because it is calling parent::defaultSettings(). On the other hand it does seem strange to automatically update contrib field types. Not sure what others think, but I guess let's leave it as it is for now.

#63.3 This is not possible, as if a site builder is configuring the entity reference fields for a module like paragraphs or inline entity form she should be able of using this feature as those are just regular entity reference fields and we could not put the setting into there. We also have defined an abstract Widget for inline entity forms checking for this setting of the field.

Well, that's the thing. Keep in only in YAML for now, because otherwise you would have to deal with userexperience in this issue. I think conceptually it would be better to have enough time to discuss the UX of this particular flag, so do the UI in 8.4.x but add the functionality for itself in 8.3.x

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -205,12 +205,12 @@ public function setValue($values, $notify = TRUE) {
   +    if ($include_computed || $this->hasNewEntity()) {
          $values['entity'] = $this->entity;
        }
   

   I think the $include_computed part should be !$this->isEmpty() && $include_computed otherwise $this->entity can be NULL, as far as I can tell and this can lead to errors down the road.

I like that the patch got simpler. On the other hand, I'm not deep into the entity system to really be able to judge this change properly.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
@@ -108,8 +108,10 @@ protected function doTestSerialization($deep_serialization) {
+    $entity_level_zero = unserialize(serialize($entity_level_zero));

Can you put some documentation why this is needed?

Thank you @hchonov
I really like how simple the solution now looks like. It would be great if some additional entity subsystem maintainer could have a look at it though.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -157,6 +157,16 @@
   +   * Whether to serialize the complete entity structure.
   +   *
   +   * If set to TRUE when the entity is being serialized a deep serialization
   +   * will be performed and the flag will be automatically set back to FALSE.
   

   Like with the setDeepSerialization() method, we need to better explain what a "deep serialization" is.

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -236,4 +236,19 @@ public function isValidationRequired();
   +   * After calling this method and serializing the entity e.g.
   +   * "serialize($entity);" a deep serialization will be performed and the flag
   +   * will be automatically reset.
   

   I think we need to explain what a "deep serialization" is, e.g. it includes computed properties or something like that.

   And maybe some usage examples as well :)

3. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -236,4 +236,19 @@ public function isValidationRequired();
   +   * @param bool $deep_serialization
   +   *   TRUE if deep serialization should be performed when the entity is being
   +   *   serialized, FALSE otherwise. Defaults to TRUE.
   

   This parameter needs to marked as (optional) at the beginning of the description, but I actually think that we should drop the default TRUE value in order to make the calling code more explicit.

4. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,14 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   * @return mixed
   +   */
   +  public function getSerializationValue($deep_serialization);
   

   Missing description for @return.

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -208,12 +208,12 @@ public function setValue($values, $notify = TRUE) {
   +    if (($include_computed && !$this->isEmpty()) || $this->hasNewEntity()) {
   

   In order to increase performance and not try to compute the 'entity' property of the field item, it's better to put the hasNewEntity() check in front.

   Actually, hasNewEntity() has the same problem and it can be optimized a bit, but that's not in the scope of this patch :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -208,12 +208,12 @@ public function setValue($values, $notify = TRUE) {
   +  public function getValue($include_computed = FALSE) {
   

   The $include_computed param is not coming from the interface so it is not actually documented anywhere :)

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -208,12 +208,12 @@ public function setValue($values, $notify = TRUE) {
   -    if ($this->hasNewEntity()) {
   +    if (($include_computed && !$this->isEmpty()) || $this->hasNewEntity()) {
   

   This needs to be turned into a more generic approach. For example, LanguageItem also has a computed property ('language') which doesn't receive this special treatment and therefore will not be part of the "deep serialized" structure.

Nice, that the patch is now a lot simpler. I am not super happy that we are unconditionally including all entities (so e.g. type, uid and revision_uid of each referenced entity...) but I think the trade-off of the reduced complexity is worth it. And I don't see any reason why that would break anything, it's just that a bit more data is serialized than strictly speaking would be needed.

Some thoughts on the patch:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -71,4 +71,34 @@ public function getLoadedRevisionId();
   +   * Example usage:
   +   * -Alter the title of a referenced entity and serialize the parent entity
   +   * through deep serialization in order to serialize the change made on the
   +   * referenced entity.
   +   * @code
   +   *   $entity->entity_reference_field->entity->setTitle('new entity reference title');
   +   *   $entity->setDeepSerialization(TRUE);
   +   *   serialize($entity);
   +   * @endcode
   

   Yay, thank you for this! Minor nitpick: Add a space after the "-"

2. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -13,6 +14,25 @@ class EntityReferenceFieldItemList extends FieldItemList implements EntityRefere
   +    if ($deep_serialization) {
   +      $values = $this->getValue(TRUE);
   ...
   +    }
   +    else {
   +      $values = parent::getSerializationValue($deep_serialization);
   +    }
   

   As far as I can tell, the $this->getValue(TRUE) call is equivalent to the parent call. So (unless I'm mistaken) I think it would be clearer if we do $values = parent::getSerializationValue($deep_serialization) before the if-else.

3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -101,6 +101,43 @@ protected function getSetting($setting_name) {
   +    // Include computed properties if the condition of
   +    // ::includeComputedProperties() is fulfilled.
   +    if ($this->includeComputedProperties($include_computed)) {
   

   I think the interesting part of this is that we already pass in $include_computed but we allow field items to (optionally) override that. So maybe the comment could be something like "Allow field types to opt-in or opt-out of including computed properties in certain cases."

4. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -101,6 +101,43 @@ protected function getSetting($setting_name) {
   +   * Helper method for ::getValue() to check if included properties have to be
   +   * included.
   

   Maybe s/have to be/should be ?

   Super minor, but would also be nice to add a @see to ::getValue() to this method. When looking at the code it's not necessary, because it's just above it, but on api.drupal.org it will be helpful for people.

5. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -101,6 +101,43 @@ protected function getSetting($setting_name) {
   +    return $include_computed && !$this->isEmpty();
   

   Would be nice to add a comment here. Something like: "Even if computed fields were requested, do not attempt to compute anything if ther are no actual values." ?

6. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,18 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   *   Whether a deep serialization is running.
   

   Instead of "is running" I think "should be performed" would be clearer.

7. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,18 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   * @return mixed
   

   This should be array right? (Or even array[] I guess?)

8. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,18 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   *   Depending on whether deep serialization is running or not the returned
   ...
   +   *   serialization is not running or additionally contain computed properties
   +   *   next to the data values if deep serialization is running.
   

   Here, as well, I think "being performed" would be nicer than "is running".

The patch looks much better now, thanks for addressing the reviews so quickly!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -71,4 +71,34 @@ public function getLoadedRevisionId();
   +   *   serialized, FALSE otherwise. Defaults to TRUE.
   

   Since the parameter is not optional anymore, there is no default :)

2. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -101,6 +101,47 @@ protected function getSetting($setting_name) {
   +        $definition = $property->getDataDefinition();
   +        if ($definition->isComputed()) {
   

   We are not using $definition again so it doesn't look very useful to assign a variable for it in the first place.

3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -101,6 +101,47 @@ protected function getSetting($setting_name) {
   +   *   TRUE if computed properties have to be included. FALSE otherwise.
   

   "FALSE otherwise" is not a sentence on its own, so we should replace the period before FALSE with a comma ;)

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -208,15 +208,10 @@ public function setValue($values, $notify = TRUE) {
   -  public function getValue() {
   -    $values = parent::getValue();
   -
   +  protected function includeComputedProperties($include_computed) {
        // If there is an unsaved entity, return it as part of the field item values
        // to ensure idempotency of getValue() / setValue().
   -    if ($this->hasNewEntity()) {
   -      $values['entity'] = $this->entity;
   -    }
   -    return $values;
   +    return $this->hasNewEntity() ?: parent::includeComputedProperties($include_computed);
      }
   

   I think this change is wrong because FieldItemBase has $include_computed as FALSE by default, so we are losing the "idempotency of getValue() / setValue()" part?

Awesome! I think this is finally ready now :)

Two things are not clear here to me:

1. The performance implications.
2. Why this is necessary. The code comment says this:
   Alter the title of a referenced entity and serialize the parent entity through deep serialization in order to serialize the change made on the referenced entity. But when would one do that? When would that be valid? Because in REST for example, we must modify entities one-by-one anyway.

From the issue summary:

first approach I am currently evaluating

I think before we make this type of change to core we need something strong than that. Also theoretically couldn't said module write a class that wraps entities and uses reflection to perform to do the full state serialisation. Also tbh the full state serialisation feels like a recipe for some very complex bugs.

Also tbh the full state serialisation feels like a recipe for some very complex bugs.

Exactly. Hence the concerns I raised in #109. Also, it can result in very complex performance bugs — because there is not necessarily a limit on the amount of recursion (when following entity references). Nor do I see a guarantee that this works correctly for cyclical references.

Thanks @alexpott and @Wim Leers!
You are right that the issue summary doesn't currently make a good case for this. Will try to find some time to get it in shape. Also we should add tests for cyclical references (and I guess fix them, because I fear that indeed, they do not work with the current patch). Very nice catch!!

But this is changing innards of Entity API, not of entity forms. If you'd only be changing form logic, I'd agree. But now, I'm not entirely convinced by #113.

So the only thing that I could think of that could cause problems are only circular references, but they could be caused even now - e.g. consider appending the entity zero to entity two - both normal and deep serialization could not detect this.

Your right that that will currently cause problems. I will file a separate bug report for that. In combination with deep serialization it becomes a much more severe issue, though. In HEAD it is only an issue if you serialize an entity that you have created with looped references in memory - as your example shows. With deep serialization this will become an issue even for entities that have been persisted in the DB. And the likelihood of serializing persisted entities vs. entities in memory is much higher, as well as the likelihood of persisted entities having circular references vs. in memory entities. So while we need to fix the bug in HEAD, I don't think this issue can go in if it has a problem with circular references.

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -141,4 +141,67 @@ protected function doTestSerialization($deep_serialization) {
   +    // Case 1: new_entity->existing_entity->parent_new_entity
   

   This description doesn't seem to be true, since all entities are saved?

   That said would be interesting to have a test case where both entities are not yet saved. Still tentatively removing the "Needs tests" tag.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
   @@ -141,4 +141,67 @@ protected function doTestSerialization($deep_serialization) {
   +    // Make sure all the entities are initialized in the structure, otherwise
   +    // they will not be contained in the serialization.
   +    $entity_level_zero->$field_name->entity->$field_name->entity;
   

   Ahh, so I guess if you do

   $entity_level_zero->$field_name->entity->$field_name->entity->$field_name->entity->$field_name->entity
   

   or something you would get a deeper level of serialization? If so, I think we are good because it would be broken code to manually instantiate nested entities to a nesting level where it could become problematic for e.g. memory reasons.

   I also think the test is quite clear.

Thanks, the test looks great to me!

@Wim Leers does that alleviate your concerns or do you also have concerns regarding deeply nested, non-cyclical entities? I personally don't see that as a concern, because both in forms (with Inline Entity Form / Paragraphs) and in rendering you end up loading all entities anyway so this will actually greatly improve performance once we start implementing it there. So this would be a hit, e.g. if we use deep serialization for forms (in a followup) and a very deeply nested entity is referenced per autocomplete. While this is not a far-fetched example (i.e. a "Related articles" field with "paragraphed" articles) I don't think the "normal" examples involve that many levels of references that it's going to be performance-relevant. But would love to hear your thoughts or if you have any specific examples in mind.

[…] but if anyone thinks that the current approach is still too general then […]

I'd go much further: I'd just hardcode the special-casing of \Drupal\Core\Entity\Plugin\DataType\EntityReference (and perhaps subclasses of that). IMHO there is no need for an API for this. If the need arises at some point in the future, we could add an API. But if this conceptually only makes sense for entity references, then we should only do it for entity references, we shouldn't add an entire API.

I remembered about this issue when @tstoeckler referenced it from #2923015-6: [PHP 7.2] Incompatible method declarations and I have a question.

The main use case for all this "deep serialization" stuff is that some code needs to get the entity property of an entity reference item when calling getValue() and when the the entity has been "changed" (i.e. it is not the same object that would be returned by loadUnchanged($target_id)), right?

If so, wouldn't it be easier to track the "changedness"/touched/dirty state of an entity in an internal property, with a setter and a getter for it? If we would have that, this patch would be just a one-line (code) change to EntityReferenceItem:

diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
index f9922b9..2bf3cb6 100644
--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -211,9 +211,9 @@ public function setValue($values, $notify = TRUE) {
   public function getValue() {
     $values = parent::getValue();
 
-    // If there is an unsaved entity, return it as part of the field item values
-    // to ensure idempotency of getValue() / setValue().
-    if ($this->hasNewEntity()) {
+    // If there is an unsaved entity or a changed entity, return it as part of
+    // the field item values to ensure idempotency of getValue() / setValue().
+    if ($this->hasNewEntity() || (isset($this->entity) && $this->entity->isChanged())) {
       $values['entity'] = $this->entity;
     }
     return $values;

Yes, with something like #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) this would become much easier, as far as I can tell #131 is spot on (although maybe @hchonov has more info).

Historically, this predates the whole movement on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed), that's why we went this route. Furthermore, even though that has seen some activity due to it touching the API-First initiative, I am still not convinced we will be able to solve that issue properly simply due to the complexity of the Entity API and all the different things you can do with entities and all the ways you can do things (i.e. set field values) in different ways. And as soon as we rely on the "dirty-fields" API for anything meaningful, bugs in it will inevitably lead to access bypass at best and data loss at worst. That's why I would be wary marking this (or anything, to be honest) postponed on that. That said, it would not be the first time that smart people (including all the smart people in this issue) have proven me wrong, where I thought we couldn't fix a complex problem properly.

Thanks for the re-roll. I think the updated code looks good. Some minor points:

1. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -6,10 +6,29 @@
   +    if ($deep_serialization) {
   ...
   +        if (($entity = $item->entity) && $entity instanceof FieldableEntityInterface) {
   +          $entity->setDeepSerialization(TRUE);
   +          $values[$delta]['entity'] = $entity;
   +        }
   ...
   +    }
   

   If I'm not mistaken we should always but the entity in $values if it exists. Regardless of whether it's a fieldable entity or not, right?

   Then we should check whether it is fieldable and the set the deep serialization flag on it, if it is.

   In other words, the check for FieldableEntityInterface should only affect the next level of serialization, not the current one, right?

2. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -107,6 +107,14 @@ public function setValue($values, $notify = TRUE) {
   +    // By default do not return computed properties for deep serialization.
   

   This comment looks a bit out of place now. Maybe just drop it?

Thanks for the updated patch.

Read through the entire patch again including the very thorough test coverage. The patch really is minimally invasive while allowing for some great stuff in contrib. And - as stated - is very well tested including recursion and cloning. There's no reason not do this.

I see that the patch changed a lot since my last reviews here so I'd like to take another look.. probably tomorrow.

That's absolutely fair. No need to rush this.

I had to read #133 many times before I could fully understand what @hchonov tried to say there, so here's a simpler version which will hopefully save other reviewers some time:

If an entity form provides the ability to edit referenced entities (e.g. via an inline entity form widget), then the top-level form is also responsible for the state of all its referenced entities (including nested sub-references), just like it is responsible for the state of its own (top-level) entity.

And here's a review for the latest patch:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -176,6 +176,18 @@
   +  protected $deepSerialization = FALSE;
   

   Since the scope of this issue has been reduced to only handle entity references, a scope which I fully agree with, I think that
   "deep serialization" doesn't really mean that much anymore, so I would propose to rename this to serializeNestedReferences.

2. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
   @@ -236,4 +236,34 @@ public function isValidationRequired();
   +  public function setDeepSerialization($deep_serialization);
   

   We should also add a getter for the new property, something like shouldSerializeNestedReferences().

   I would mark both of them as @internal, so they can be removed later in case #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) manages to handle this use-case properly.

3. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,19 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +  public function getSerializationValue($deep_serialization);
   

   Based on the point above, I think this new API method is not necessary and that we should handle nested reference serialization directly in EntityReferenceItem::getValue(), by adding an additional check next to hasNewEntity():

   if ($this->hasNewEntity() || $this->getEntity()->shouldSerializeNestedReferences()) {
     $values['entity'] = $this->entity;
   }
   

#141.3: The problem with that is that getSerializationValue() recursively marks children as needing this property, which I dislike about the patch anyway (a getter leading to a side-effect), but did have no better idea till now.

Also it feels this patch cannot really decide if it wants to do this for entity references on a per field basis or on the content entity base.

Maybe the set method should check all fields implementing some kind of interface and then recursively call the field methods?

A tricky issue for sure ...

Also it feels this patch cannot really decide if it wants to do this for entity references on a per field basis or on the content entity base.

That's a very good point! And it basically means that we shouldn't try to serialize *all* (nested) references, just the ones that are editable through an inline entity form, which means that this "toggle" variable needs to be moved to EntityReferenceItemList so it can be controlled/set at the widget level.

Typo correction

Here's a re-roll, no other changes.

Fixed EntityManager deprecation and code style.

I ran into a potential issue with this patch regarding multi-value entity reference fields. The referencedEntities method will try to load the entity from storage if target_id is present and bypass the entity property which contains the deserialized entity.

https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

Reroll

Fixed the deprecation notices on the ContentEntitySerializationTest.php from #156.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added patch against #158 in 10.1 version

Setting as need review for tests to run.

Missing core/tests/Drupal/KernelTests/Core/Entity/ContentEntitySerializationTest.php
in patch #162 which was present in #158.

Hiding patch #162.

Addressed the comment on #164
Attached patch against Drupal 10.1.x