Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods

+++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
@@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
+      $items = $entity->get($field_name);

This could throw an exception if the field doesn't exist, we should wrap it in a ::hasField check - sometimes normalizers add additional data to the output that doesn't correspond to an entity field, e.g. #2698785: Cannot deploy book nodes - book metatdata not serialized does this

I currently find this patch very confusing still. That may be me though. I'd love to see damiankloip review this.

1. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +    // Iterate through remaining items in data array. These should all
   +    // correspond to fields.
   

   "remaining"? That implies some are discarded?

   Anyway, I think this entire comment can be removed.

2. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      // Remove any values that were set as a part of entity creation (e.g
   +      // uuid). If the incoming field data is set to an empty array, this will
   

   So how exactly does it do this?

3. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      // also have the effect of emptying the field in REST module.
   

   We should not mention REST module. As far as this code is concerned, we're just dealing with serialization.

4. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +32,26 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +        // Denormalize the field data into the FieldItemList object.
   +        $context['target_instance'] = $items;
   +        $this->serializer->denormalize($field_data, get_class($items), $format, $context);
   

   The comment is talking about FieldItemList. But the $context['target_instance'] suggests an entity reference field. And then finally we just call denormalize on $field_data and don't even modify $entity anywhere.

   I don't understand how this works.

5. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   + * Converts the Drupal field item object structure to HAL array structure.
   

   c/p remnant: this is not at all about HAL.

6. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   +   * The interface or class that this Normalizer supports.
   +   *
   +   * @var string
   

   Overly verbose; can just use "inheritdoc".

7. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   +    if (!isset($context['target_instance'])) {
   +      throw new InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldItemNormalizer');
   +    }
   

   Oh so this is where target_instance comes to play again. So this is an "internal use only" thing, i.e. an implementation detail we should shield the world against. Right?

   If so, I suggest we use a more specific name, and possibly prefixed with a double underscore?

8. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   +    if ($context['target_instance']->getParent() == NULL) {
   

   Strict check please.

9. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,93 @@
   +    // If this field is translatable, we need to create a translated instance.
   +    if (isset($data['lang'])) {
   +      $langcode = $data['lang'];
   +      unset($data['lang']);
   +      $field_definition = $field_item->getFieldDefinition();
   +      if ($field_definition->isTranslatable()) {
   +        $field_item = $this->createTranslatedInstance($field_item, $langcode);
   +      }
   +    }
   

   Isn't this lang attribute HAL-specific stuff?

10. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,45 @@
    + * Converts the Drupal field structure to HAL array structure.
    

    Same as above.

11. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,45 @@
    +   * The interface or class that this Normalizer supports.
    +   *
    +   * @var string
    

    Same as above.

12. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,45 @@
    +    if (!isset($context['target_instance'])) {
    +      throw new InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldNormalizer');
    +    }
    

    Same as above.

13. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,45 @@
    +    if ($context['target_instance']->getParent() == NULL) {
    +      throw new InvalidArgumentException('The field passed in via $context[\'target_instance\'] must have a parent set.');
    +    }
    

    Same as above.

First of all: wow. This definitely needs an Entity Field API expert's eyes on it. Preferably multiple.

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -27,11 +27,13 @@ services:
          # Set the priority lower than the hal entity reference field item
          # normalizer, so that we do not replace that for hal_json.
          # @todo Find a better way for this in https://www.drupal.org/node/2575761.
   -      - { name: normalizer, priority: 5 }
   +      - { name: normalizer, priority: 9 }
   

   There was a comment justifying this priority. But the hal priority is not being changed, yet this one is. So either this change must be reverted, or the justification must be updated.

2. +++ b/core/modules/serialization/serialization.services.yml
   @@ -27,11 +27,13 @@ services:
   +      # Priority must be high than serialization.normalizer.field but less than Hal
   

   Nit: s/Hal/hal/

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -27,11 +27,13 @@ services:
   +      # Field normalizer
   

   Nit: s/Field/field/

4. +++ b/core/modules/serialization/serialization.services.yml
   @@ -74,3 +76,11 @@ services:
   +      - { name: normalizer, priority: 8 }
   ...
   +      - { name: normalizer, priority: 8 }
   

   These need justifications too.

5. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,32 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      if ($field_name == $bundle_key) {
   

   ===

6. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,32 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      $fieldItemList = $entity->get($field_name);
   

   Nit: underscores, not camelCase.

7. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,32 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      // Remove all values that were set as a part of entity creation as the
   +      // field values were not denormalized. When passing this field item list
   +      // to field denormalizers there should be no values in the field.
   +      $fieldItemList->setValue(array());
   

   Is this also correct for the case of POST an entity but with only required fields provided in the request body? Will this still result in the default values being set for a new entity?

   IOW: what if $field_data === []? Does the denormalizer then automatically set it to the default values again? At which point I wonder if this code should only run if $field_data !== []?

8. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,55 @@
   +    if ($context['__target_field_item_instance']->getParent() === NULL) {
   +      throw new InvalidArgumentException('The field item passed in via $context[\'__target_field_item_instance\'] must have a parent set.');
   +    }
   

   What is this parent necessary for? I don't see any ->parent() invocation?

9. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
   @@ -0,0 +1,50 @@
   +class FieldNormalizer extends ListNormalizer implements DenormalizerInterface {
   

   Should this not be called FieldItemListNormalizer?

   Why does this extend ListNormalizer?

10. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,50 @@
    +    if ($context['__target_field_instance']->getParent() == NULL) {
    

    ===

11. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
    @@ -0,0 +1,50 @@
    +    /** @var FieldItemListInterface $items */
    

    FQCN.

12. +++ b/core/modules/serialization/tests/modules/field_normalization_test/field_normalization_test.services.yml
    @@ -0,0 +1,5 @@
    +      - { name: normalizer , priority: 20 }
    

    Why this priority?

Thanks, this now looks much better. Now blocked on Entity Field API expert's review.

1. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +   * {@inheritdoc}
   +   */
   +  public function denormalize($data, $class, $format = NULL, array $context = []) {
   +    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
   

   So what's interesting is that the parent class has a whole bunch of logic that content entity specific too. 80% of it is bundle related and calls content/fieldableentity specific methods.

   separate issue but that should probably move down too?

   Second, can we avoid code duplication (and difference implementations) between this and \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize(). That has a bunch of hal specific things, but actually creating the fields and running normalizers on each field is basically the same, no? except that that seems to filter out the bundle field differently and also does empty-ing slightly different. And the hal implementation also has special logic for the langcode, not sure if that applies here.

   Btw. All those tricks are necessary because we have no proper entity factory that can create an *empty* entity. Look how much simpler this gets with #1867228: Make EntityTypeManager provide an entity factory.

2. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      if ($field_data) {
   +        // The field instance must be passed in the context so that denormalizer
   +        // can update field values. The entity itself does not need to be passed
   +        // because nothing except the current field needs to be set.
   +        $context['__target_field_instance'] = $field_item_list;
   +        $this->serializer->denormalize($field_data, get_class($field_item_list), $format, $context);
   +      }
   

   So the basic problem that we assign twice.

   First, we create the entity with the raw values. And then we loop over it again and set the values, again.

   This only works because entity field API allows you to put whatever you want into the values of a field item and it will also happily return it again.

   But it might actually cause problems once we actually have normalizers that result in different structure where trying to set that would cause problems.

   If you compare with hal.module, it works differently, it actually creates the entity only with the absolutely required data* (bundle if existing and langcode) and then assigns everything only after it has been denormalized. So again, can't we share more code with hal?

I spoke to berdir about this on IRC, going to do a bit of work on it.

Let's first try to address the point about assigning the field values twice. This does something more like hal module does; create an empty entity just based on the bundle then apply each field value from the data.

I spoke with Berdir about moving the code currently in \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() (and probably what's in Drupal\serialization\Normalizer\ContentEntityNormalizer::normalize()) into a new FieldableEntityNormalizer as this is most accurate. The main concern around this initially is that this logic completely doesn't apply to config entities. HOWEVER, we have the small issue around BC. So based on that I think we might be stuck adding this functionality into EntityNormalizer instead so we can keep the fieldable stuff it provides but we could keep EntityNormalizer functionality as-is and reuse some of these methods in a FieldableEntityNormalizer instead possibly. I have also refactored some of the existing code in an attempt to make it more re-usable in subsequent patches (if we try and tackle being able to reuse this stuff in hal module too).

This is a first attempt, it seems to work ok so far.

1. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      // The bundle field was denormalized in parent::denormalize().
   +      if ($field_name === $bundle_key) {
   +        continue;
   +      }
   

   This comment seems strange to me. As far as I can tell, all fields we're normalized in parent::denormalize(). Is this being ignored because it's read-only? Is so, feel free to leave the code as is, and point to #2350429: Clarify the meaning of read-only fields and properties in the code, but I think the current comment could be improved.

2. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +      if ($field_data !== []) {
   ...
   +      }
   +      if ($field_data) {
   ...
   +      }
   

   I can somehow guess why that's both needed, but this should could use some serious comment on the different input data here. As far as I can see, the point being that $field_data can also be an (empty) string or integer.

3. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +        // can update field values. The entity itself does not need to be passed
   +        // because nothing except the current field needs to be set.
   

   This is somewhat lying as people can always do $items->getEntity() and still do crazy stuff with the entity.

4. +++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
   @@ -32,4 +30,35 @@ public function normalize($object, $format = NULL, array $context = array()) {
   +        $context['__target_field_instance'] = $field_item_list;
   

   The '__' prefix suggests this is something very much internal, but it seems like pretty legitimate usage of the $context parameter. Can you explain?

   Also why "_instance" instead of "_item_list"?

5. +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
   @@ -0,0 +1,52 @@
   +  protected $supportedInterfaceOrClass = 'Drupal\Core\Field\FieldItemInterface';
   

   Could use FieldItemInterface::class

6. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
   @@ -0,0 +1,47 @@
   +      $context['__target_field_item_instance'] = $items->appendItem();
   

   Again I think "_item" is more clear than "_item_instance" in my opinion.

@tstoeckler looks like this was not based on that latest patch? I'll see which ones apply on the current patch, some other good points though - thanks! We will be trying to consolidate what we can with the hal functionality, so I think that means (for 8.x atleast) that we use the same naming scheme that hal uses. I.e. 'target_instance' so we can hopefully share more code.

Oh yeah, that was a pretty big x-post, sorry.

This addresses most of your feedback I think, and makes the name changes to fall in line with hal module as suggested above. As if we want to consolidate this functionality more, we need to use the name and $context usage that hal already uses. better naming will have to be talked about for Drupal 9 I think.

1. Doesn't exist anymore, but yes, it was because it's a read only field. We now just create an entity with the bundle, then iterate over the remaining field data.
2. This has a comment already in the more recent patch and is done slightly differently too (we don't need to check the value).
3. Updated the comment. I think that's clearer...
4 & 6: Renamed to only use 'target_instance' like hal module
5. Changed!

Let's see how this patch gets on before trying to see what we can share between serialization and hal module.

EDIT: the previous patch got removed/deleted and I can't restore it for some reason...

This should fix the EntityNormalizerTest so at least we have something passing that we can tweak.

#24:

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -39,9 +41,73 @@ public function __construct(EntityManagerInterface $entity_manager) {
+    $entity_type_id = $this->determineEntityTypeId($class, $context);
+    $entity_type_definition = $this->getEntityTypeDefinition($entity_type_id);
+
+    // The bundle property will be required to denormalize a bundleable
+    // fieldable entity.
+    if ($entity_type_definition->hasKey('bundle') && $entity_type_definition->isSubclassOf(FieldableEntityInterface::class)) {
+      // Get an array containing the bundle only. This also remove the bundle

If all this logic actually belongs in a FieldableEntityNormalizer, can we then create a D9-only issue, put this in a helper method, and annotate that helper method with a TODO pointing to that D9 issue?

#32: This should also update the HAL module.

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -25,13 +25,16 @@ services:
   -      - { name: normalizer, priority: 5 }
   +      - { name: normalizer, priority: 9 }
   
   @@ -74,3 +77,14 @@ services:
   +      - { name: normalizer, priority: 8 }
   

   These priorities don't allow anything to be injected in between.

2. +++ b/core/modules/serialization/serialization.services.yml
   @@ -25,13 +25,16 @@ services:
   +      # Priority must be high than serialization.normalizer.field but less than hal
   

   80 cols violation.

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -74,3 +77,14 @@ services:
   +      # Priority must be higher than serializer.normalizer.field_item.hal and lower than any field type specific
   

   80 cols violation.

Created the FieldableEntityNormalizer D9 issue: #2834734: Add a FieldableEntityNormalizer (rather than having support for fields in EntityNormalizer, which we can't change anymore due to BC) - this is referenced in the latest patch.

This does:

• Fix the smaller nits from #33 and #34
• The priorities is tricky, as hal module has already used 10, which is unfortunate. Up to now I have liked to use only intervals of 5 to priorities. This is no longer possible as we need to sit between normalizers that have 5 and 10 :/. Also fixed the comments for those priorities as they were the wrong way round (comments referred to needing to be higher priority compared to hal module, but what was meant was lower).
• Add a FieldableEntityNormalizerTrait where the methods added in the previous couple of patches now live
• hal module's ContentEntityNormalizer now uses this trait to load the entity type definition and denormalize the field items, most of the other code I think might be tricky or unnecessary to try and share?

#32: This should also update the HAL module.

@Wim can you clarify what you mean by this comment too please? :) thanks!

trait/property notice.

This should also update the HAL module.

Never mind that, I confused this with #2751325: All serialized values are strings, should be integers/booleans when appropriate.

I think this looks great. RTBC as far as I'm concerned. I'll let @tedbow actually RTBC this, since he knows this code better than I do.

Thanks Wim. That un-confuses me now too :)

Need to make sure the _restSubmittedFields mess is added to the non field case too I think...

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -14,6 +15,8 @@
   +  use FieldableEntityNormalizerTrait;
   
   +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -11,19 +12,14 @@
   +  use FieldableEntityNormalizerTrait;
   
   +++ b/core/modules/serialization/src/Normalizer/FieldableEntityNormalizerTrait.php
   @@ -0,0 +1,140 @@
   +trait FieldableEntityNormalizerTrait {
   

   Got to love traits - nice to not maintain the duplicate code.

2. FILE: .../serialization/src/Normalizer/FieldableEntityNormalizerTrait.php
   ----------------------------------------------------------------------
   FOUND 3 ERRORS AFFECTING 3 LINES
   ----------------------------------------------------------------------
      1 | ERROR | [x] The PHP open tag must be followed by exactly one
        |       |     blank line
     71 | ERROR | [ ] Missing parameter comment
    116 | ERROR | [ ] Missing parameter type
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

   A few nits to fix

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -25,13 +25,28 @@ services:
   -      - { name: normalizer, priority: 5 }
   +      - { name: normalizer, priority: 8 }
   

   We need a CR for this. And the delicate dance of priorities here makes me think that we might take the opportunity to borrow a leaf from Symfony's book at space these numbers out a bit more... They use numbers to the power of 2... ie. 32,64,128,256,512,1024... not a substitute for a proper dependency system but better than what we're doing.

On 3: #2575761: Discuss a better system for discovering and selecting normalizers

Thanks Alex. Here is a new patch to fix the nits.

The priorities are indeed a massive pain in everyones behind. It is amplified by the fact that we have to juggle normalizers for various formats in here too. I would ideally like to split the serializer out so we have one serializer instance per format. That's another story though. We should have gone with better priorities initially and it would have made our lives a little easier. The trouble with changing these now is that people could be depending on the current priorities. E.g. JSON API uses priorities based on what core is currently using. I would guess there is a whole load of contrib and custom code that's doing the same.

CR: https://www.drupal.org/node/2836018

#46.3:

We need a CR for this. And the delicate dance of priorities here makes me think that we might take the opportunity to borrow a leaf from Symfony's book at space these numbers out a bit more... They use numbers to the power of 2... ie. 32,64,128,256,512,1024... not a substitute for a proper dependency system but better than what we're doing.

What we really should do, is get rid of priorities altogether. It's the same brittle unmaintainable system as we already have with "weights" in Drupal. We're moving things away from weights (e.g. assets in D7 vs D8), but sadly with Symfony we just got more of it. We need "before" and "after" relationships, then let code calculate the right order.

But alas, that would be an even bigger BC break of course.

The trouble with changing these now is that people could be depending on the current priorities. E.g. JSON API uses priorities based on what core is currently using. I would guess there is a whole load of contrib and custom code that's doing the same.

Indeed.

But @damiankloip, your patch is modifying priorities. So is there a good reason to not do what Alex Pott is suggesting? The only reason I can think of would be: the current patch does NOT break the JSON API module.

It does, yes. IMO this is minimal though, for example, it does not break any hal usage. If we completely change priorities we would have to change them everywhere. For the record, I am OK (and would like to do that), I just assumed we might not be allowed to?

Right. I can't answer that, that's up to Alex Pott/core committers. So, back to RTBC.

This is actually major, because this prevents contrib from defining field normalizers.

And for the same reason, it's also blocking several core issues (see the related issues).

Was a random CI fail.

Committed 23a9dd8 and pushed to 8.3.x. Thanks!

Yay!

This unblocked:

1. #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property
2. #2827164: Entity reference field normalization has target_type and target_uuid, but not used in denormalization
3. #2834734: Add a FieldableEntityNormalizer (rather than having support for fields in EntityNormalizer, which we can't change anymore due to BC)

This blocked #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property, which needs to be fixed in 8.2.x too. Besides, this is also a bug in 8.2.x, so I think this actually needs to be committed to 8.2.x too. Our bad for not setting the version right.

The committed patch also applies cleanly to 8.2.x.

+++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
@@ -14,6 +15,8 @@
+  use FieldableEntityNormalizerTrait;

+++ b/core/modules/serialization/serialization.services.yml
@@ -25,13 +25,28 @@ services:
+  serialization.normalizer.field_item:
...
+  serialization.normalizer.field:

I'm not sure that adding methods and services are 8.2.x eligible.

Conclusion: we consider both this bug and #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property as unfixable in 8.2.

I believe the test on both the interface and bundle key is too much? One can implement fieldable entities without also supporting bundles. See #2866083: EntityNormalizer::denormalize() shouldn't require bundle key for a follow-up.

Adding linked issue we might remove in #3266544: Remove remaining (textual) references to the HAL module