FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method

The JSON API sister issue for this has a green patch already. See #2955615: Field properties are not being denormalized. This issue should be able to adopt the same approach.

#2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API has run into this. Here's a patch that adds support for it. If all is well, this will just pass tests on the first try 🤘

Woah, very close, only EntityTestMapField(Json|HalJson)AnonTest failed and Drupal\Tests\serialization\Unit\Normalizer\EntityReferenceFieldItemNormalizerTest!

Easy. One oversight that only manifests itself for @FieldType=map, and one mock.

This should be green!

We probably want to add test coverage here.

Tests-only patch is also the interdiff.

+++ b/core/modules/serialization/tests/modules/test_datatype_boolean_emoji_normalizer/src/Normalizer/BooleanNormalizer.php
@@ -0,0 +1,36 @@
+  public function normalize($object, $format = NULL, array $context = []) {
+    return $object->getValue() ? '👍' : '👎';
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function denormalize($data, $class, $format = NULL, array $context = []) {
+    if (!in_array($data, ['👍', '👎'], TRUE)) {
+      throw new \UnexpectedValueException('Only 👍 and 👎 are acceptable values.');
+    }
+    return $data === '👍';
+  }

This may seem far-fetched, but it isn't really.

A concrete example of this can be found in the @DataType=timestamp field, where a UNIX timestamp is being exposed to the outside world as an RFC3339 datetime string.

Perhaps an even more convincing example is @DataType=datetime_iso8601, which contrary to what the name suggests, does NOT get stored as an ISO8601 datetime string: the timezone information is omitted in storage, because they're all converted to UTC prior to storing! (That's what #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API is about.)

This clearly shows the need to not expose our storage implementation details to the world. And it's pointless to implement custom normalizers for every format (it's impossible to have a single @FieldType-level normalizer for both HAL and non-HAL normalizations).

https://www.drupal.org/files/issues/2018-12-28/2957385-9.patch didn't include #7. D'oh. Test cancellation requested.

