Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented

This will be 8.2.x eligible, but since this is green it means we are missing test coverage for this behavior.

Also think we need to add this behavior to Element\DateElementBase so it can be used in both Element\Datelist and Element\Datetime; right now Datelist and Datetime have different logic here (a quick git blame looks like this has been here since the beginning).

Can we move this logic into DateElementBase::processDatetime() and call it from both elements?

We also need a test to demonstrate the problem and to prevent a regression down the road. Probably a kernel test in Drupal\KernelTests\Core\Element?

This is what I was thinking of for the common function. No need to worry about duplicated line; and we want it pas part of the process list anyway.

Need to look at the test later.

elseif (!empty($element['#timezone'])) {
      $element['#date_timezone'] = $element['#date_timezone'];
    }

I think I understand why this line is there. It's connected with something I noted in #2866402: Datetime element's timezone handling is fragile :

If #date_timezone is present, and $date is not a DrupalDateTime and #timezone is empty, then the value of #date_timezone is reset to drupal_get_user_timezone(). But when the form gets saved and the valueCallback gets called again, it will be interpreting times using the original value of #date_timezone regardless of the value set here.

So setting a #timezone property on the element (with value identical to #date_timezone) is a way to make #date_timezone stay correct throughout.

This is of course a stupid, fragile and undocumented hack that it's unlikely anyone is actually using. But perhaps thinking along these lines is the historical cause of what we've got here.

Regardless, I think we should close this issue and find a more comprehensive solution in #2866402: Datetime element's timezone handling is fragile .

@mpdonadio wants to continue in this issue and close #2866402: Datetime element's timezone handling is fragile .

Here's a kernel test that explores the oddities I pointed out there. I'll update the IS here tomorrow, integrate patches, etc.

OK, here are 2 possible solutions:
"clean" which makes things very simple, and "maxBC" which preserves as much BC as possibility for people who are extending this element's class and modifying ::valueCallback or ::getInfo but not ::processDatetime.

I haven't followed @mpdonadio's wish to add a separate process handler for timezones, as this is more complicated than my "clean" solution but less BC preserving than my "maxBC". Given the additional problems that we're now trying to address here, I don't see how it helps us.

(The interdiff shows the differences between these solutions, not with previous patches as these are different approaches to previous).

Major as it can corrupt data and is needed as a prerequisite for more complex time zone handling in #2632040: [PP-1] Add ability to select a timezone for datetime field, which is a major feature request?

I haven't followed @mpdonadio's wish to add a separate process handler for timezones, as this is more complicated than my "clean" solution but less BC preserving than my "maxBC". Given the additional problems that we're now trying to address here, I don't see how it helps us.

The reason for this is because the same general problem exists in the Datelist element, so ideally we would centralize logic in the base class.

the same general problem exists in the Datelist element, so ideally we would centralize logic in the base class

I figured so - but the "clean" solution I suggest removes the need for this kind of code. The basic idea of it is that things already go wrong if #date_timezone is not set AND we don't want to use the user timezone. So therefore we can default #date_timezone to the user timezone, and trust that it is always there and always correct - there's no need to update it. This is the only setup that actually works currently, so we don't need to support more complex setups.

We should probably put an additional process handler in for #2632040: [PP-1] Add ability to select a timezone for datetime field though, as we will then need to update #date_timezone, to reflect the element user's selection of timezone

I really like the 'clean' patch from above much more than the max bc. If anybody is already extending this and messing about with the date_timezone element, it's probably being set/overridden by that extending class, so it's possible perhaps to go with the clean solution here?

I think we need to expand the #16-clean patch to include Datelist.

As requested, #16-clean patch expanded to cover the Datelist elemet as well as Datetime element. This involved quite a bit lot of refactoring of the tests, making the interdiff for the tests (which are 95% of code here) hard to follow.

We have an agreed solution and tests, what we need now is code review of the tests and then we are RTBC.

Sorry, forgot to update the API doc strings as described in the IS. Only a change to the comments, so test-only patch from #27 is still valid.

I've drafted a change record: https://www.drupal.org/node/2880055. Please review.

Self-review of patch line 350:
$message = "The correct timezone should be set on the processed datetime elements: (expected, actual) \n" . print_r($wrongTimezones, TRUE);
needs to be $this->elementType not 'datetime' in the $message string.

I think this looks good. These are some nits.

I want to run the tests locally and play with them before RTBC. This is the weird situation of both a doc bug and a code bug, so I think we will want a Change Record.

