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

Support from Acquia helps fund testing for Drupal Acquia logo

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.

Status: Reviewed & tested by the community » Needs work

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

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

Random b/c OutsideIn.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 3fd781f on 8.5.x
    Issue #2889814 by mpdonadio: DateTimePlus doesn't apply the correct time...

  • catch committed bc12195 on 8.4.x
    Issue #2889814 by mpdonadio: DateTimePlus doesn't apply the correct time...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.