FieldNormalizers are very unforgiving, impossible to debug error response given

I think throwing some better error message would be the preferred solution. We should tell people as early as possible when something of their passed input is wrong.

So we could just check that the value that's about to be passed to the denormalization for the field item is an array or not. It should then be handled correctly. Alternatively, we could check for this scalar value and just handle that instead like:

  /**
   * {@inheritdoc}
   */
  public function denormalize($data, $class, $format = NULL, array $context = array()) {
    if (!isset($context['target_instance'])) {
      throw new InvalidArgumentException('$context[\'target_instance\'] must be set to denormalize with the FieldNormalizer');
    }

    /** @var FieldItemListInterface $items */
    $items = $context['target_instance'];
    $item_definition = $items->getItemDefinition();
    $item_class = $item_definition->getClass();

    if (!is_array($data)) {
      $context['target_instance'] = $items->appendItem();
      $this->serializer->denormalize($data, $item_class, $format, $context);
    }
    else {
      foreach ($data as $item_data) {
        // Create a new item and pass it as the target for the unserialization of
        // $item_data. All items in field should have removed before this method
        // was called.
        // @see \Drupal\serialization\Normalizer\ContentEntityNormalizer::denormalize().
        $context['target_instance'] = $items->appendItem();
        $this->serializer->denormalize($item_data, $item_class, $format, $context);
      }
    }

    return $items;
  }

Even just casting the value to an array would actually get handled correct too, I think...

I made a change to the hal FieldNormalizer as we have the same problem in both. I then realised we have the same code in both - if the hal FieldNormalizer extends the serialization FieldNormalizer we can share this method.

Since we did #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods this is also a problem for serialization module too. Hence the direction of the patch. Re-titling accordingly.

Here is just a quick review.

1. +++ b/core/modules/serialization/src/Normalizer/FieldNormalizer.php
   @@ -30,9 +31,18 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +      throw new InvalidArgumentException('The field passed in via $context[\'target_instance\'] must have a parent set.');
   ...
   +      throw new UnexpectedValueException('Field values must use an array structure');
   

   Can we use the field name in the exception message? This makes it a bit easier to debug later one day

2. +++ b/core/modules/serialization/tests/src/Kernel/FieldItemSerializationTest.php
   @@ -117,4 +117,19 @@ public function testFieldNormalizeDenormalize() {
   +   *
   +   * @expectedException \Symfony\Component\Serializer\Exception\UnexpectedValueException
   +   * @expectedExceptionMessage Field values must use an array structure
   

   Note: We use $this->setExpectedException now

Thanks Daniel! Seems like fair points you have there.

WFM

Committed/pushed to 8.3.x, thanks!

Just wanted to share our unfortunate experience with regard to this change.

We're on the trailing end of upgrading from 8.2 to 8.4 and just discovered that our external integrations are broken as they are using JSON with abbreviated field structures. When the integrations started failing, I was able to trace it back to this issue. I'm surprised that a BC breaking change like this didn't end up with at least a change record.