Leaving at NR and assigning to myself so I remember to do a final look.

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -6,6 +6,7 @@
   +use DateTimeZone;
   

   We typically don't add use statements for PHP classes in the root namespace.

2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -147,12 +148,8 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   -   *   - #date_timezone: The local timezone to use when creating dates. Generally
   -   *     this should be left empty and it will be set correctly for the user using
   -   *     the form. Useful if the default value is empty to designate a desired
   -   *     timezone for dates created in form processing. If a default date is
   -   *     provided, this value will be ignored, the timezone in the default date
   -   *     takes precedence. Defaults to the value returned by
   +   *   - #date_timezone: The local timezone to use when displaying or
   +   *     interpreting dates. Defaults to the value returned by
       *     drupal_get_user_timezone().
   

   Think this is going to warrant a Change Record.

3. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   +use Drupal\KernelTests\Core\Entity\EntityKernelTestBase;
   +use Drupal\Core\Datetime\Entity\DateFormat;
   +use Drupal\Core\Form\FormStateInterface;
   +use Drupal\Core\Form\FormBuilderInterface;
   +use Drupal\Core\Form\FormInterface;
   +use Drupal\Core\Form\FormState;
   +use DateTimeZone;
   +use Drupal\Core\Datetime\DrupalDateTime;
   +
   

   Should be alphabetical.

4. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   +class TimezoneTest extends EntityKernelTestBase implements FormInterface {
   

   Needs blank line before.

5. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   +    $this->assertTrue(($rightDates === $this->testConditions), $message);
   

   Can use assertEquals (fails show better info to help with debug).

6. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   +    $this->assertTrue((count($wrongTimezones) === 0), $message);
   

   Can use assertCount().

Addresses #31 and #32. Thanks for the quick review @mpdonadio!
The change record is at https://www.drupal.org/node/2880055.

Are we confident that the test-only patch demonstrates the problem, and just covers the tweaks we made to the logic:

1) Drupal\KernelTests\Core\Datetime\Element\TimezoneTest::testDatetimeElementTimesUnderstoodCorrectly
On all elements the time should be understood correctly as 2000-01-01 06:30:00 UTC:
Array
(
[Default date present with default timezone, #date_timezone different] => 2000-01-01 00:00:00
[Default date present with unusual timezone, #date_timezone different] => 2000-01-02 01:30:00
[Default date present with unusual timezone, no #date_timezone] => 2000-01-01 13:00:00
)

I think this does, but want a second opinion as the TZ problems can be maddening at times.

Two small nits, but I am still reviewing the tests.

1. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   + */
   +
   +class TimezoneTest extends EntityKernelTestBase implements FormInterface {
   +
   

   This my bad; I shouldn't have asked for this. The blank between the comment and the class opening needs to be removed (the sniff is "There must be exactly one newline after the class comment"

2. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,392 @@
   +  /**
   +   * Simulate form being loaded and default values displayed to user.
   +   *
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   A form_state object.
   +   * @param \Drupal\Core\Form\FormBuilderInterface $form_builder
   +   *   A form_builder object.
   +   */
   +  protected function setupForm(FormStateInterface &$form_state, FormBuilderInterface &$form_builder) {
   +    $form_id = $form_builder->getFormId($this, $form_state);
   

   This needs a @return. Oddly, PHPCS missed this, but PhpStorm complained when

@mpdonadio wanted it left at NR in #32 so he can do a final review, which he said in #34 he was only part way through.

We also need a second review opinion on the point in #34; it's my IS being discussed so I can't give it.

Ah, back to NR then :) Had set to NW for #35.

Re #35

I think this does, but want a second opinion as the TZ problems can be maddening at times.

I agree that the test demonstrates the issue here after another walk through it.

Back to NW for the nits in #35.

Queueing multiple tests because suspecting a random fail on https://www.drupal.org/pift-ci-job/868537.

We have a clear IS, we have thorough tests and test-only patch (#33), we have a change record, we have agreement in principle from @mpdonadio, we have reviews from @mpdonadio and @jhedstrom.

Are you OK to RTBC @jhedstrom?

Probably a windows/linux issue causing #44 to not apply, let's hope someone with a better dev environment can fix this.

The patch in #44 is UTF-16 encoded; that's why it won't apply.

re-rolled the patch

The PHP 7 tests fails are due to #2918570: Drupal\KernelTests\Core\Image\ToolkitGdTest fails on PHP 7.1.x-dev and 7.0.x-dev following a PHP bugfix, not a problem in this patch.

PHP7 test fails were due to the fact that the patch was tested against 8.3.x, instead of 8.6.x. 8.3.x does not have the fix from #2918570: Drupal\KernelTests\Core\Image\ToolkitGdTest fails on PHP 7.1.x-dev and 7.0.x-dev following a PHP bugfix

This is only a reroll from #51 I have seen that all comments in #35 was addressed in the last patch.

I have only fixed one nit from #51:

+ '#date_timezone' => drupal_get_user_timezone() ,
->
+ '#date_timezone' => drupal_get_user_timezone(),

Sorry no interdiff 57 > 59

- $this->assertTrue((count($wrongTimezones) === 0), $message);
+ $this->assertCount(0, $wrongTimezones, $message);

And now the last CS nit.

Thanks for the effort here, this is a really thorough test, great work.

Just a couple of minor nits/typos.

In terms of the #timezone origins. It looks like it came in with the original patch to add date field to core. I looked into the Date module's history and could not find any places where it existed in their git history, so I suspect it came in with the original date patch.

Searching through core's git history, it came in with #501428: Date and time field type in core. The first time it appeared was in this re-roll by @webchick. I think we're safe to remove it as it isn't documented, but I'll confer with other committers.

1. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,394 @@
   +    // This also set's PHP's assumed time.
   

   nit, sets, not set's

2. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,394 @@
   +   * Initial times are inevitably presented to the user using using a timezone,
   

   two usings here

3. +++ b/core/tests/Drupal/KernelTests/Core/Datetime/Element/TimezoneTest.php
   @@ -0,0 +1,394 @@
   +        $actualTimezone = array_search($actualDate->getTimezone()
   +          ->getName(), $this->timezones);
   ...
   +        $this->assertEquals(drupal_get_user_timezone(), $this->date->getTimezone()
   +          ->getName(), "Test date still set to user timezone.");
   

   not sure we need to break the line here, that's something phpstorm does automatically, but it doesn't help readability, I think it makes it worse

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datelist.php
   @@ -183,17 +179,6 @@ public static function processDatelist(&$element, FormStateInterface $form_state
   -    }
   -    elseif (!empty($element['#timezone'])) {
   -      $element['#date_timezone'] = $element['#date_timezone'];
   -    }
   

   This bit doesn't actually do anything with $element['#timezone'] it just assigns $element['#date_timezone'] = $element['#data_timezone'] which is a no-op.

2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -222,14 +218,6 @@ public static function processDatetime(&$element, FormStateInterface $form_state
   -    }
   -    elseif (empty($element['#timezone'])) {
   -      $element['#date_timezone'] = drupal_get_user_timezone();
   -    }
   -
   

   This could maybe have an effect but isn't the whole point we want to remove the fallback handling here?

Seems fine to me to remove those two hunks.

thanks @vaplas!

Re-rolled.

Thanks @Jo Fitzgerald. Back to RTBC.

Looks good to me as well, @larowlan's feedback was addressed and I think it's ok to perform this change in a minor as plugin classes are not considered part of the public API.

Patch doesn't apply cleanly anymore though.

Re-rolled (deja vu).

thanks Jo

Saving credits.

Committed 5731bc7, pushed to 8.7.x, and published the CR. Thanks!

Tracking down a bug related to this commit and date_recur. I'll dig in deeper but these $date->setTimezone(new \DateTimeZone($element['#date_timezone'])); calls are sometime's DateTimeZone objects and fatal errors occur.

@@ -77,6 +76,7 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
       if (!empty($element['#default_value'])) {
         $date = $element['#default_value'];
         if ($date instanceof DrupalDateTime && !$date->hasErrors()) {
+          $date->setTimezone(new \DateTimeZone($element['#date_timezone']));
           static::incrementRound($date, $increment);
           foreach ($parts as $part) {
             switch ($part) {

I'll try to track down the source

Found the source in date_recur #3015647: $element['value']['#date_timezone'] is not a string fatal error.

It's me being pedantic or due #3015647: $element['value']['#date_timezone'] is not a string fatal error. we should stress '#date_timezone' must be the timezone name/string?
Currently says: "the local timezone" which one may wrongly assume DateTimeZone object is valid?

A follow-up to improve docs would be welcome :)

#3016064: Improve documentation for Datetime and Datelist #date_timezone property created!

Thanks!