Datetime::getInfo() caches user's timezone causing unpredictable timestamps.

Yes this is big problem on multi-location sites that are using Scheduler module.

First reported in #3085947: Entered date is stored using cached timezone from previous user then we realised it was a Core issue.

This function was deprecated in #3009377: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener in Drupal 8.8.0-dev. Are you able to test with the latest 8.8.x branch?

Hi @kim.pepper,
Thanks for linking that issue. I have just tested with 8.8-dev updated as of today, and yes the problem is still there. You are right that in core/lib/Drupal/Core/Datetime/Element/Datetime.php, datetime::getInfo() method now has '#date_timezone' => date_default_timezone_get() not '#date_timezone' =>drupal_get_user_timezone(). However, that does not seem to solve the problem. I think as @partdigital pointed out in the issue summary, the getInfo() method only gets called on cache clear, and that is the key problem.

I also found this problem and tried to solve it as follows:
First, in getInfo() ,delete :

'#date_timezone' => date_default_timezone_get(),

then:

public static function valueCallback(&$element, $input, FormStateInterface $form_state) {
        if(empty($element['#date_timezone'])){
            $element['#date_timezone']=date_default_timezone_get();
        }
       ...
}

@yunke yes that seems like a good approach - did it work?

After further research, I don't think #3009377: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener caused this issue, but it made a presumably already-existing bug much more noticable. Prior to that change, when caches are cleared on the backend (where the default timezone is set to UTC), the cached value at least fell to whatever the site's default timezone was.

In manual testing, this resolves the issue. We'll need tests to ensure against future regression though.

I think this has more to do with dates than the form system, so moving component.

This adds a test that demonstrates the current bug, and the fix. The test-only patch is also the interdiff.

+++ b/core/tests/Drupal/KernelTests/Core/Datetime/DatetimeElementFormTest.php
@@ -58,8 +58,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#date_date_format' => ['Y-m-d'],
-      '#date_time_format' => ['H:i:s'],

@@ -69,8 +69,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#date_date_format' => ['Y-m-d'],
-      '#date_time_format' => ['H:i:s'],

Note, these were straight-up bugs. The values here are strings, not arrays. No error was thrown because DateTimePlus catches all exceptions and logs them.

This is an easier to understand test.

Test looks really nice to me, straight forward and to the point, nice one!

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -69,6 +71,10 @@ public function getInfo() {
+    if (!isset($element['#date_timezone'])) {
+      $element['#date_timezone'] = date_default_timezone_get();
+    }

Über-nit: This could be a 1 liner

$element['#date_timezone'] = $element['#date_timezone'] ?? date_default_timezone_get();

Looking forward to using

<?php
$element['#date_timezone'] ??= date_default_timezone_get();
?>

in PHP 7.4 :-)

Looks great

The one-liner could also be

$element += ['#date_timezone' => date_default_timezone_get()];

+++ b/core/tests/Drupal/KernelTests/Core/Datetime/DatetimeElementFormTest.php
@@ -58,8 +58,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#date_date_format' => ['Y-m-d'],
-      '#date_time_format' => ['H:i:s'],
+      '#date_date_format' => 'Y-m-d',
+      '#date_time_format' => 'H:i:s',

Good catch! I agree this is ready on green.

> Prior to that change, when caches are cleared on the backend (where the default timezone is set to UTC), the cached value at least fell to whatever the site's default timezone was.

Confused by this. What exactly is backend here? If you talk about /admin/something or the clear cache admin_toolbar link then that's just a regular request, the other issue doesn't care if it's frontend or backend.

But maybe backend here is actually drush or console.. and maybe they're not triggering the request subscriber that sets the default timezone. So that *would* be a regression of that issue, but I'd say that maybe drush should be responsible for setting that then?

But maybe backend here is actually drush or console..

That's what I meant by backend. No user is set when doing a drush cr, so whatever date.timezone value is set in php.ini is used, in my case, UTC.

So that *would* be a regression of that issue

It's a regression, but the bug of caching whatever user or non-user had set in the Datetime::getInfo() was already a bug, this just makes it much more obvious. Either way, this should fix both I think :)

Updating the IS to reflect the notes in #22. It would be great to get this fixed.

The fix and test looks fine to me.

Despite 3 different nitpicks on the same line, I think the current line to set the default is also fine :)

And I posted in https://github.com/drush-ops/drush/issues/1166#issuecomment-575320943 about the more general problem that might also affect other places that rely on the timezone being set correctly in drush.

Patch #22 was passing before (many times), but failed this morning, so I re-queued it, and it passed. Setting back to RTBC as per #24

Committed 3d9ac25 and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!

Patch for Drupal core 8.7.x