DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/DateTimeIso8601.php
@@ -23,10 +24,10 @@ class DateTimeIso8601 extends StringData implements DateTimeInterface {
-        $datetime = DrupalDateTime::createFromArray($this->value);
+        $datetime = DrupalDateTime::createFromArray($this->value, DateTimeItemInterface::STORAGE_TIMEZONE);
       }
       else {
-        $datetime = new DrupalDateTime($this->value);
+        $datetime = new DrupalDateTime($this->value, DateTimeItemInterface::STORAGE_TIMEZONE);

This couples core to the datetime.module, so maybe explicit 'UTC' w/ a comment?

The new assertion looks good, but I think we also need this in TypedDataTest. The problem there is that

    // Date Time type.
    $value = '2014-01-01T20:00:00+00:00';
    $typed_data = $this->createTypedData(['type' => 'datetime_iso8601'], $value);
    $this->assertTrue($typed_data instanceof DateTimeInterface, 'Typed data object is an instance of DateTimeInterface.');
    $this->assertTrue($typed_data->getValue() == $value, 'Date value was fetched.');
    $this->assertEqual($typed_data->getValue(), $typed_data->getDateTime()->format('c'), 'Value representation of a date is ISO 8601');
    $this->assertEqual($typed_data->validate()->count(), 0);
    $new_value = '2014-01-02T20:00:00+00:00';
    $typed_data->setValue($new_value);
    $this->assertTrue($typed_data->getDateTime()->format('c') === $new_value, 'Date value was changed and set by an ISO8601 date.');
    $this->assertEqual($typed_data->validate()->count(), 0);
    $this->assertTrue($typed_data->getDateTime()->format('Y-m-d') == '2014-01-02', 'Date value was changed and set by date string.');
    $this->assertEqual($typed_data->validate()->count(), 0);
    $typed_data->setValue(NULL);
    $this->assertNull($typed_data->getDateTime(), 'Date wrapper is null-able.');
    $this->assertEqual($typed_data->validate()->count(), 0);
    $typed_data->setValue('invalid');
    $this->assertEqual($typed_data->validate()->count(), 1, 'Validation detected invalid value.');
    // Check implementation of DateTimeInterface.
    $typed_data = $this->createTypedData(['type' => 'datetime_iso8601'], '2014-01-01T20:00:00+00:00');
    $this->assertTrue($typed_data->getDateTime() instanceof DrupalDateTime);
    $typed_data->setDateTime(new DrupalDateTime('2014-01-02T20:00:00+00:00'));
    $this->assertEqual($typed_data->getValue(), '2014-01-02T20:00:00+00:00');
    $typed_data->setValue(NULL);
    $this->assertNull($typed_data->getDateTime());

all of those date strings have a UTC offset in them, which implicitly sets them to UTC (kinda, see https://3v4l.org/jctWg). When you parse a string w/ an offset that will override the time zone in the constructor.

Not totally sure what is best here.

+++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
@@ -153,11 +155,45 @@ public function testGetAndSet() {
+    $typed_data->setDateTime(new DrupalDateTime('2014-01-02T20:00:00'));
+    $this->assertSame('+11:00', $typed_data->getDateTime()->getTimezone()->getName());

This had me scratching my head for a while. The default timezone for tests is 'Australia/Sydney', which happens to have a UTC offset of +11:00 on 2014-01-02.

Let's look closer

class DateTimeIso8601 extends StringData implements DateTimeInterface {

  public function getDateTime() {
    if ($this->value) {
      if (is_array($this->value)) {
        // Data of this type must always be stored in UTC.
        $datetime = DrupalDateTime::createFromArray($this->value, 'UTC');
      }
      else {
        // Data of this type must always be stored in UTC.
        $datetime = new DrupalDateTime($this->value, 'UTC'); // NOTE 2
      }
      return $datetime;
    }
  }

  public function setDateTime(DrupalDateTime $dateTime, $notify = TRUE) {
    $this->value = $dateTime->format('c'); // NOTE 1
    // Notify the parent of any changes.
    if ($notify && isset($this->parent)) {
      $this->parent->onChange($this->name);
    }
  }

}

The test does `$typed_data->setDateTime(new DrupalDateTime('2014-01-02T20:00:00'));`. The TZ of the anonymous object passed to `setDateTime()` will be 'Australia/Sydney', per PHP's rules. Set a breakpoint to see.

At "NOTE 1", `$this->value` gets set to '2014-01-02T20:00:00+11:00', which stores the timezone offset and not the timezone name in the string.

At "NOTE 2", we build a new `DrupalDateTime()` from the string '2014-01-02T20:00:00+11:00'. The timezone name gets set to '+11:00' per PHP's rules, not 'Australia/Sydney'. This then is what is in the assertion.

To be honest, I don't think we can really do anything about this. Using '+00:00' in `getDateTime()` smells bad. If we want to be pedantic, then I think the attached interdiff may be best. I am not wed to that though.

My issues about in #6 are addressed to my satisfaction (and sorry for not being more specific about what "kinda" meant).

Does this work have an BC implications. What about existing code?

I think we need a change record to detail the change and the issue summary could be updated to be more explicit about why this change is correct rather than pointing to lots of other issues to work out why.

+++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
@@ -153,11 +155,45 @@ public function testGetAndSet() {
+    $typed_data->setDateTime(new DrupalDateTime('2014-01-02T20:00:00'));
+    $this->assertSame('+11:00', $typed_data->getDateTime()->getTimezone()->getName());
+    $this->assertEqual($typed_data->getValue(), '2014-01-02T20:00:00+11:00');

Given this had a datatime maintainer scratching their head maybe we should add a comment for future us.

I committed this to 8.7.x - as I'm unsure of the implications for contrib if they have used this to manipulate and then store values I'm going to leave there. If there are compelling reasons to backport then we can discuss and do that.

Committed f51bfe1 and pushed to 8.7.x. Thanks!