UTC+12 is broken on date only fields

By any chance, do you know if this worked properly in Drupal 7 with the Date module, and if so which version of Date (one of the later release changed how the dates are actually stored).

Pretty sure the problem is the datetime_date_default_time() that are scattered throughout the module to warp the time on date-only to noon UTC, so they can be treated as DrupalDateTime objects (which derive from \DateTime). Going to have to review some old issues around this. I suspect we need to handle the edge cases in there.

Confirmed with Asia/Anadyr as the TZ (Pacific/New_Zealand wasn't an option for me).

The value in the database is OK. The problem is that

- the value is read from the database, the current time is set on the object.
- DateTimeWidgetBase::formBase() calls datetime_date_default_time(), which sets the time to noon UTC.
- the TZ is applied, so 12 hours are added and we roll into the next day.

I need to dig for the issue where we set this behavior. It used to be set TZ, then set default date, but we had weird problems with lots of date rollovers, and swapped the order. Obviously, we didn't consider all of the edge cases...

Blerch...

#5, if you have a D7 install handy with Date and configured for NZ that you can report on, it would help. My D7 instance is usually in a state of destruction from work on other things.

I'll dig into this a bit.

From the comment on datetime_date_default_time():

If we use noon, dates in most timezones will have the same value for in both the local timezone and UTC.

wouldn't that only apply to timezones that are the same as UTC? It seems more reasonable to just set it to 0, which would resolve the problem entirely I think.

I'll work on a test.

I wasn't able to reproduce this in local testing (I'm at UTC-8 server time, and even setting the timezone to Pacific/Auckland didn't create the issue). I expected this test change to fail, but it doesn't. Uploading here because it might fail on the test bots as expected since they are set to UTC server time.

Oddly, changing this to 0 instead of 12 made the date start decreasing by one day on each save:

function datetime_date_default_time($date) {
  $date->setTime(0, 0, 0);
}

The problem with midnight UTC is that the negative offsets will become the previous day once you apply the timezone offset based on the system settings (eg, I'm UTC-4 at the moment, so 2016/06/03 00:00:00 UTC is 2016/06/02 20:00:00 for me => previous day). When you use noon UTC, most cases will warp to the same day. Rollover happens at the extremes, which is what is happening with this bug.

I keep confusing myself everytime I write up more thoughts. I am going to try to diagram this. It seems like we are in a catch-22 situation. Again.

I'm wondering if we need to back up and hash out in words what a date-only object is and what time it is and where. Then we can back out to whether we are implementing that properly. And taking into account that storage only has the date and not time or timezone, so that will always be implicit.

Still trying to figure out what issue we adjusted this in.

I'm wondering if we need to back up and hash out in words what a date-only object is and what time it is and where

I think the intent of the date-only field type is to not have any timezone handling. The current implementation seems to add timezone handling due to perhaps a limitation in the datetime widget form.

In some of the fixes over in #2627512: Datetime Views plugins don't support timezones, timezone offsets are explicitly removed for any date-only fields.

Updated IS to reflect the removal of timezone offset handling from date-only fields. Going to see how involved this fix is.

Here's a start at removing timezone offset calculations from date-only fields.

I still was unable to get the test to mimic the exact behavior described in the IS (note the 3 subsequent form submissions do not increment the date). However, the test changes do illustrate an end user in a UTC-12 timezone experiencing the date shifting (the submitted 2012-12-31 date gets converted to 12-30, and then later is displayed as 2013-01-01).

Run TestBot, run!

These fixes are against 8.2.x.

and again...

On tablet, so I can't do a proper review. Patch looks mostly good, just some small things.

Still not 100% convinced this is proper approach. I'm going to try to dig for comparisons with other systems. But this, combined with the all day date range in the end date issue would really cover all scenarios.

Anyone else have thoughts?

Just some quick thoughts...

1. +++ b/core/modules/datetime/datetime.module
   @@ -46,11 +46,11 @@ function datetime_help($route_name, RouteMatchInterface $route_match) {
   -  $date->setTime(12, 0, 0);
   +  $date->setTime(0, 0, 0);
    }
   

   Prob need a CR about this in case anything in contrib is using this? We need to @deprecated that function at some point, too..

2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
   @@ -140,7 +141,14 @@ public function settingsSummary() {
   +    if ($this->getFieldSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE) {
   

   Nit. Can we do a === here?

3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
   @@ -33,8 +34,6 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -        else {
   -        }
            $this->setTimeZone($date);
   

   Looks like an accidental change that would get this pushed back?

4. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -62,7 +62,8 @@ protected function setUp() {
   +      // A timezone with an offset greater than UTC+12 is used.
   +      ->set('timezone.default', 'Pacific/Auckland')
          ->save();
    
   

   I'm wondering if we need to defer the TZ configuration to the test itself, and then loop through a set representing extremes at both ends?

Looks like an accidental change that would get this pushed back?

Ha, I thought I was rolling back my own refactoring when I removed that--didn't realize that was already in core that way. Since we're refactoring that method anyway, is there harm in removing code mistakes?

Good call on looping through some timezones. The extremes caught a few more issues.

Note that the interdiff was done with --ignore-all-space to make the indentation change easier to review.

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -54,17 +54,25 @@ class DateTimeFieldTest extends WebTestBase {
+    // UTC+14.

Oops, this should be +12. I wanted to do +14 but couldn't find one that PHP supported.

<code>
+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -874,4 +905,19 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
+      // A timezone with an offset greater than UTC+12 is used.

And this comment is no longer relevant.

I think Pacific/Tongatapu will be the most extreme positive offset at UTC+13 and Pacific/Kwajalein should be UTC-12 which I think it the most negative extreme in PHP.

I went through the time zone list, and picked out some that I think will be good: few at each extreme, few in between, and UTC, all without DST so the test results are predictable:

  /**
   * An array of timezone extremes to test.
   *
   * @var string[]
   */
  protected static $timezones = [
    // UTC-12, no DST.
    'Pacific/Kwajalein',
    // UTC-11, no DST
    'Pacific/Midway',
    // UTC-7, no DST.
    'America/Phoenix',
    // UTC.
    'UTC',
    // UTC+5:30, no DST.
    'Asia/Kolkata',
    // UTC+12, no DST
    'Pacific/Funafuti',
    // UTC+13, no DST.
    'Pacific/Tongatapu'
  ];

I think this warrants a CR about the change, especially to datetime_date_default_time(). Kinda wondering if we should @deprecate that as part of this?

Otherwise, I am OK with this approach, especially if we can get all-day into the end-date patch.

Kinda wondering if we should @deprecate that as part of this?

I was going to mark it as @deprecated, but realized there wasn't a good replacement to direct folks to. We could hard code the 00:00:00 time everywhere it's used and just deprecate it without a replacement?

This patch updates the timezone list to test.

Reviewed this again. I think this is good to go, and paired with the all-day feature from #2161337: Add a Date Range field type with support for end date, will allow better flexibility for user needs. Have Change Record ready.

+++ b/core/modules/datetime/datetime.module
@@ -46,11 +46,11 @@ function datetime_help($route_name, RouteMatchInterface $route_match) {
 function datetime_date_default_time($date) {
-  $date->setTime(12, 0, 0);
+  $date->setTime(0, 0, 0);
 }

Is this change actually necessary? And doesn't this mean we need an upgrade path? What happens to existing fields - do they change when you resave them?

In the current patch, the only thing the change to the default time impacts in the tests is the display part (line 207 of DateTimeFieldTest, with the short/medium/long formatters). Rather than displaying date-only fields with a time of 00:00:00, they display with 12:00:00. In the real world, I'm not sure folks would be using date formats with a time component for date-only fields.

I spoke with @mpdonadio in IRC, and we agree it makes sense to have the test look for the default time if displaying a date-only field with a formatter that includes a time component.

Updated CR to reflect new patch. Attached the interdiff between #34 and #38.

I am OK with not changing the default. The changes to DateTimeFieldTest are easier to read in place because of the loop that was introduced, or as `git diff -b origin/8.2.x`.

#41, we are just waiting for a committer to take another look at this. There is a decent backlog of RTBC issues for them to go through.

Adding blocker tag b/c this is hard blocking and soft blocking some Datetime work that we would really like to get into 8.2.x.

Committed 5694189 and pushed to 8.2.x. Thanks!

The patch does not apply to 8.1.x - I think it might be eligible because whilst it is a behaviour change - the current behaviour is wrong. @mpdonadio if you think it is worth back-porting then feel free to re-open.