Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX

The title of this issue is a little confusing here.

Storage is UTC. In the UI, when a user enters a value, DateTimeWidgetBase::massageFormValues() handles the conversion to UTC. On output, the DateTimeFormatterBase::setTimeZone() handles the conversion based on system/user settings.

TBH, I have never thought about this from the REST perspective. Pinging that team now.

I tweaked the title.

Initial thoughts from some other committers where that the server (ie, Drupal) needs to stay UTC, and that the client (ie, you) needs to handle TZ conversion based on your rules.

However, I am trying to get a definitive answer on this from @Crell or @klausi on this.

OK, this is a really good question.

Short answer: client should handle conversion to/from UTC when using the REST w/ entities with datetime fields.

Long answer: REST isn't really timezone aware. The biggest problem is that according to Wim, cacheable metadata isn't woven into the REST system, which is needed for it to be TZ aware.

This is a good write-up on best practices: http://apiux.com/2013/03/20/5-laws-api-dates-and-times/

Even though this is centered on the datetime field, I think this is a generic problem that rest.module should have a documented answer for. We also still have the created and changed timestamps, which are stored as UNIX timestamps (i.e. integers), and which AFAICT just use the server's timezone.

IMHO we should just always return UTC and convert, if needed.

#9: sounds sensible to me. But then we should document it as such, and have tests to prove it actually works. So +1.

Note that if it turns out that the created and changed basefields are actually stored in UTC (which AFAICT is not the case), then maybe this issue can be moved to 8.2.x: if only docs + tests are needed, then this doesn't need to wait for 8.3.

Marked #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX as a duplicate of this. It contains useful information, especially for the tests that we'll need here.

#8 + #10, Unix timestamps by definition are offsets from 1970-01-01T00:00:00+00:00, so always reference UTC.

Fixed the dup issue.

#13: oh wow, I didn't know that! Feeling like such a noob now :D Is it possible that at some point it was common to have timezone-specific timestamps? Because I definitely remember dealing with timezone-specific timestamps some years ago!

I created #2824717: Add a format constraint to DateTimeItem to provide REST error message
This should give a more useful message when a datetime value is provided in the wrong format.

Yay, thanks for that! Reviewing that patch now.

That new issue addresses the DateTimeItem case. But there's also the TimestampItem case. CreatedItem</code + <code>ChangedItem extend TimestampItem, so it affects all "created" and "changed" fields, which almost every content entity type has.

So this issue is then solely about TimestampItem. A number is a number, so I don't see how we could possibly add a validation constraint. Hence the only thing I think we can do is document this more clearly. For example:

</code></dt>
<dd>
<code>
    $fields['created'] = BaseFieldDefinition::create('created')
      ->setLabel(t('Authored on'))
      ->setDescription(t('The time that the node was created.'))

The description should become: The time that the node was created, in the UTC timezone.

    $fields['changed'] = BaseFieldDefinition::create('changed')
      ->setLabel(t('Changed'))
      ->setDescription(t('The time that the node was last edited.'))

The description should become: The time that the node was changed, in the UTC timezone.

Repeat for all other entity types

I don't think tests are needed.

Alternatively, we could do something very different: we could transform those UNIX timestamps to 2016-11-01T18:46:41 when normalizing. Then we could do proper validation.

Because, in the end … why does a REST API consumer have to deal with UNIX timestamps? They're much more familiar with 2016-11-01T18:46:41. UNIX timestamps actually can be considered an implementation detail.

Thoughts?

#16, the "alternatively" idea has been bouncing through my head lately because of some of the related issues. Ideally, I think we need to normalize this both inbound and outbound. Right now we are exposing the implementation and not an interface; the API expects people to know about the storage details for different time-related fields. In addition, our storage implementation is a little weird right now (no timezone w/ date+time).

From a REST perspective, we should always give data back in ISO 8601 format (well, the PHP 'c' flavor for date+time and YYYY-MM-DD for date-only) regardless of how we store it. Inbound, we should accept ISO 8601 (a few different variants) and normalize to storage for the item. This gets a bit hairy about how many of the variants we want to accept.

Right now we are exposing the implementation and not an interface

Exactly!

Inbound, we should accept ISO 8601 (a few different variants) and normalize to storage for the item.

+1

This gets a bit hairy about how many of the variants we want to accept.

I'm not concerned about this. We can become more liberal in what we accept in the future.

Note that using ISO 8601 instead of UNIX timestamps would break BC. So we would need to think about BC:

1. New sites only deal with ISO 8601 dates.
2. Existing sites continue to use the old mechanism, but can opt in. Just like we did for another issue, see rest.settings.yml:
   

   # Before Drupal 8.2, EntityResource used permissions as well as the entity
   # access system for access checking. This was confusing, and it only did this
   # for historical reasons. New Drupal installations opt out from this by default
   # (hence this is set to false), existing installations opt in to it.
   # @see rest_update_8203()
   # @see https://www.drupal.org/node/2664780
   bc_entity_resource_permissions: false
   

   In this case, we'd have bc_timestamp_normalizer: false (which is set to TRUE automatically in an update hook that will only run for existing sites).

Alternatively, we could do something very different: we could transform those UNIX timestamps to 2016-11-01T18:46:41 when normalizing. Then we could do proper validation.

Big +1 for this.

Thanks for chiming in, @jhedstrom!

So since we seem to be getting consensus… updating issue title.

Let's see how much breaks. Should be BC?

