DST-related test failures in FilterDateTimeTest

I have two proof-of-concepts for this in my head that I will post later to see if this could be the one-in-a-million fail from a test running during the DST switch. (one is to change the TZ during the test, the other is to sledgehammer REQUEST_TIME to be the same as when that fail ran...).

At a minimum, though, we could update this test do the timezone loop that we use in other places.

@xjm, thanks for making this reproducible!

The patches in #10 won't help, because many/most classes get the time from something like

$request_time = $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME');

The problem is that Drupal\datetime\Plugin\views\filter\Date is using a different timezone offset than the test. It is either because of a rollover during the test, or because REQUEST_TIME in the test is different than $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') in the views plugin. I know what is happening, but not 100% why (it's lines 115-116 of Date::opSimple).

We either touched those lines of code recently, or have an in-progress patch about this right now. Trying to find the related issue.

Actively working on a fix.

Two possible related issues.

OK, this is a a weird one that I am still unravelling.

1478422920 is 2016-11-06T09:02:00 in UTC.

2016-11-06T09:02:00 in UTC is 2016-11-06T01:02:00 in America/Vancouver.

1:02am in Vancouver happened twice: before the DST switch, and then again after the fall back.

This is what is happening. The test simulates the user entering 2016-11-06T01:02:00 in Vancouver for the node value via the timestamp value (the formatter service wants timestamps). This goes into storage as 2016-11-06T09:02:00. The test simulates the user entering 2016-11-06T01:02:00 in Vancouver for the filter. This gets converted to the timestamp 1478419320, which is 2016-11-06T08:02:00 in UTC.

In other words, the timestamp we are operating at is the one after the DST switch. The string 2016-11-06T01:02:00 is getting converted to the one before the DST switch. Test gets confused and fails.

I am debugging this against #2627512-139: Datetime Views plugins don't support timezones, thinking that would solve it (uh, it doesn't). Will reroll my debug patch against HEAD and post tomorrow so everyone can cry with me. Will also investigate why this process isn't symmetric.

Appears to be default behavior in PHP, https://3v4l.org/q5400

Still digging.

Here is the problem expressed as a unit test, because that is what developers do.

I think we need to expressly thread the timezone offset through when doing UI (not just the timezone name). Also need to do some manual testing (or add test coverage) to see if this happens elsewhere, both during DST spring-forward and fall-back.

FWIW, manually testing datetime fields seems to work. We should be able to update DateTimeFieldTest and DateRangeFieldTest to cover it. The method is basically the same as the date-only hunk where we save off the same field values a few times and make sure they don't change and make sure they render out properly.

Thought about this some more. I think if we can get #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter and #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented in, then this will become a non-issue, as the TZ is part of the \Drupal\Core\Datetime\Element\Datetime form element. We would just need to adjust FilterDateTimeTest to properly simulate the TZ handling, or do it as a proper integration test rather a kernel-ish test.

We are triaging this: @cilefen

• The issue summary makes sense.
• Normally random test failures are critical, but as mentioned in the issue summary this happens rarely (only twice a year).
• We made sure the patch still applies to 8.4.x-dev so this doesn't need a re-roll.

I am crediting ourselves on the triage.

The 2017a tzdb introduced a change to Chile, but I forget the details. I'm thinking we need to add a message in system_requirenents to say what database is being used, and also log it in one of the date related tests.

@cilefen pointed me to this today, which I had forgotten about: http://symfony.com/blog/new-in-symfony-2-8-clock-mocking-and-time-sensit...

I'm wondering if with the push to BTB, and more of the testing framework leveraging PHPUnit, we can see if can leverage any of this to stabilize the time / DST related randoms

It's back, but now in \Drupal\datetime\Tests\Views\FilterDateTest https://www.drupal.org/pift-ci-job/636971 from #2858159: The "User has a revision" views argument handler generates a SQL error. Different test, but uses the same static::$date = REQUEST_TIME; logic, so I guess it would run into the same type of fails.

If moving to PHPUnit helps, these tests have no business being a webtest anyway, they never use the browser, so we should convert them to a kernel test for the PHPUnit Initiative anyway.

This #2865992: Convert Datetime module Views tests to Kerneltest converts these tests to kernel tests.

