Add a format constraint to DateTimeItem to provide REST error message

Thanks for this. This got lost in the shuffle.

1. +++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php
   @@ -0,0 +1,28 @@
   +      $time = strtotime($date_input);
   +      if (date('Y-m-d\TH:i:s', $time) != $date_input) {
   +        $this->context->addViolation($constraint->message, array('@datetime' => $date_input));
   

   Hmmm. Rather not use the date function. Can we do something like

     if (\DateTime::createFromFormat(DATETIME_DATETIME_STORAGE_FORMAT, $date_input) === FALSE) {
       $this->context->addViolation($constraint->message, array('@datetime' => $date_input));
     }
   

   instead? Probably also need to add logic to check for date+time vs date-only.

2. +++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php
   @@ -0,0 +1,28 @@
   +        $this->context->addViolation($constraint->message, array('@datetime' => $date_input));
   

   Nit, short array syntax.

Also need explicit test coverage in DateTimeItemTest, and do something similar for daterange?

Going to work on this for a bit tonight.

Tagging out for the night.

The changes to DateTimeItemTest are a bit radical, but needed and in-scope if we are adding this constraint. One, some of the changes are needed to ensure the constraint gets called. Two, both date+time and date-only need to be tested. C, if you look closely at a few of these changes, you will see that a lot of DateTimeItemTest was invalid and the problems only surfaced when the constraint was added.

DateTimeItemTest passes locally, but didn't look at other fails from #3. We'll see what happens with this.

Still need to add the daterange constraint.

As a side note, I am tempted to call this a bug since we aren't currently catching some invalid uses, and this can result in PHP fatals. However, this may be disruptive for 8.2.x (esp outside of core).

+++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
@@ -108,16 +192,118 @@ public function testSetValue() {
+   * @expectedException \PHPUnit_Framework_AssertionFailedError
+   */

+++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
@@ -108,16 +192,118 @@ public function testSetValue() {
+   * @expectedException \PHPUnit_Framework_AssertionFailedError

Need to use `setExpectedException()`, https://www.drupal.org/node/2808063#comment-11753147

1. +++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php
   @@ -0,0 +1,30 @@
   +      /* @var $datetime_item \Drupal\datetime\Plugin\Field\FieldType\DateTimeItem */
   

   Should be typehinted to the interface, \Drupal\Core\TypedData\Type\DateTimeInterface.

2. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
   @@ -64,12 +82,50 @@ public function testDateTimeItem() {
   +   * Tests using entity fields of the date+time field type.
   

   s/date+time/date/

   (because date only?)

3. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
   @@ -108,16 +192,118 @@ public function testSetValue() {
   +      // Valid ISO 8601 dates, but unsupported by DateTimeItem.
   ...
   +      // Valid date strings, but unsupported by DateTimeItem.
   

   Where can we find documentation that explains _why_ these are unsupported?

4. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
   @@ -108,16 +192,118 @@ public function testSetValue() {
   +      ['2014-01-01T20:00:00Z'],
   +      ['2014-01-01T20:00:00+04:00'],
   

   Especially these, they seem very common?

5. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
   @@ -108,16 +192,118 @@ public function testSetValue() {
   +      // Proper format for different field setting.
   +      ['2014-01-01'],
   

   "proper", which to me reads as "valid", but this is the data provider listing all invalid values. So… I'm confused.

6. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
   @@ -108,16 +192,118 @@ public function testSetValue() {
   +      // Valid date strings, but unsupported by DateTimeItem.
   ...
   +      // Proper format for different field setting.
   +      ['2014-01-01T20:00:00'],
   

   Same questions here.

Thanks Wim.

Re 12.3 and 12.4, I am not sure we really document this anywhere. When setting the value, you need to use the storage format (DateTimeItem doesn't implement its own setValue, and the computed value uses DateTimeComputed which does a `DrupalDateTime::createFromFormat($storage_format, $value, DATETIME_STORAGE_TIMEZONE);`. So, I added some common uses where people would use well-known formats to make sure they fail appropriately for the scope of this issue (just add a constraint to enforce how DateTimeItem works). We should have a followup to change this behavior to normalize values to what storage accepts.

Re 12.5 and 12.6, that is just me not figuring out a good way to comment that. Those cases are covering trying to store a date-only value in a date+time field, and vice versa. By "proper" I meant they are in the correct storage format, but for the wrong type. DateTimeItemTest in HEAD is invalid because of this.

#12.1

Should be typehinted to the interface, \Drupal\Core\TypedData\Type\DateTimeInterface.

Don't get this? $value is a DateTimeItem, which extends FieldItemBase and doesn't implement that (the item's value does)?

Working on this over the weekend. I see what the additional fails are and where they are coming from, but I don't understand why they are happening.

Work in progress for a full test run. This it will be green.

I think my confusion is when the constraint gets used, and the fact that we have two widgets for the item, one uses a string and the other that uses the array of parts. I suspect using validation on the value DataDefinition would be better, but we need to get the format which is in the item. And then datetime and date both use datetime_iso8601 for the DataDefinition, but format doesn't match our storage (see #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken).

Very befuddled right now.

Just discovered that Symfony 3 includes a DateTimeNormalizer: http://api.symfony.com/3.1/Symfony/Component/Serializer/Normalizer/DateT....

Still need to review #15.

Unassigning myself. Also removing Needs Tests, since it looks like they got added.

@mpdonadio: Does this mean you will not have time in the near future to push this further? Or does it mean you don't ever intend to continue this?

Wim, just unassigned myself since I am not actively working on it right now.

I think #15 needs a review in general, and also some thoughts around the questions in that comment.

I plan on wrapping this up once I have some better direction, as it looks like this may be important for some some recent bugs that were uncovered.

There is some test overlap changes with this and #2832264: DateTimeItemTest is not valid. Probably want that in first, then we can fold in the specific changes for this patch.

This patch should also remove some of \Drupal\Core\Datetime\Element\Datetime::validateDatetime() since it should no longer be necessary to check for $form_state->setError($element, t('The %field date is invalid. Please enter a date in the format %format.', array('%field' => $title, '%format' => static::formatExample($format))));

This means we need to make this constraint work (or something like it) work for DateRangeItem as well.

Needs reroll b/c #2832264: DateTimeItemTest is not valid. Working on that now, and will also start on some of the other work.

Think this is a good re-roll, and passes locally. Starting on some of the work.

That was fun. The problem was DateTimeWidgetBase::massageFormValues() not actually doing anything to the item when the form element didn't convert the user input into a DrupalDateTime object. In the case of the DateTimeDatelistWidget, the $value was the array input form element.

Re #21, developers can use that form element on their own, so I am not sure how we can get rid of any of that validation?

If we also add to daterange, we probably want to add to timestamp/created/changed. In this issue, or two followups?

So #25 addressed #21.

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraint.php
@@ -14,6 +14,14 @@
+  public $bad_type = "The datetime value '@value' is invalid for the format '@format'";
...
+  public $bad_format = "The datetime value '@value' is invalid for the format '@format'";

These messages are identical. Do we want that?

I don't see where we are testing that the appropriate constraint violation error message is being sent. Not in REST, not outside REST. Am I overlooking that?

If we also add to daterange, we probably want to add to timestamp/created/changed. In this issue, or two followups?

I'd say follow-ups. Or do you have good reasons for those not to be follow-ups? For timestamp/created/changed, we already have #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX. So I'm not entirely sure what you mean?

For the first point in #26.

Thought I had posted this. Guess not.

Do we want to split out DateTimeItemTest to test the separate exception messages? I started down that road, but

$entity = EntityTest::create();
$entity->set('field_datetime', [ '2014', '01', '01', '00', '00', '00']);
$this->entityValidateAndSave($entity);

results in

There was 1 failure:

1) Drupal\Tests\datetime\Kernel\DateTimeItemTest::testDatetimeValidationType
Failed asserting that exception message 'Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime:
    field_datetime: this field cannot hold more than 1 values. (code 756b1212-697c-468d-a9ad-50dd783bb169)
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.0:
    The datetime value '2014' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.1:
    The datetime value '01' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.1.value:
    This value should be of the correct primitive type.
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.2:
    The datetime value '01' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.2.value:
    This value should be of the correct primitive type.
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.3:
    The datetime value '00' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.3.value:
    This value should be of the correct primitive type.
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.4:
    The datetime value '00' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.4.value:
    This value should be of the correct primitive type.
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.5:
    The datetime value '00' is invalid for the format 'Y-m-d\TH:i:s'
Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).field_datetime.5.value:
    This value should be of the correct primitive type.
' contains 'The datetime value should be a string.'.

Not 100% sure how to handle this scenario in a kernel test; in the UI the validation is happening in a different place.

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraint.php
@@ -17,7 +17,7 @@ class DateTimeFormatConstraint extends Constraint {
+  public $bad_type = "The datetime value '@value' should be a string.";

s/should/must/ ?

And yes, ideally we want explicit test coverage for both. However:

Do we want to split out DateTimeItemTest to test the separate exception messages? I started down that road, but …

I don't think that's necessary. I think we can simply set the expected message by passing the second parameter to $this->setExpectedException(). I tried that, and ended up with exactly the same output as you. This is happening because of the order that constraints are evaluated in. That will require #2820364-20: Entity + Field + Property validation constraints are processed in the incorrect order to be fixed, which will likely take months at best, years is not unlikely…

This solves the problem and proves via tests with not 100% accuracy but definitely 90% accuracy that this is working as expected. Let's go with this for now.

Thanks!

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!

This is going to fail.

1) Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest::testDelete
Drupal\Core\Entity\EntityStorageException: 'field_storage_config' entity with ID 'entity_test.field_datetime' already exists.

Wuh? I don't understand this? OK, It's a few minutes later and I already rolled and uploaded this (and I am tired). I think I need to do the FieldStorageConfig / FieldConfig in the setUp method...

-{"message":"The datetime value \u00272017-03-01\u0027 is invalid for the format \u0027Y-m-d\\TH:i:s\u0027"}
+{"message":"Unprocessable Entity: validation failed.\nfield_datetime: field_datetime: this field cannot hold more than 1 values.\nfield_datetime.1: The datetime value \u00272017-03-01\u0027 is invalid for the format \u0027Y-m-d\\TH:i:s\u0027\n"}

I don't get the first part about more than one value?

Couldn't sleep, clown would eat me.

Should be green now. @WimLeers, if you like this test I will clone it to the date-only flavor.

Eh…

01:12:57.618 Fatal error: Cannot redeclare class Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest in /var/www/html/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php on line 18

That's so weird, because there's nothing else that's declaring this! I can't reproduce the failing test. So, retesting.

1. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +    $fieldStorage = FieldStorageConfig::create([
   +      'field_name' => 'field_datetime',
   +      'type' => 'datetime',
   +      'entity_type' => 'entity_test',
   +      'settings' => ['datetime_type' => DateTimeItem::DATETIME_TYPE_DATETIME],
   +    ]);
   +    $fieldStorage->save();
   ...
   +    $field = FieldConfig::create([
   ...
   +    $field->save();
   

   Remove $fieldStorage and $field, just call ->save() immediately.

2. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +    // Reload entity so that it has the new field.
   +    $this->entity = $this->entityStorage->load($this->entity->id());
   +    $this->entity->set('field_datetime', ['value' => '2017-03-01T20:02:00']);
   +    $this->entity->save();
   

   I don't think this is necessary with the createEntity() override?

3. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +  protected function getExpectedNormalizedEntity() {
   

   You could simplify this: you could do:

   return  parent::getExpectedNormalizedEntity() + ['field_datetime' => …];
   

4. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +  protected function getNormalizedPostEntity() {
   

   Same here.

5. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +      $message = "Unprocessable Entity: validation failed.\nfield_datetime.0: The datetime value must be a string.\nfield_datetime.0.value: This value should be of the correct primitive type.\n";
   +      $this->assertResourceErrorResponse(422, $message, $response);
   

   YAY!

6. +++ b/core/modules/datetime/tests/src/Functional/EntityResourceEntityTest/EntityTestDatetimeTest.php
   @@ -0,0 +1,171 @@
   +      $message = "Unprocessable Entity: validation failed.\nfield_datetime.0: The datetime value '2017-03-01' is invalid for the format 'Y-m-d\\TH:i:s'\n";
   +      $this->assertResourceErrorResponse(422, $message, $response);
   

   YAY!

if you like this test I will clone it to the date-only flavor.

That'd be splendid! We'll have super solid test coverage then :) We won't have to worry about this ever again!

Thanks.

#35-2, I'll play with this some more tonight. The base classes have some assumptions about how the IDs start out and progress through the test, and the parent class creates the entity before the child class can add the new fields. I ran into a situation where either the get tests would fail b/c the new field wasn't populated properly (I think because it uses the entity from the setUp) or the post/patch/delete tests would fail because the IDs got out of sync.

Alright, things that can't be simplified can stay this way. But I think the get*Normalized*Entity methods can be simplified, and those are the ones with the biggest gains.

(Also: if you see how you can remove that assumption from the base class without massive changes, removing those assumptions would be fine too.)

#35 should take care of except for (2). I can't see how to work around how EntityTestResourceTestBase::setUp() handles creating $this->entity and get the field in EntityTestDatetimeTest.

Also did some additional tidying up to make this less fragile.

This passes locally.

If this passes on the bot, do we want the date-only version?

Be gone unused `use`!

Still don't see the bot doesn't like this.

Think I figured this out.

#42: HAH! Good find :)

#39: Yay! Yes, I do think we want a date-only field to also have explicit test coverage. NW just for that. This is very close now! :)

OK, this should be all green.

Added a test class for the date-only flavor of datetime fields, and also fixed a bunch of things phpcs was complaining about (esp since these are mostly new files).

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateonlyTest.php
@@ -0,0 +1,143 @@
+      $normalization[static::$fieldName][0]['value'] = '2017-03-01T01:02:03';
...
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The datetime value '2017-03-01T01:02:03' is invalid for the format 'Y-m-d'\n";

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDatetimeTest.php
@@ -0,0 +1,143 @@
+      $normalization[static::$fieldName][0]['value'] = '2017-03-01';
...
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The datetime value '2017-03-01' is invalid for the format 'Y-m-d\\TH:i:s'\n";

Awesome!

Wonderful work here, @mpdonadio :) Thanks!

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php
@@ -0,0 +1,37 @@
+        if (\DateTime::createFromFormat($format, $value) === FALSE) {

Shouldn't we do exactly what is done in \Drupal\datetime\DateTimeComputed::getValue() and use DATETIME_STORAGE_TIMEZONE too. Plus shouldn't we check the result of hasErrors() method on the return?

\Drupal\Component\Datetime\DateTimePlus::createFromFormat() can also throw exceptions - do they need to be handled here too?

Also in the issue summary it says:

it was not obvious if the client should provide datetime fields in UTC or a timezone.

The answer is that the time should be UTC - but we're still not really telling people that the service that posts the dates to Drupal that they are responsible for sending them in UTC. Not sure how to do that though.

Also there is a BC break here? Are there values that we used to accept but we're not? I don't think so but I just want to confirm that.

#47 raises some actionable issues. I think reworking this to use DateTimePlus::createFromFormat() is the better plan here. May end up with one more constraint message from the ambiguous date check.

I found an obscure bug in DateTimePlus, #2858295: DateTimePlus doesn't track errors properly.

I'll finish this up tonight. Should be quick, and I have an idea to make the tests a little more robust / futureproof.

Ok, I think #47 is take care of.

Changed tests are green, let's try a full run.

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraint.php
@@ -28,4 +28,11 @@ class DateTimeFormatConstraint extends Constraint {
+   * Message for when the value did now parse properly.

s/now/not/

#47:

The answer is that the time should be UTC - but we're still not really telling people that the service that posts the dates to Drupal that they are responsible for sending them in UTC. Not sure how to do that though.

We automatically convert to UTC, i.e. to

/**
 * Defines the timezone that dates should be stored in.
 */
const DATETIME_STORAGE_TIMEZONE = 'UTC';

Also there is a BC break here? Are there values that we used to accept but we're not? I don't think so but I just want to confirm that.

Confirming that this is not the case.

@mpdonadio Please confirm :)

[Saving my crosspost, will look at #53 later and post an update; letting #52 run to see if anything broke.]

+++ b/core/modules/datetime/tests/src/Functional/EntityResource/EntityTest/EntityTestDateonlyTest.php
@@ -0,0 +1,154 @@
+      $request_options[RequestOptions::BODY] = $this->serializer->encode($normalization, static::$format);
+      $response = $this->request($method, $url, $request_options);
+      $message = "Unprocessable Entity: validation failed.\n{$fieldName}.0: The datetime value must be a string.\n{$fieldName}.0.value: This value should be of the correct primitive type.\n";
+      $this->assertResourceErrorResponse(422, $message, $response);

This is just an aside for @WimLeers and the other REST maintainers to ponder. This is a little messy, and I essentially had to look at the error to get the full message instead of knowing it from the constraint (IOW, the constraint just added one of the errors in the full response, but I have to look for all of them). What could be a nice enhancement to the REST test framework would be a `assertResourceErrorResponseContains` that takes the $response, explodes it on newlines, and then does an in_array() for the $message that you pass in. Not in-scope for this, but could be a nice followup that could reduce the fragility of hardcoding the full error message here.

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraint.php
@@ -0,0 +1,38 @@
+  /**
+   * Message for when the value isn't a string.
+   *
+   * @var string
+   */
+  public $badType = "The datetime value must be a string.";
+
+  /**
+   * Message for when the value isn't in the proper format.
+   *
+   * @var string
+   */
+  public $badFormat = "The datetime value '@value' is invalid for the format '@format'";
+
+  /**
+   * Message for when the value did now parse properly.
+   *
+   * @var string
+   */
+  public $badValue = "The datetime value '@value' did not parse properly for the format '@format'";

So, do we want to add the UTC language to all of these? UTC is only needed for date+time; for date-only, the time portion is ignore so the date is always the local time (see #2739290: UTC+12 is broken on date only fields). Just want to be sure before I split these out into six messages instead of three.

Re, BC, we have this in the patch

+++ b/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php
@@ -0,0 +1,56 @@
+        $datetime_type = $item->getFieldDefinition()->getSetting('datetime_type');
+        $format = $datetime_type === DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT : DATETIME_DATETIME_STORAGE_FORMAT;
+        $date = NULL;
+        try {
+          $date = DateTimePlus::createFromFormat($format, $value, new \DateTimeZone(DATETIME_STORAGE_TIMEZONE));
+        }
+        catch (\InvalidArgumentException $e) {

My new local version uses DrupalDateTime instead of DateTimePlus so we full match other places, such as

DateTimeComputed::getValue() has

  $datetime_type = $item->getFieldDefinition()->getSetting('datetime_type');
    $storage_format = $datetime_type === DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT : DATETIME_DATETIME_STORAGE_FORMAT;
    try {
      $date = DrupalDateTime::createFromFormat($storage_format, $value, DATETIME_STORAGE_TIMEZONE);
      if ($date instanceof DrupalDateTime && !$date->hasErrors()) {

so that validates precisely to the two defined storage formats.

DateTimeWidgetBase::massageFormValues() has

   if (!empty($item['value']) && $item['value'] instanceof DrupalDateTime) {
        $date = $item['value'];
        switch ($this->getFieldSetting('datetime_type')) {
          case DateTimeItem::DATETIME_TYPE_DATE:
            // If this is a date-only field, set it to the default time so the
            // timezone conversion can be reversed.
            datetime_date_default_time($date);
            $format = DATETIME_DATE_STORAGE_FORMAT;
            break;

          default:
            $format = DATETIME_DATETIME_STORAGE_FORMAT;
            break;
        }
        // Adjust the date for storage.
        $date->setTimezone(new \DateTimezone(DATETIME_STORAGE_TIMEZONE));
        $item['value'] = $date->format($format);
      }

So, both the computed value and the field widget use the defined storage formats.

#2832264: DateTimeItemTest is not valid cleaned up DateTimeItem test, and this patch adds coverage of invalid values in that. The DateTimeFieldTest tested invalid values.

I think we are fully backwards compatible with the advertised API for this field (the two storage constants in datetime.module). I think it may be possible that you could use the FieldAPI to set a value that doesn't follow the format and get it saved into the database, but with the way DateTimeComputed::getValue() is written, I don't see how anything could really work since it validates to the defined formats.

And

The answer is that the time should be UTC - but we're still not really telling people that the service that posts the dates to Drupal that they are responsible for sending them in UTC. Not sure how to do that though.

and

We automatically convert to UTC, i.e. to ...

@alexpott has a point here, and I missed addressing that in #52 when I picked this back up after the side issue.

When you use the field via the UI, the date you enter gets converted to UTC (see DateTimeWidgetBase::massageFormValues). If you use the Field API or REST, UTC is implied; the date formats for storage do not have a timezone explicitly specified, and the items do not implement FieldItemInterface::preSave() to do any conversion, so the user has to make sure the value is already UTC. If we reject based on the format, we should also mention the UTC thing for date+time or local for date-only.

If we could go back in time, storage for date+time should have the offset included, always adjusted to UTC, and there should probably be a preSave() to enforce this, and possible some other item-specific things from FieldItemInterface. But, this is where we are :/ See also #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken.

The constraint and the normalizer issue will help matters, though.

#54: We could do that. But I think this is fine. The fact that it's a bit ugly & painful is good: it shows that we should improve our error responses! In fact, this is really only a problem for entity validation error responses. Obviously, they're hard to parse by REST clients too. We have #2856110: [PP-1] Expose entity validation errors in a machine readable REST API: for that.

#55: so what I wrote in #53 is incorrect? About the automatic conversion to UTC? EDIT: #57 says that what I said was indeed wrong. #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX automatically converts to UTC. This should too. Let's also add explicit test coverage for that.

Not sure what we can do to test UTC conversion here, but I'll ponder it. Essentially, user has to give '2017-03-21T15:00:00' and make sure that is already UTC. I think the best we can do is add the message to the errors. And for '2017-03-21' it is supposed to be the local date for the user setting the value (timezones get ignore for date-only).

15:57:00 <mpdonadio> WimLeers: re those timestamp constraints ... i don't think we can explicitly test the UTC conversion, just add some language to the error message to tell the user that the data needs to be UTC
15:57:02 <Druplicon> https://www.drupal.org/node/2350597 => Extract a common DataContainerInterface for lists and complex data #2350597: Extract a common DataContainerInterface for lists and complex data => 38 comments, 4 IRC mentions
16:31:48 <WimLeers> mpdonadio: If we have automatic conversion to UTC, then we don't need language about UTC in the error message!
16:32:01 <WimLeers> mpdonadio: I always thought this had automati conversion to UTC
16:32:43 <WimLeers> mpdonadio: I thought this did that: +          $date = DateTimePlus::createFromFormat($format, $value, new \DateTimeZone(DATETIME_STORAGE_TIMEZONE));
16:32:51 <WimLeers> mpdonadio: because DATETIME_STORAGE_TIMEZONE === 'UTC'

OK, here is another explanation the time zone thing a different so we can figure out how to proceed.

In storage, date+time field is stored with the format 'Y-m-d\TH:i:s'. An example of this would be '2017-04-05T:02:17:00'. Note that this only has date and time. There is no offset or time zone in that string. This is what we mean that storage is implicitly UTC. Anything that updates storage is responsible for converting to UTC, and anything consuming the field is responsible for converting to the proper zone.

Another way to say this is that DateTimeItem doesn't do anything with timezones. The 'value' is the string; 'date' is a DrupalDateTime object with the TZ set to UTC (see DateTimeComputed::getValue).

On an entity form (simplifying this a bit)

- The time is entered in the user's timezone. The widget then converts to UTC (see DateTimeWidgetBase::massageFormValues).
- The entity storage system then takes this value and shoves it in the database,

In a field formatter

- The value is read out of the database, and set to UTC (lazy via DateTimeComputed::getValue())
- The formatter converts to the proper time zone (see DateTimeDefaultFormatter::formatDate())

When using the Field API directly, you need to use DATETIME_DATETIME_STORAGE_FORMAT, and do the time zone conversion yourself (added some coverage here). REST needs to send in UTC, and convert anything that it consumes to what it needs. Adding conversion to/from in this issue is out of scope (short term we can add normalizers for REST, long term I think we need to do #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken and also convert in a new DateTimeItem::setValue()).

Based on this, how do we want to proceed? Add something to the error messages?

Anything that updates storage is responsible for converting to UTC, and anything consuming the field is responsible for converting to the proper zone.

This is clear. What I was saying in #53 is that I thought this was doing an automatic conversion from any timezone to UTC. I think I must have been context switching too much, because I now think I've been confusing this with #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX!

You see, that issue is adding timezone conversion. In fact, you saying all these things made me doubt that it was working there. But AFAICT, it is working there — see #2768651-107: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, in which I added explicit test coverage for this.

So… what I think we want is:

1. This issue can go in as-is, because it adds the necessary field-level validation constraint, with a helpful error message. This will help any code using the Entity/Field API. And it's already a step forward for REST clients.
2. However, as a second step, we'll want to add a denormalizer much like the one we're adding in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, which does support timezones, and simply always converts to UTC.

Because then:

• Point 1 = this issue. Nothing REST-related: no normalizer.
• Point 2 requires adding a DateTimeItemNormalizer. It's not a hard requirement, but it'd help improve the DX for sure. I'm happy to create that follow-up issue for you. And I think it should be blocked on #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, so that #2768651 does all the hard work, and then this follow-up issue would only have to follow the example set by #2768651.

Thoughts?

#62, I think that is a good plan. We had talked about a normalizer for DateTimeItem and DateRangeItem in IRC, but never really documented it anywhere. It would also let us think about how a normalizer and a solution for #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken could be related.

In #30 I asked for functional REST tests. Those were added in #32 and later. I forgot to remove Needs tests in #46, when I RTBC'd this.

Then @alexpott un-RTBC'd this in #47. I gave a partial answer to #47 in #53, and it was wrong. I confused two issues. See #62 for the clear-up.

Created the follow-up I mentioned — I also vaguely remember discussing this in IRC. Now we have an issue to continue in: #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones :)

That means this issue is now fully RTBC-worthy again! Thanks for your patience :)

More JSTB fails:

https://www.drupal.org/pift-ci-job/650067
https://www.drupal.org/pift-ci-job/650091

Going to wait a day or two to set this back to RTBC to see if this is an reliable random failure again (actual quote heard at a sprint) that has an issue / fix.

Let's see if this stays RTBC; there are issues about the randoms this encountered logged on #2829040: [meta] Known intermittent, random, and environment-specific test failures.

Committed a132d45 and pushed to 8.4.x. Thanks!

WOOOT!

I didn't realize this wasn't committed to 8.3.x. I think that's a problem, because it means there's another entire release without validation for data stored for DateTimeItem fields.

I queued a test of #61 for 8.3.x.

This has been waiting to be committed for >1 month now. It seems core committers are not looking at Patch (to be ported) issues.

With 8.4.x-alpha1 looming, I am moving this back to Fixed to help clear up the queue.

+1