That code needs to live in a new TimestampItemNormalizer class.

OK, not 100% sure I am on right track here. Work-in-progress in case I can't wrap this up; if someone wants to pick this up feel free. As soon as I made the patch, I realized the changes to the rest module should really be in the hal module since there is where the config is needed? Also, not sure this class is really being picked up; I never saw my debug when testing and didn't get fails. No interdiff since this is a fresh-start.

This patch looks like an excellent start! :)

As soon as I made the patch, I realized the changes to the rest module should really be in the hal module since there is where the config is needed?

Do you mean the BC layer stuff?

1. +++ b/core/modules/hal/hal.services.yml
   @@ -4,6 +4,11 @@ services:
   +  serializer.normalizer.timestamp_item.hal:
   +    class: Drupal\hal\Normalizer\TimestampItemNormalizer
   

   We also need this in serialization.module: that module is responsible for JSON, and provides the default normalization. The HAL module extends (decorates if you will) the default normalization, to change those things it needs to change.

2. +++ b/core/modules/rest/config/install/rest.settings.yml
   @@ -9,3 +9,11 @@ link_domain: ~
   +# are not a universal format for interchance. Instead, RFC3339 timestamps are
   

   s/interchance/interchange/ :)

3. +++ b/core/modules/rest/config/install/rest.settings.yml
   @@ -9,3 +9,11 @@ link_domain: ~
   +bc_timestamp_normalizer: false
   

   Looking at this again, I think bc_timestamp_normalizer_unix would be a more descriptive name.

4. A denormalize() method is missing.
5. Test coverage is missing. Especially test coverage that verifies A) the new behavior is absent when you have BC turned on, B) both the new and the old behavior is supported when you have BC turned off.

As soon as I made the patch, I realized the changes to the rest module should really be in the hal module since there is where the config is needed?

Do you mean the BC layer stuff?

Yeah, the BC stuff. Since the config is in the new normalizer class in HAL, the setting should probably be moved there.

