Problem/Motivation

DateTimePlus doesn't apply the correct time zone when converting a string with an offset.

Per the documentation for \DateTime::__construct():

The $timezone parameter and the current timezone are ignored when the $time parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

See also https://3v4l.org/S9r0q

Proposed resolution

This a documentation bug; update the docblocks to align with \DateTime::__construct()

Remaining tasks

User interface changes

API changes

Data model changes

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Status: Active » Needs review
FileSize
1.34 KB

Here is a demo.

There was 1 failure:

1) Drupal\Tests\Component\Datetime\DateTimePlusTest::testDateTimezone with data set #4 ('2007-01-31T21:00:00-05:00', 'America/New_York', 'America/New_York', 'DateTimePlus uses the specifi...vided.')
DateTimePlus uses the specified timezone if provided.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'America/New_York'
+'-05:00'

mpdonadio’s picture

Better test-only version.

mpdonadio’s picture

Issue summary: View changes

The last submitted patch, 2: 2889814-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 2889814-test-only.patch, failed testing. View results

mpdonadio’s picture

Issue summary: View changes

I'll update the test-only patch to cover the timestamp case once I verify that it is also broken.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
3.42 KB
1.81 KB

This

- Updates the docs to match what the code does with timezones for the constructor, and to align it with what the test coverage is.
- Fixes the bug described in the IS
- Adds some test coverage

The static create methods also needs to be fixed...

Status: Needs review » Needs work

The last submitted patch, 8: 2889814-08.patch, failed testing. View results

jhedstrom’s picture

Interesting. This passes locally, but not on testbot... I wonder if we need to explicitly set the server timezone for the tests or something? Or perhaps its a php version issue (I have 7 locally)?

mpdonadio’s picture

KTB should have a default timezone from the phpunit bootstrap, which I have a related issue to sanity check: #2889803: Add test coverage for default time zone in tests.

11 hours def suggests a 'Australia/Sydney' vs 'UTC' prob, as I am pretty sure that date is in DST then and therefore UTC+11.

Haven't tried that locally yet or dug into it much.

mpdonadio’s picture

This fails for me locally in all PHP versions.

    // 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());

The problem is the values all have offsets in them. Old behavior was to ignore the system one; patch changes this.

So, the question is whether we

1) move forward with the patch, and adjust this test
2) consider this a documentation bug, and just update docblocks

Kinda leaning towards (2) right now. It aligns better with how \DateTime works, and I also fear that this may cause BC problems for anyone who is relying on the behavior.

?

mpdonadio’s picture

Issue summary: View changes
jhedstrom’s picture

I think updating the documentation to how things currently behave and keeping things inline with \DateTime makes sense.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +documentation
FileSize
2.02 KB

Docblock fix. Text is lifted nearly verbatim from the note on http://php.net/manual/en/datetime.construct.php

I already rolled and attached the patch, but wondering if the @see should be moved from @param mixed $timezone to the top level of the docblock?

jhedstrom’s picture

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -250,7 +250,11 @@ public static function createFromFormat($format, $time, $timezone = NULL, $setti
+   *   either is a UNIX timestamp (e.g. @946684800) or specifies a timezone

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -34,7 +34,11 @@ class DrupalDateTime extends DateTimePlus {
+   *   (e.g. @946684800) or specifies a timezone

Are the @ symbols intentional here?

mpdonadio’s picture

#16, yes. If you want to parse a unix timestamp directly from the $time string, the format is with a leading @, eg '@1500000000'.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.