+++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
@@ -51,7 +52,37 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   protected function constructValue($data, $context) {
-    return $data;
+    $field_item = $context['target_instance'];

This is modifying the default FieldItemNormalizer, but we still need to update HAL's ::constructValue() too. That explains the failures (the ones I expect 9-rerolled.patch to have).

Copy/pasted the exact same code, which should make it green.

Shall we keep the code duplicated or we move it into a shared trait, for example \Drupal\serialization\Normalizer\FieldableEntityNormalizerTrait?

• #9's test-only patch had 3 failures.
• #9's tests+fix patch (proper one at #11) had 1 failure.
• #13 has 0 failures.

✅

This is now officially blocked on review.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1575,7 +1575,8 @@ protected function assertStoredEntityMatchesSentNormalization(array $sent_normal
         // Fields are stored in the database, when read they are represented
         // as strings in PHP memory. The exception: field types that are
         // stored in a serialized way. Hence we need to cast most expected

@@ -1583,6 +1584,21 @@ protected function assertStoredEntityMatchesSentNormalization(array $sent_normal
         $expected_field_normalization = ($field_type !== 'map')
           ? static::castToString($field_normalization)
           : $field_normalization;

This is now more complex than necessary. We can do this better now! That will make it less coupled to the map field type and be more robust for the future.

Oops, removed one actually useful/correct/helpful comment.

#17 failed for MenuLinkContent + Shortcut REST tests. All other tests passed though! 💪

Simple oversight.

As usual, I'm really surprised that we've gone this long without this patch!

Yep…

Let's not introduce a trait for this. Instead, let's add a hard-to-miss comment

That trait I mentioned already exists. @gabesullice said in chat that he's then ambivalent about it. But duplicating nontrivial code seems unlikely to be approved by core committers. Plus, the docblock for \Drupal\serialization\Normalizer\FieldItemNormalizer::constructValue() already diverged from \Drupal\hal\Normalizer\FieldItemNormalizer::constructValue(), so it's already been poor at remaining consistent, so we can fix all of that in one go.

(That is the bulk of this interdiff.)

#19: Thanks for the review!

since $property_name comes from user input

Not anymore since #17 :)

Gabe pointed out via chat that I misinterpreted

since $property_name comes from user input

Sorry!

I'm opting for the same approach as used by \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::assertStoredEntityMatchesSentNormalization().

(In doing so, found a nit to fix!)

Issue summary updated.

I'm not sure we should have an assert when this is enforced by the interface

Hm, I'm not sure what my reasoning was there either … 😅

Oh! Note how getItemDefinition() says a DataDefinitionInterface needs to be returned, not a FieldItemDataDefinitionInterface! And since we're calling getPropertyDefinitions() next, we're asserting that interface to ensure that that method actually exists.

I think a change record would make sense here, but I think it'll make more sense after #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem lands. This issue is effectively introducing the infrastructure to make #2926507 possible at all.

A change record for #2926507 already exists: https://www.drupal.org/node/2934779. I added this issue to it. I don't think it should be published until after #2926507 also lands.

Done!

Rebase powered by git, didn't apply cleanly anymore after #3020001: Error when normalizing entity reference field item that references to a new entity landed earlier today.

#32.2: That else only exists because @jibran asked for it in #27. Adding a comment like // Fall back to returning without denormalizing if no denormalizer exists. doesn't seem very helpful? If you think it is, I'll add it.

#2977107: Use more specific entity.manager services in module .services.yml files conflicted with this in a trivial way. Rebased.

Thanks for taking a look! 🙏

#37:

1. Because it only needs that one method from the trait, and PHP requires to specify its visibility. The use statement is indeed specifying the same visibility as the one in the trait.
2. ✅ Good catch, that's a c/p leftover. Removed!
3. ✅
4. ✅ I used to have a reason to do this, but it was wrong, so: added! 😀 If we're doing this, we might as well also do xml to prove it works for all formats, so did that too.

#38.1: This (if ($main_property_name === NULL)) is purely theoretical (#27.1 asked for it). There is no way to make it occur in Drupal core, not even with @FieldType=map, because that requires an array. Since it's purely theoretical, I'm removing it here. There's no feasible way to test this. I agree this makes it needlessly complex. If you'd like to keep something like this, I suggest we convert it to an exception instead of an early return.

#38.2: this is about allowing the main property to be implied, which $field_item->setValue() allows: it considers $node->title->setValue('foobar') and $node->title->setValue(['value' => 'foobar']) as equivalent. The first specifies only the value for the main property, relying on \Drupal\Core\TypedData\TypedDataInterface::setValue() to handle this nicely. The second specifies the property explicitly, allowing setValue() to do less work. Basically, ::setValue() implements the robustness principle aka Postel's law.
Because \Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize() and \Drupal\hal\Normalizer\FieldItemNormalizer::denormalize() rely on ::setValue(), they inherit this same behavior.

Therefore, we need to do match this logic when denormalizing properties: if we receive 'foobar', then we have to assume it's the main property, so we look up which @DataType the main property has and then check if a denormalizer exists for that @DataType. If none exists, we just return the value as-is: this matches behavior in HEAD and allows @DataType denormalizers to continue to be optional.

Hopefully you were nodding along, that should've made sense :)

Now here's the thing: the \Drupal\test_fieldtype_boolean_emoji_normalizer\Normalizer\BooleanItemNormalizer::denormalize() @FieldType-level denormalizer did not yet respect the robustness principle. So before we can add explicit test coverage for #38.2, we need to make it equally forgiving (or applied to the example I've been using: right now it only accepts ['value' => foobar'], not just 'foobar').

_{Oh, also refactored the existing tests very slightly to make it much easier to add the test coverage requested in #38.2.}

And this then actually addresses #38.2: explicit test coverage for that case you pointed out!

Hah, TIL! 🐣

@plach++
@tim.plunkett++

YAY! 🥳

It doesn't need backporting indeed :)