Well, actually the normalizer belongs in the serialization module (see #28.1). So the BC flag would need to be there.

But agreed that rest.settings.yml is not the right place.

1. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,80 @@
   +    $formats = [
   +      'U',
   +      \DateTime::ISO8601,
   +      \DateTime::RFC3339,
   +    ];
   +    $timezone = new \DateTimeZone('UTC');
   ...
   +    foreach ($formats as $format) {
   +      if (($date = \DateTime::createFromFormat($format, $data, $timezone)) !== FALSE) {
   +        return ['value' => (int) $date->format('U')];
   

   Can you add a code comment here describing why the first successful format is used? And why one or more might fail?

2. +++ b/core/modules/rest/config/install/rest.settings.yml
   @@ -9,3 +9,11 @@ link_domain: ~
   +# are not a universal format for interchance. Instead, RFC3339 timestamps are
   

   Should be 'interchange' I think :)

Still a work in progress, but think the feedback is addressed.

Not sure if the denormalize() is right. Does it need to use the $context to name itself and attach to the parent entity?

Kind of torn about whether using the date.formatter is overkill. On one hand, we have the service, so we should use it. On the other, we aren't using the language or timezone support that it provides. Leaning towards using it.

And can't get it to hit debug to see what is really going on when I run various tests.

Will do a Unit test once someone says this looks essentially correct.

I think this looks excellent! I only have nits. What's needed most, is test coverage :)

1. +++ b/core/modules/serialization/config/install/serialization.settings.yml
   @@ -0,0 +1,7 @@
   +# are not a universal format for interchange. Instead, RFC3339 timestamps are
   

   Nit: Perhaps it's me not being a native speaker, but the "Instead" is fairly confusing to me. I think "Now" would make it clearer.

2. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -0,0 +1,8 @@
   +      label: 'Whether the pre Drupal 8.3.x behavior of returning Unix timestamps is enabled or not.'
   

   Nit: s/Unix timestamps/Unix timestamps instead of RFC3339 timestamps for TimestampItem fields/

3. +++ b/core/modules/serialization/serialization.install
   @@ -0,0 +1,26 @@
   + * Enable BC for Timestamp: return Unix timestamps.
   

   Nit: s/return/continue to return/

4. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +  protected $allowedFormats = [
   +    'U',
   +    \DateTime::ISO8601,
   +    \DateTime::RFC3339,
   +  ];
   

   So we allow three formats? That doesn't match the documentation elsewhere.

5. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    // Check for the BC setting. If TRUE, use a Unix timestamp. Otherwise, use
   +    // a RFC 3339 timestamp with the time zone set to UTC.
   +    if ($this->configFactory->get('serialization.settings')->get('bc_timestamp_normalizer_unix')) {
   +      $format = 'U';
   +    }
   +    else {
   +      $format = \DateTime::RFC3339;
   +    }
   +
   +    return [
   +      $field->getName() => ['value' => $this->dateFormatter->format($value, 'custom', $format, 'UTC')],
   +    ];
   

   This ALWAYS returns a UTC timestamp. In >=8.3, it will return a RFC3339-formatted timestamp. In Drupal <8.3, it will return a UNIX timestamp.

   IOW: be conservative in what you send: we always send in a single format.

   This looks great!

6. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    foreach ($this->allowedFormats as $format) {
   +      if (($date = \DateTime::createFromFormat($format, $data, $timezone)) !== FALSE) {
   +        $data_definition = DataDefinition::create('timestamp');
   +        $timestamp_item = TimestampItem::createInstance($data_definition)
   +          ->setValue(['value' => (int) $date->format('U')]);
   +        return $timestamp_item;
   +      }
   +    }
   

   We try all three formats: UNIX, ISO8601 and RFC3339, IOW: be liberal in what you accept.

7. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    throw new UnexpectedValueException(sprintf('The specified date "%s" is not in an accepted format.', $data));
   

   This should list accepted formats (and possibly provide the request time as an example).

I didn't see this before, but just noticed it only just got moved to serialization module :) A few comments here:

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -45,6 +45,11 @@ services:
   +      - { name: normalizer }
   

   This will need a priority, otherwise the regular field item normalizer could normalize first. See the EntityRererenceItemNormalizer as an example.

2. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    $this->configFactory = $config_factory;
   

   Related to my comment https://www.drupal.org/node/2751325#comment-11794602 I think we should consider not having config knowledge in a normalizer iteself, I don't think this is the right place for that implementation detail to live. Maybe based on the BC option we add the normalizer (or configure it differently) or not.

3. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    $field = $field_item->getParent();
   

   We shouldn't need to care about the parent here.

4. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +      $field->getName() => ['value' => $this->dateFormatter->format($value, 'custom', $format, 'UTC')],
   

   Related to the other comment, this is not correct. We just want to return an array with the values here, so ['value' => ...]. This will lead to a normalized structure like ['created' => ['created' => ['value' => 1234567890]]] etc...

5. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,113 @@
   +    foreach ($this->allowedFormats as $format) {
   

   This seems kind of inefficient. Maybe we should also consider adding the format to the normalized data too. I think that would be useful either way.

1. +1
2. Ah, yes, this makes sense! Quoting #2751325-63: All serialized values are strings, should be integers/booleans when appropriate: Keeping the normalizer simpler. Keeping the conditionality out of it. +1!

09:33:37 <Druplicon> WimLeers: 6 hours 46 min ago <mpdonadio> tell WimLeers not really sure how to move https://www.drupal.org/node/2768651 forward based on latest feedback ... not really familiar with how best to proceed with some of the feedback

1. #33 only contains nits, and is asking for test coverage.
2. #36 is mostly small stuff, except point 2. #36.2 is just saying "let's not let the normalizer check config on how to represent a timestamp, let's instead just only add this normalizer service to the container if the configuration says the site wants to use the improved timestamp normalization, and otherwise therefore fall back to the original (HEAD) behavior".

Does that help? :)

Also, the denormalize() implementation, are you expecting that to get called when denormalizing an entity? It would only get called if you denormalized that particular timestamp item afaict.

are you expecting that to get called when denormalizing an entity

Yes.

It would only get called if you denormalized that particular timestamp item afaict.

And yes, that's a huge bug in the existing system. For that, we have #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods.

#33 should be take care of, and I started to stub out a test. Test will fail. I am doing something silly in the setUp, but it is escaping me right now.

#36 should be taken care of, but not sure if using config here and this way is appropriate, and not sure what to do about 36-5. If we don't choose the formats we want to accept, then we need to accept anything and use strtotime(), which can be problematic. Having both \DateTime::ISO8601 and \DateTime::RFC3339 may be overkill, but doing it this way would allow someone to extend the class and just change the list of formats to allow additional ones.

We should probably wait on #2751325: All serialized values are strings, should be integers/booleans when appropriate and #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods for this now. The first so we can reuse the BC helpers in the compiler pass and the latter so we can have denormalization that works.

Here is a rerolled patch that assumes #2751325: All serialized values are strings, should be integers/booleans when appropriate is in. So still needs to wait until then. I made quite a few changes, I have included a kind-of-interdiff file, which is changes I made past porting the patch to use the new changes in #2751325: All serialized values are strings, should be integers/booleans when appropriate. Also note, it moved the single normalization class test to a unit test. I think we should still maybe add some coverage to the rest tests maybe? Similar to the dependent issue.

OK, so I realised we need to do some more work if we want this to work everywhere as expected, like for hal. As it's a field level normalizer, we need a separate normalizer for hal that behaves the same as the hal FieldNormalizer (i.e. returns the field structure keyed by field name so it's mergeable by the content entity normalizer). Also added this BC case to the EntityResourceTestBase coverage in the form of a new helper method for timestamp values, as well as introduce a new bc attribute for normalizer service tags. This can then be used more generically by hal and serialization modules.

1. +++ b/core/modules/hal/hal.services.yml
   @@ -17,6 +17,11 @@ services:
   +      - { name: normalizer, priority: 20, bc: bc_timestamp_normalizer_unix }
   
   +++ b/core/modules/serialization/serialization.services.yml
   @@ -20,7 +20,7 @@ services:
   +      - { name: normalizer, priority: 5, bc: bc_primitive_data_normalizer }
   
   @@ -53,9 +53,10 @@ services:
   +      - { name: normalizer, priority: 8, bc: bc_timestamp_normalizer_unix }
   
   +++ b/core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php
   @@ -23,7 +23,9 @@ public function process(ContainerBuilder $container) {
   +      // If there is a BC key present, pass this to determine if the normalizer
   +      // should be skipped.
   +      if (isset($attributes[0]['bc']) && $this->normalizerBcSettingIsEnabled($attributes[0]['bc'])) {
   

   I like it :)

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -396,12 +396,42 @@ public function testGet() {
   -    // BC: serialization_update_8300().
   +    // BC: serialization_update_8300() and serialization_update_8301().
   

   I'd prefer it if these were tested separately. By having a separate "section" for each BC thing, it's easier to understand.

   I know that the code is testing each BC thing in isolation, but I'd just like separate code sections rather than everything in a big "if".

   So:

   // BC: 8300
   // test…
   
   // BC: 8301
   // test…
   

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1105,6 +1135,39 @@ protected function removeFieldsFromNormalization(array $normalization, $field_na
   +  protected function formatExpectedTimestampItemValues($timestamp) {
   

   Let's move this outside of EntityResourceTestBase, into BcTimestampNormalizerUnixTestTrait.

   Then NodeResourceTestBase and others can just use that trait.

Seems fair, this should address those points. Thanks! (Still leaving as needs work for now).

Per #45.

#2751325: All serialized values are strings, should be integers/booleans when appropriate is in! Unblocked.

+++ b/core/modules/serialization/src/RegisterSerializationClassesCompilerPass.php
@@ -23,7 +23,9 @@ public function process(ContainerBuilder $container) {
-      if ($this->normalizerShouldBeSkipped($id)) {
+      // If there is a BC key present, pass this to determine if the normalizer
+      // should be skipped.
+      if (isset($attributes[0]['bc']) && $this->normalizerBcSettingIsEnabled($attributes[0]['bc'])) {
         continue;

@@ -58,24 +60,6 @@ public function process(ContainerBuilder $container) {
-  /**
-   * Determines whether a given normalizer should be skipped and not added.
-   *
-   * @param string $id
-   *   The normalizer ID.
-   *
-   * @return bool
-   */
-  protected function normalizerShouldBeSkipped($id) {
-    switch ($id) {
-      case 'serializer.normalizer.primitive_data':
-        return $this->normalizerBcSettingIsEnabled('bc_primitive_data_normalizer');
-      default:
-        // Default to FALSE as most normalizers will be added.
-        return FALSE;
-    }
-  }
-

This stuff already landed in #2751325: All serialized values are strings, should be integers/booleans when appropriate.

We need to reroll this.

Ignore this patch.

Rerolled from wrong point. Having trouble finding a place where #48 actually applies to start from. I think this may be more of a re-do than a straight re-roll.

Here's a careful rebase of #48. Many patches have landed in the mean time that indeed conflicted with it. Fortunately, I reviewed all of them, so I was in a good position to rebase this.

1. +++ b/core/modules/hal/hal.services.yml
   @@ -17,6 +17,11 @@ services:
   +     - { name: normalizer, priority: 20, bc: bc_timestamp_normalizer_unix }
   
   +++ b/core/modules/serialization/serialization.services.yml
   @@ -51,6 +51,12 @@ services:
   +      - { name: normalizer, priority: 8, bc: bc_timestamp_normalizer_unix, bc_config_name: 'serialization.settings' }
   

   Missing bc_config_name.

   I did think to add it to the other one, but I forgot it for the first when rebasing.

   Fixed that in this reroll.

2. +++ b/core/modules/serialization/config/schema/serialization.schema.yml
   @@ -5,3 +5,6 @@ serialization.settings:
          label: 'Whether to retain pre Drupal 8.3 behavior of serializing all primitive items as strings.'
   ...
   +      label: 'Whether the pre Drupal 8.3.x behavior of returning Unix timestamps instead of RFC3339 timestamps for TimestampItem fields is enabled or not.'
   

   Should be updated to match the existing message.

3. +++ b/core/modules/serialization/serialization.install
   @@ -54,5 +54,14 @@ function serialization_update_8302() {
   +function serialization_update_8301() {
   

   This will be 8401 instead now.

4. +++ b/core/modules/serialization/serialization.install
   @@ -54,5 +54,14 @@ function serialization_update_8302() {
   +  $rest_settings = $config_factory->getEditable('serialization.settings');
   

   s/rest/serialization/

5. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,99 @@
   +  ];
   +
   +
   +
   +  /**
   

   Triple blank line. Should be single.

So we need to still address:

1. all points in #47
2. all points except the first in #60.

#60 should be done, and visually verified #60-1 was in.

#47-2 doesn't seem to totally apply any more, but I reordered the code so to match the update hook numbers / version numbers.

#47-3 was already done.

Are we missing anything?

At first I thought I had introduced an error in a YAML file. But then I saw that ResolvedLibraryDefinitionsFilesMatchTest also failed during YAML parsing. We definitely didn't touch that. So something's wrong with either testbot or the PECL YAML extension.

Retesting.

1. --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   

   I don't like the code that was moved around here. I think it was clearer previously.

   So, reverting the move. Keeping the comment changes.

2. +++ b/core/modules/serialization/serialization.install
   @@ -54,12 +54,22 @@ function serialization_update_8302() {
   + * @defgroup updates-8.3.x-to-8.4.x Updates from 8.3.x to 8.4.x
   

   The closing comment for this still has "8.2.x to 8.3.x". Fixed.

Addressed both of those nits myself.

In #48:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -148,14 +149,10 @@ protected function getExpectedNormalizedEntity() {
       'created' => [
-        [
-          'value' => 123456789,
-        ],
+        $this->formatExpectedTimestampItemValues(123456789),
       ],

In my careful rebase of that same patch in #58:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -149,12 +150,12 @@ protected function getExpectedNormalizedEntity() {
       'created' => [
         [
-          'value' => 123456789,
+          'value' => $this->formatExpectedTimestampItemValues(123456789),
         ],
       ],

Not quite the same as you can tell. My bad!

That's the biggest problem. But there's more:

1. +++ b/core/modules/hal/hal.services.yml
   @@ -17,6 +17,11 @@ services:
   + serializer.normalizer.timestamp_item.hal:
   

   This has an indentation of one space, but should be indented by two spaces.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -477,6 +479,44 @@ public function testGet() {
   +      $this->config('serialization.settings')->set('bc_primitive_data_normalizer', FALSE)->save(TRUE);
   

   I copy/pasted this from #48, but that key has since changed from bc_primitive_data_normalizer to bc_primitives_as_strings. So this fails of course.

All of those are mistakes I introduced, so fixing them myself.

Now: an actual code review!

1. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,39 @@
   +    $values = parent::normalize($field_item, $format, $context);
   

   s/$values/$normalization/

2. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,39 @@
   +    // Wrap the values in another array, keyed by field name. So values can be
   +    // merged. The values are also wrapped in an additional array so multiple
   +    // items can be merged cleanly.
   

   This is repeating itself.

   Nest the normalization in another array keyed by field name so that multiple items can be merged cleanly.

3. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,39 @@
   +    return array(
   

   Nit: s/array()/[]/

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -477,6 +479,44 @@ public function testGet() {
   +    // Only run this for fieldable entities. It doesn't make sense for config
   +    // entities as config values are already casted. They also run through the
   +    // ConfigEntityNormalizer, which doesn't deal with fields individually.
   ...
   +      // Again do an identical comparison, but this time transform the expected
   +      // normalized entity's values to strings. This ensures the BC layer for
   +      // bc_timestamp_normalizer_unix works as expected. This method should be
   +      // using ::formatExpectedTimestampValue() to generate the timestamp value.
   +      // This will take into account the above config setting.
   

   These comments arecopy/pasted from the comment for bc_primitives_as_strings and then updated only partially. This does not yet make sense.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -477,6 +479,44 @@ public function testGet() {
   +      // Config entities are not affected.
   +      // @see \Drupal\serialization\Normalizer\ConfigEntityNormalizer::normalize()
   +      ksort($expected);
   +      $actual = $this->serializer->decode((string) $response->getBody(), static::$format);
   +      ksort($actual);
   +      $this->assertSame($expected, $actual);
   

   In fact… this is currently not yet testing anything!

   The tests here definitely need work. I also doubt we can test this in the same way that we tested bc_primitives_as_strings, because not every entity type even has TimestampItem fields. For bc_primitives_as_strings, we added EntityResourceTestBase::castToString() and called that on an expected normalization. That allows us to test this in a generic way.

   But the same is not true for timestamp items.

   I think the same applies here as what I wrote at #2824717-30: Add a format constraint to DateTimeItem to provide REST error message:

   Well actually, we still are lacking functional tests that verify that when you interact with DateTimeItem fields via REST you get sensible error messages.

   See \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase and specifically \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestResourceTestBase. The datetime module could subclass that test class, add a datetime field. It'd then be automatically testing GET/PATCH/POST/DELETE requests. You'd just have to override existing methods.

   Note that you can override assertNormalizationEdgeCases() to test various kinds of wrong data to be sent, which will allow you to test the error response!

   Replace "datetime" with "timestamp" and the exact same strategy can work here too :)

6. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,97 @@
   +    $data = parent::normalize($field_item, $format, $context);
   

   s/$data/$normalization/

7. +++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
   @@ -0,0 +1,97 @@
   +    $data['format'] = \DateTime::RFC3339;
   

   Should we also document that format is not a property on TimestampItem? That it is solely there to inform the client that consumes the normalized data?

8. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
   @@ -0,0 +1,146 @@
   +    $this->setExpectedException(UnexpectedValueException::class);
   

   Let's also validate the exception message.

Finally: a look at the BC impact:

+++ b/core/modules/serialization/serialization.install
@@ -56,3 +56,22 @@ function serialization_update_8302() {
+/**
+ * Enable BC for timestamp formatting: continue to return UNIX timestamps.
+ */
+function serialization_update_8401() {
+  $config_factory = \Drupal::configFactory();
+  $serialization_settings = $config_factory->getEditable('serialization.settings');
+  $serialization_settings->set('bc_timestamp_normalizer_unix', TRUE)->save(TRUE);
+}

#2751325: All serialized values are strings, should be integers/booleans when appropriate defaulted to FALSE: i.e. BC layer turned off.

I think that in this case, we do want to have the BC layer turned on by default, because the disruption is far greater.

Addressed most of the feedback, However, the tests needing work I'm not sure I fully understand yet. To me the concept is the same as the strings. This is what the \Drupal\Tests\rest\Functional\BcTimestampNormalizerUnixTestTrait::formatExpectedTimestampItemValues method is for, no? This gets the expected result dependent on whether the config setting is set or not. This seems good to me. What am I missing?

EDIT: I also missed points 7 and 8 from your review. Will just wait on the test bot.

+++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
@@ -20,20 +20,17 @@ class TimestampItemNormalizer extends BaseTimestampItemNormalizer {
+    $normalized = parent::normalize($field_item, $format, $context);

Supernit: $normalized works, I personally prefer $normalization.

#70 says points 7 and 8 were not yet addressed. Points 4 and 6 were also not yet addressed.

Point 5 is being questioned in #70. And… he's of course entirely right. I forgot about formatExpectedTimestampItemVAlues() for a moment! My bad.
However, for full test coverage, it'd be nice to supplement the unit test TimestampItemNormalizerTest with a functional test as described in point 5, where I point to #2824717-30: Add a format constraint to DateTimeItem to provide REST error message. A function test that tests the various formats of sending a timestamp for denormalization would be valuable.

This addresses the other test fails, plus points 4, 7, 8 from #68. RE the $normalization variable name. I am not too keen on that name, as it is more a verb 'normalization', what you actually get is $normalized data. However, this is currently very inconsistent. we use $attributes, $values, and $data in other places.

So this leaves, the variable naming, and the additional test coverage now :) Let's make sure this passes.

"Normalized data" or "a normalization" can both work.

It's "a serialization of an object" and "a normalization of an object".

But I can live with this. I agree that it's "attributes", "values" and "data" that are bad names.

I just spent a while debugging this. We had a couple of issues:

- The denormalization for hal was broken, turns out we do really need to use a trait to share the functionality here. So both normalizers (default and hal) can extend their respective base normalizers. This was causing issues with hals handling of translated field values.
- Add a new normalizedFieldValues() for hal normalizers so extended classes can easily override the normalized data
- More test fixes (using the BC trait to construct the values)

And outstanding, there are still a couple of fails, to do with the \Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields() method I think. It seems that the 422 responses validated contain issues with the normalized data not containing numeric values (timestamp). Denormalization seems to be working ok, so does some validation happen before that or something? on the data? Wim - can you shed any light on this?

I read the #76 interdiff without reading the comment, to check whether I understood it. I think I understood it :)

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -86,6 +74,31 @@ protected function constructValue($data, $context) {
   +  protected function normalizedFieldValues(FieldItemInterface $field_item, $format, array $context) {
   

   This is pure refactoring. Logic is unchanged.

2. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -2,12 +2,23 @@
   -class TimestampItemNormalizer extends BaseTimestampItemNormalizer {
   +class TimestampItemNormalizer extends FieldItemNormalizer {
   

   Rather than extending serialization's timestamp normalizer and adding HAL-specific stuff, we're now extending the HAL field item normalizer and adding timestamp-specific stuff.

3. +++ b/core/modules/hal/src/Normalizer/TimestampItemNormalizer.php
   @@ -2,12 +2,23 @@
   +  use TimeStampItemNormalizerTrait;
   
   @@ -19,18 +30,10 @@ class TimestampItemNormalizer extends BaseTimestampItemNormalizer {
   -  public function normalize($field_item, $format = NULL, array $context = []) {
   ...
   +  protected function normalizedFieldValues(FieldItemInterface $field_item, $format, array $context) {
   +    $normalized = parent::normalizedFieldValues($field_item, $format, $context);
   +    return $this->processNormalizedData($normalized);
      }
   

   And so this is why: to reduce the amount of duplication. No more normalize() method. Instead override this new protected helper method. And then call a method that lives on a trait, so we can share it among the hal normalization and the default normalization.

The 422 responses wrt not containg numeric values: AFAICT it's due to changes in the normalization. \Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields() is what fails, on its first 422 assertion. With this patch applied, we're getting a different error response: a different validation error.

So we must look at the request body we send. The request body that is sent is generated through this code:

    $user = static::$auth ? $this->account : User::load(0);
    $original_normalization = array_diff_key($this->serializer->normalize($user, static::$format), ['changed' => TRUE]);
    …
    $normalization = $original_normalization;
    $normalization['mail'] = [['value' => 'new-email@example.com']];
    $request_options[RequestOptions::BODY] = $this->serializer->encode($normalization, static::$format);

Therefore, the content of $request_options[RequestOptions::BODY] MUST have changed. Comparing the before vs after should bring us an explanation.

Yes, the request body changed. It now has the formatted timestamp values instead of unix timestamps only. The reason I am confused is that I would have thought the validation runs after denormalization, in which case the values should be replaced with their UNIX timestamp equivalents again?

This should address the sorting mis-match issue. I created a new recursiveKSort helper function on EntityResourceTestBase. This could be moved to a move generic place though?

Looks like some menu link REST tests have been added very recently which are now failing (that didn't exist before) because they need to use the new trait.

Yes, #2832859: Write EntityResourceTestBase subclasses for: MenuLinkContent landed yesterday.

OK, me and Wim did a team effort on this bit of debugging. The short version is that our code in \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() was assuming a fieldable entity type would also be using bundles, which users DO NOT :) So it was falling back to the 'basic' behaviour - which is just calling create with all the data passed in, and hence, no field level denormalization called.

Here is a tests only patch. I added some additional coverage and changed existing expectations to match what they should be. We only really need the bundle specific params to create the entity if it uses bundles. Otherwise, it doesn't really matter (like the case of users).

YAY!

The edge case described in #85 is one we overlooked in #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods.

This is getting very close. Shall I go ahead and create a change record?

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -86,6 +74,31 @@ protected function constructValue($data, $context) {
   +   * @param \Drupal\Core\Field\FieldItemInterface $field_item
   +   * @param string|NULL $format
   +   * @param array $context
   +   *
   +   * @return array
   

   Docs needed.

2. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -86,6 +74,31 @@ protected function constructValue($data, $context) {
   +    $values = [];
   

   s/$values/$normalized/

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -467,10 +469,49 @@ public function testGet() {
   +      // Reset the config value and rebuild.
   +      $this->config('serialization.settings')->set('bc_primitives_as_strings', FALSE)->save(TRUE);
   +      $this->rebuildAll();
   ...
   +      // Rebuild the container so new config is reflected in the addition of the
   +      // TimestampItemNormalizer.
   +      $this->rebuildAll();
   ...
   +      // Reset the config value and rebuild.
   +      $this->config('serialization.settings')->set('bc_timestamp_normalizer_unix', FALSE)->save(TRUE);
   +      $this->rebuildAll();
   

   \Drupal\serialization\EventSubscriber\BcConfigSubscriber should remove the need for rebuilding the container?

   I know that #2751325: All serialized values are strings, should be integers/booleans when appropriate also added this where it is activating this BC layer, but that can simply be removed.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -467,10 +469,49 @@ public function testGet() {
   +      // Again do an identical comparison, but this time transform the expected
   +      // normalized entity's values to strings. This ensures the BC layer for
   

   This is copy/pasted from the comment for bc_primitives_as_strings. "values to strings" does not make sense here.

5. +++ b/core/modules/serialization/src/Normalizer/TimeStampItemNormalizerTrait.php
   @@ -0,0 +1,85 @@
   +   * @param array $data
   +   * @return array
   

   .

6. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
   @@ -0,0 +1,151 @@
   +   * Creates a Timestamp Item prophecy.
   

   Nit: s/Timestamp Item/TimestampItem/

7. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
   @@ -0,0 +1,151 @@
   +   * @return \Prophecy\Prophecy\ObjectProphecy
   

   Nit: Append |\Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem.

Thanks Wim, all good points. Hopefully we don't have a need for the rebuildAll() calls anymore and the subscriber can work as expected for tests too!

Title updated, IS updated & CR created: https://www.drupal.org/node/2859657.

Once the fails in #88 are addressed, this is RTBC.

I think these are new fails from the rebuidlAll() removal in the tests.

Mehhhh :( Clearly you're right. I wonder why this passed locally without those calls then :( Anyway, that's really a nit in this issue. We shouldn't block this issue on test cleanup.

Per #87, this is now RTBC :)

Awesome work, @mpdonadio & @damiankloip! :)

As of #2456257-64: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work, this is blocking #2456257. So, tagging blocker and marking Major since #2456257 is major too.

Looks like this needs a reroll.

Re-rolled.

+++ b/core/modules/serialization/serialization.install
@@ -46,3 +46,20 @@ function serialization_update_8302() {
+
+/**
+ * @defgroup updates-8.3.x-to-8.4.x Updates from 8.3.x to 8.4.x
+ * @{
+ * Update functions from 8.3.x to 8.4.x.
+ */
+/**
+ * Enable BC for timestamp formatting: continue to return UNIX timestamps.
+ */
+function serialization_update_8401() {
+  $config_factory = \Drupal::configFactory();
+  $serialization_settings = $config_factory->getEditable('serialization.settings');
+  $serialization_settings->set('bc_timestamp_normalizer_unix', TRUE)->save(TRUE);
+}
+/**
+ * @} End of "defgroup updates-8.3.x-to-8.4.x".
+ */
diff --git a/core/modules/serialization/serialization.services.yml b/core/modules/serialization/serialization.services.yml

We need to remove the grouping, #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x. Here is my crosspost.

+++ b/core/modules/serialization/config/install/serialization.settings.yml
@@ -2,3 +2,10 @@
+# Before Drupal 8.3, timestamps were always returned as Unix timestamps, whic

The word 'whic' should be changed to the word 'which'.

Fixed typo error and attached new patch.

Another commit credit mining attempt from 'Value Bound' - thanks!

This should fix the test failures, new tests added since the last patch.

Yep, #99 started showing failures after #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL was committed yesterday, which added new test coverage, that this patch then also needed to update.

#2843752: EntityResource: Provide comprehensive test coverage for Item entity added new test coverage that now also needs to be updated. Done.

Looks good to me!

Upon replying to #2824717-61: Add a format constraint to DateTimeItem to provide REST error message, I wanted to make the comparison to this issue, and recommend it does the same as this issue: support converting from any timezone to UTC, so that the client does not have to deal with that.

The IS says:

Note that you can send ISO8601 or RFC3339 timestamps using any timezone; we'll convert them to UTC before saving. That's why this prevents the annoying/unpleasant surprise described in the problem report!

But the test coverage only has this:

+++ b/core/modules/serialization/tests/src/Unit/Normalizer/TimestampItemNormalizerTest.php
@@ -0,0 +1,151 @@
+    $data['RFC3339'] = ['2016-11-06T09:02:00+00:00', $expected_stamp];
+    $data['ISO8601'] = ['2016-11-06T09:02:00+0000', $expected_stamp];

We should also have test cases with timezones other than UTC.

I added that to the IS in #90. To prove that I have not misunderstood the code, and to prove that that claim is correct, we should expand the test coverage.

So in this reroll, I'm adding four test cases: a positive and a negative offset for both the ISO8601 and RFC3339 test cases cited above. Also updated the CR to provide an example of this.

That seems totally reasonable to me! The test coverage looks good.

Rebased. Straight reroll.

#2866083: EntityNormalizer::denormalize() shouldn't require bundle key reported one of the problems fixed by this issue. Closed as a duplicate.

#2843754: EntityResource: Provide comprehensive test coverage for Feed entity landed yesterday, we'll need to update FeedResourceTestBase etc in this patch now too. Easy enough.

However, the majority of the failures are due to the changes made by #2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types, which causes failures for every entity type that has PATCH-protected (i.e. read-only) fields. This should not cause a problem, because logically, we should first check whether a field is read-only or not, and then validate the input value. But, alas the entity validation system that uses Symfony's constraints works in a very illogical order. So, this now fails because we're sending a random string as the value for certain read-only timestamp fields (e.g. changed), which then results in a 422 with this error message:

string(203) "{"message":"The specified date \u0022A3[=\u003E\u0026!c\u0022 is not in an accepted format: \u0022U\u0022 (UNIX timestamp), \u0022Y-m-d\\TH:i:sO\u0022 (ISO 8601), \u0022Y-m-d\\TH:i:sP\u0022 (RFC 3339)."}"

We encountered this problem before in #2820364: Entity + Field + Property validation constraints are processed in the incorrect order.

Once again the Entity/Field API is blocking progress for REST :(

This should fix the 6 Feed-related failures.

We have only two options here:

1. The "correct" approach. Postpone this issue on #2820364: Entity + Field + Property validation constraints are processed in the incorrect order, which is changing something quite fundamental in the Entity/Field API, and which has been blocked on maintainers of that subsystem for months.
2. The "practical" approach. Revert the changes made to EntityResourceTestBase in #2825973: Introduce a EditorialContentEntityBase class for revisionable and publishable entity types, and instead keep testing the order in which PATCH-protected fields are validated.

I vote we go for #2. That means progress now, rather than being held hostage by a chain of issues in another subsystem. Let's be pragmatic.

This effectively reverts all changes to EntityResourceTestBase made in #2825973, and instead updates those tests' static $patchProtectedFieldNames properties that are affected by this.

d.o--

This makes sense to me, we could be waiting a long time for this to get resolved via the entity validation system... This is pretty minimal changes to the coverage too. So we have to systematically iterate and remove the patch protected fields as it usually fails on the first one? If so, just wondering if it would be better to go the other way, just add the one specific patch protected field to each request? or do you want to make sure you test multiple in one go?

Other than that clarification, agree this looks great.

Looks like #2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info) struck.

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -86,6 +74,34 @@ protected function constructValue($data, $context) {
   +   * @return array
   +   */
   +  protected function normalizedFieldValues(FieldItemInterface $field_item, $format, array $context) {
   

   Let's document the array structure. This method is overridden so it'd be useful.

2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php
   --- a/core/modules/hal/tests/src/Kernel/EntityTranslationNormalizeTest.php
   +++ b/core/modules/hal/tests/src/Kernel/EntityTranslationNormalizeTest.php
   
   +++ b/core/modules/hal/tests/src/Kernel/EntityTranslationNormalizeTest.php
   @@ -43,12 +43,12 @@ public function testNodeTranslation() {
   -      'uid' => $user->id(),
   +      'uid' => (int) $user->id(),
          'type' => $node_type->id(),
          'status' => NodeInterface::PUBLISHED,
          'langcode' => 'en',
   -      'promote' => 1,
   -      'sticky' => 0,
   +      'promote' => TRUE,
   +      'sticky' => FALSE,
   

   Why are these changes in scope? Test appears to pass without them.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -902,15 +963,19 @@ public function testPatch() {
   -    foreach (static::$patchProtectedFieldNames as $field_name) {
   -      $normalization = $this->getNormalizedPatchEntity() + [$field_name => [['value' => $this->randomString()]]];
   -      $request_options[RequestOptions::BODY] = $this->serializer->serialize($normalization, static::$format);
   +    // First send all fields (the "maximum normalization"). Assert the expected
   +    // error message for the first PATCH-protected field. Remove that field from
   +    // the normalization, send another request, assert the next PATCH-protected
   +    // field error message. And so on.
   +    $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
   +    for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
   +      $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i));
   +      $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
   

   This is kinda sad that we're back to this construction. I tried to run without this change but Drupal\Tests\rest\Functional\EntityResource\Comment\CommentJsonCookieTest fails. I guess this is because the order of the normalisers has changed.

1. Documented!
2. Yeah, I think it was more for consistency, but not needed so reverted.

#124.3: See #113 + #116.

Committed da74f22 and pushed to 8.4.x. Thanks!

diff --git a/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
index c9a6290..a275f64 100644
--- a/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
+++ b/core/modules/rest/tests/src/Functional/BcTimestampNormalizerUnixTestTrait.php
@@ -1,6 +1,5 @@
 <?php
 
-
 namespace Drupal\Tests\rest\Functional;
 
 /**
diff --git a/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
index 8f6d8ad..704b22f 100644
--- a/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
+++ b/core/modules/serialization/src/Normalizer/TimestampItemNormalizer.php
@@ -4,7 +4,6 @@
 
 use Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem;
 use Symfony\Component\Serializer\Exception\InvalidArgumentException;
-use Symfony\Component\Serializer\Exception\UnexpectedValueException;
 
 /**
  * Converts values for TimestampItem to and from common formats.
@@ -40,5 +39,4 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
     return parent::denormalize($data, $class, $format, $context);
   }
 
-
 }

Fixed coding standards on commit.

I can't quite believe this is finally in… :)

Updated #2852860: REST: top priorities for Drupal 8.4.x with the happy news :)

WOW!