If we know the forward/backwards conversion during the hour of DST change is the problem (see #23), can we just wrap the failing assertion with a check to not run it during this hour? Maybe even explicitly skip (not sure if phpunit lets you skip assertions and not whole test methods). We would still catch a regression the next day.

?

Hi all! I dug through this yesterday and today, and I think I have finally come to a conclusion: Named time zones are ambiguous during the periods clocks are adjusted backward, as we have no way to find the exact UTC offset using just the date-time (no time zone info) and the named time zone such as America/Vancouver, which during that 1 hour most clocks are adjusted when DST ends, can be one of two UTC offsets. This would explain why we did not see any failures last weekend when the EU switched to DST, and I predict we won't see any when North America starts DST soon either, since no date-times will be repeated or ambiguous.

Apart from extending date-times with time zone offsets (needed to disambiguate named time zones), I'm not sure if there is any solution to this.

The case of errors caused by changing the time one hour back will be repeated systematically.

2017

Created 4 Nov 2017 at 09:20 UTC. mairi posted patch. Updated 4 Nov 2017 at 10:12 UTC.
https://www.drupal.org/pift-ci-job/803185

2018

Created 3 Nov 2018 at 08:55 UTC. Automatic re-testing. Updated 3 Nov 2018 at 09:48 UTC.
https://www.drupal.org/pift-ci-job/1111364

Created 3 Nov 2018 at 09:55 UTC. Automatic re-testing. Updated 3 Nov 2018 at 10:43 UTC.
https://www.drupal.org/pift-ci-job/1111369

Created 3 Nov 2018 at 09:25 UTC. Automatic re-testing. Updated 3 Nov 2018 at 10:22 UTC.
https://www.drupal.org/pift-ci-job/1111365

It is wise to omit the first hour after the time change when the clock hand is moved backwards from the tests. This is necessary because of the confirmed effect on automatic tests.

In such cases it is not possible to distinguish between the first hour before and after the time change.

#3018996: DateTimePlus does not create dates with proper timestamps during DST transitions

Notice: The same behavior is in other zones in the Eastern Hemisphere ('Australia/Sydney' or 'Europe/Warsaw').

Because of the suggestion this is what is preventing the test for #2868014: [PP-1] Views Date Filter Datetime Granularity Option from passing, it would be great to see this resolved.

Re: #50: Sadly, this patch doesn't help the tests in #2868014 (see #2868014-95: [PP-1] Views Date Filter Datetime Granularity Option). Still probably worth landing this in its own right, but unfortunately not because it moves that one forward. :(

I haven't totally wrapped my head around this issue, so I'm not going to bump to RTBC.

It does seem like a bit of magic for the test itself to have this much conditional logic. Perhaps this would be better if that conditional logic lived in the formatter itself, and then have tests that show it does the right thing, even when "now" is in the middle of the weird periods of DTS fallback? Or something. Need to read the whole discussion here and dig deeper to really have a clear opinion...

Thanks/sorry,
-Derek

[edited]

Per #30 back to major.

The America/Vancouver time zone changed from daylight savings to standard time at 2:00 AM on Sunday, Nov 1, 2020. The GMT offset is currently UTC/GMT -8 hours (PST).

The tests failed again https://www.drupal.org/pift-ci-job/1868691

Created 31 Oct 2020 at 10:32 CET. Krzysztof Domański added custom test. Updated 31 Oct 2020 at 11:27 CET.

Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
-    0 => Array &1 (
-        'nid' => '4'
-    )
-)
+Array &0 ()

On October 31th because the tested date is set to the day after the REQUEST_TIME (86400 seconds -> 1 day).

static::$date = REQUEST_TIME + 86400;

// Set the timezone.
date_default_timezone_set(static::$timezone);
$this->config('system.date')
  ->set('timezone.default', static::$timezone)
   ->save();

// Add some basic test nodes.
$dates = [
  '2000-10-10T00:01:30',
  '2001-10-10T12:12:12',
  '2002-10-10T14:14:14',
  // The date storage timezone is used (this mimics the steps taken in the
  // widget: \Drupal\datetime\Plugin\Field\FieldWidget::messageFormValues().
  \Drupal::service('date.formatter')->format(static::$date, 'custom', DateTimeItemInterface::DATETIME_STORAGE_FORMAT, DateTimeItemInterface::STORAGE_TIMEZONE),
];

See #3018996: DateTimePlus does not create dates with proper timestamps during DST transitions. In such cases it is not possible to distinguish between the first hour before and after the time change. The reason is a bug in PHP. I don't expect that issue to be resolved in the coming years :( so IMO #46 makes sense to go.

I found two such cases this year. I ran these tests at the wrong hour... :(
https://www.drupal.org/pift-ci-job/1868692
Created 31 Oct 2020 at 10:32 CET. Krzysztof Domański added custom test. Updated 31 Oct 2020 at 11:32 CET.

https://www.drupal.org/pift-ci-job/1868691
Created 31 Oct 2020 at 10:32 CET. Krzysztof Domański added custom test. Updated 31 Oct 2020 at 11:27 CET.

+++ b/core/modules/datetime/tests/src/Kernel/Views/FilterDateTimeTest.php
@@ -38,6 +38,20 @@ protected function setUp($import_test_views = TRUE) {
+    // Change variable $date, in the first hour after the time change. When you
+    // move the clock back, the time is the same again. However, it is treated
+    // as a date before the change. In such a situation, comparing it with the
+    // universal time ('UTC') causes errors.
+    // https://www.drupal.org/project/drupal/issues/2825845
+    $a = $formatter->format(static::$date, 'custom', DateTimeItemInterface::DATETIME_STORAGE_FORMAT, static::$timezone);
+    $b = $formatter->format(static::$date - 3600, 'custom', DateTimeItemInterface::DATETIME_STORAGE_FORMAT, static::$timezone);
+    if ($a == $b) {
+      static::$date -= 3600;
+    }
+

I think something less fragile is to the use 'I' format (see https://www.php.net/manual/en/datetime.format.php) and check whether the test is in the middle of the DST fallback. ($a would be 0 and $b would be 1).

1. Drupal\Core\Datetime\DateFormatter::format() returns incorrect result for format 'c' and 'I'.
See #3018996: DateTimePlus does not create dates with proper timestamps during DST transitions.

2. Instead of Drupal\Core\Datetime\DateFormatter::format() we need \DateTime::format(); here.

$date = new \DateTime();
$date->setTimestamp($timestamp);
$date->setTimezone(new \DateTimeZone("America/Vancouver"));
$custom = $date->format('c');
$isDST = $date->format('I');

3. What when the clock hand is moved one hour forwards? static::$date += 3600;?

+    if ($a == $b) {
+      static::$date -= 3600;
+    }

It was fixed in PHP 8.1.7. Now it works in Drupal 10 and PHP 8.1.7. It still doesn't work in the older version e.g. Drupal 9.5 and PHP 8.0. See #3018996: DateTimePlus does not create dates with proper timestamps during DST transitions.

According to #65 this is fixed in PHP. Closing.