[PP-1] Add ability to select a timezone for datetime field

+1 from me, but I'm not sure if we would be able to get this in 8.0.x (regressions for contrib that made it into core don't automatically count as bugs; it's up to the core maintainer's discretion).

I agree this is a feature request, so bumping version.

I was hoping to get further on this but ran out of time. Here's a very rough start. Current work takes heavily from the 7.x date module's timezone handling.

Thanks. This will need upgrade path, but it will be easy to just add the default setting to all of the field instances.

A start at tests.

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
@@ -61,6 +62,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
+    // @todo Shouldn't this only be needed if `$field_definition->getSetting('timezone_handling') === TRUE`?
+    $properties['timezone'] = DataDefinition::create('string')
+      ->setLabel(t('Timezone'));
+

Just a thought here: wouldn't it be more interesting to add a new field type that stores a date + timezone.
That way we have a clear separation between two fields:

- one that stores dates always stores in UTC
- one that allows storing a date with timezone

That makes the

- the storage per field type very clear, no extra empty column - or worse having to update the tables in case you turn on/off timezone handling.
- it makes the upgrade path easier, as in, there shouldn't be one at all.

Updating IS.

Cross post there. I like the idea of #7 a lot.

Not sure if I like the idea of a different field type. Not totally against it, but would have to sleep on that one. The Drupal 7 Date module also stores the offset in the field table for the selected timezone. I believe it does this to allow date calculation to be handled at the SQL level when necessary.

tldr: I now also think single field type is fine

----

So I thought some more about the split after I found #2161337: Add a Date Range field type with support for end date - theoretically we might end up with following scenario if we'd create different field types for all of them:

- single date
- single date with timezone
- single + end date
- single + end date with timezone

Now, more or less the same scenario exists for text (see #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI) (which is where I got my 'inspiration' from) : plain vs formatted vs small vs long etc. In analogy with that patch it /could/ mean we'd expose TimestampItem as well in field UI some day (there is no reason to hide it imo) which would give us following field types exposed in Field UI

- single date (timestamp)
- single date + end date (timestamp)
- single date (iso8601)
- single date with timezone (iso8601)
- single + end date (iso8601)
- single + end date with timezone (iso8601)

Now, that looks a little silly, but then again, Field UI isn't perfect (ahum), but the field types and the API would be very clean, especially on the storage level, which in the end is probably my main concern as in: why have one (or more) column(s) that wouldn't store anything, right ? And with class inheritance, you call the parent, add a new column and property in the new class and you're done.

However, looking at the current datetime widgets and formatters, it'd especially mean a lot of new formatters and widgets per new field type, which on a first glance will lead to many duplicate code, or a lot of calls to the parent class in case of extending, let alone verbose names as well for the classes/files.

So, in the end, I think adding timezone in the same field type is fine, and the same for the end date too in that other issue.

What @swentel said. I spent a while thinking and working this out on paper yesterday, as well as reading the Drupal 7 Date module and issues, but didn't get a chance to write it up. It gets worse once you consider adding recurrences (which I think need to be in core, too; ugh). I just don't see how you can accomplish this w/o a really bad class hierarchy and bad (or less than proper) OO implementations.

Just reuploading the patch from #6 to get a TestBot run.

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
@@ -80,6 +85,17 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
+    if ($field_definition->getSetting('timezone_handling') === 'date') {
+      $schema['columns']['timezone'] = array(
+        'description' => 'The date timezone',
+        'type' => 'varchar',
+        'length' => 50,
+      );
+    }

I'm not sure anymore what happens in case you'd change the setting when there's no data yet and go from datetime to date: will the db column be added than or not, that's something to consider.

Thanks for tagging this to be worked on this weekend, but we are going to postpone this on #2161337: Add a Date Range field type with support for end date. TZ support will be needed in some manner when we add end dates, and both patches are similar and touch the exact same portions of code.

Removing sprint tags, as this is postponed.

Given the approach now in #2161337: Add a Date Range field type with support for end date, lets unpostpone this and see if we can work on both in parallel.

Let's go with the approach of adding a timezone property to the schema and always saving it in the field data. This will eliminate the dynamic schema.

?

Going to work on this a bit.

Still more to do here. I added some @todo's in the patch, but additionally we will need more tests, and an upgrade path still.

Was thinking about this today. Why do we need to do a schema update for storage? If this is really just about user entry, then why don't we just

- add the setting for the widget
- when we process the entry, warp the date to UTC based on the setting for storage; this is basically what we do now, we just don't give the user the option on how it happens per instance
- when the widget is used for editing, then we just need to use the setting to warp UTC back to what the setting uses

The upgrade path then just needs to add the default setting to add existing field instances (ie, just config), and not have to touch storage.

This has the added benefit that date storage is still sortable w/ simple string comparisons If we either update the storage string to add TZ or add another column for TZ, then things will get messy (especially for the Views plugins).

?

just to share my common use case for needing a timezone being collected (which almost always because a client 'wants visual timezone selection'):

content editor: -- event is going to be a video meeting.

• i'm a content editor, in Ontario. [time zone 0]
• I'm entering adding event content type
• i pick a location (seperate field) -- field not specifically needed, but client asked for field.. IE: not all events are Video..
• --i pick a date AND time ("probably" with end time on same day, 2-4pm..)
• --i want to clarify that this location is in alberta, [time zone3] and that is a different timezone.

end-user reviewing events (personalization)

• i live in Nova Scotia [time zone -1]
• i see a view of events
• i need reminder that event time to be displayed by my user timezone setting -- so i don't miss video meeting [+4hours from me]

@mpdonadio.
i will agree -- shouldn't need separate storage of the field as it should be a whole date stored. that can/will be easily converted come render time with somekind of personalizing to the end user.

it's always an issue of converting an original saved date/time in relation to location of the event it's being connected to... and the end user who's trying to match his schedule for it.

---------------------
or even (on the far side): date field should always have a location widget attached. HAHA.

Timezone Picker

#24, thanks for outlining a use case. We may want to edit the authoring experience into the IS.

Just to clarify, we are talking about just the content authoring experience in this issue. I don't think there is a question of whether this is needed; we are just at the minutiae stage right now for how it gets implemented. We had originally thought that we would need to update how things get stored (which would bring it close to how the D7 Date module is implemented). I'm thinking now that we always store UTC, and that this new option just handles conversion before we store.

As for the user experience, we already have support for that. The field formatters support TZ overrides to force one for a field, else system default behavior is used (from how admin/config/regional/settings is set up).

IOW, handle entry and display as true inverses.

Don't want to hijack this issue (and maybe it is in fact a separate issue, but I don't think so), but I would argue that this is actually a bug report.

If you look into \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::setDateTime() you see that core itself uses PHP's c date format to store a date time. That, though, includes the timezone offest in the 2004-02-12T15:19:21+00:00 format as can be seen on the linked PHP documentation page. So while it does not store the actual timezone identifier (which the patches here are about) it does store information that is in many cases (albeit not all) completely sufficient for timezone-aware date handling.

However, the storage of the datestring is limited to 20 characters, so the timezone makes the string to long. So in fact using DateTimeIso8601::setDateTime() leads to unsaveable entities. Try it yourself:
Given an entity with a datetime field with the ID date:

$datetime = new DrupalDateTime();
// Get a field item, regardless of whether one exits already.
$date_item = $entity->get('date')->first() ?: $this->get('date')->appendItem();
$date_item->get('value')->setDateTime($datetime);

$entity->save();

Seems like proper API usage to me, but it yields:
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'date'

That's what makes this a bug report. And solving the bug would involve supporting time zones as part of datetime fields - at least in the ISO8601-conform offset form. That's why I didn't open a new issue for this.

Thoughts?

Additional problem:
Even setting aside the column length problem, if you have a date of the form 2004-02-12T15:19:21+00:00 in the database \Drupal\datetime\DateTimeComputed::getValue() will never return a proper datetime object because it expects the format Y-m-d\TH:i:s or Y-m-d depending on the storage setting of the date time field.

I'm not convinced we have a problem, more of a happy accident? Maybe this was by design (before my time with this). Not ruling out a problem, thought, considering what I have dug up with TZ handling. However, with the changes we made to testing to run tests in non-UTC, I think we are OK.

Storage uses the ISO date w/o TZ suffix, w/ the implicit assumption that it is UTC.

The data type uses proper ISO dates w/ the TZ suffix as the value.

The field item uses the data type as the value, and then has a computed value for the actual DrupalDateTime object.

DateTimeComputed brokers this, and all is well.

I don't think this would be a problem with the approach I am proposing. I am thinking that we can take the settings form changes from the existing patch and then adjust the logic in DateTimeWidgetBase::formElement()

  $element['value'] = array(
      '#type' => 'datetime',
      '#default_value' => NULL,
      '#date_increment' => 1,
      '#date_timezone' => drupal_get_user_timezone(),
      '#required' => $element['#required'],
    );

Instead of calling `drupal_get_user_timezone()` we would use the setting logic, and keep the non-widget code as-is.

?

OK, I will open a separate issue then. I still think #27 is a bug, but then I'll let you continue here without any further interruption. :-)

#30, I re-read your use case while looking core. It's a bug, and it looks like we are missing test coverage. It looks like all of the test are explicitly using the storage format string when setting values. That's on the public API, so that should work properly.

Re #31: Glad you agree. See you in #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken then ;-)

- when we process the entry, warp the date to UTC based on the setting for storage; this is basically what we do now, we just don't give the user the option on how it happens per instance

The issue with this is that converting from a specified timezone (eg, America/Los_Angeles) to UTC loses data since it would just be stored with -08:00. The if the user creates a date formatter where they want the actual selected timezone to display, they cannot do so reliably. I verified the D7 module stores the textual representation of the timezone.

Summary of IRC discussion.

- Keep value column in implicit UTC
- Add new widget setting for TZ entry handling
- Add new column for TZ name that was selected or computed by system behavior during input
- Keep logic that converts user entry to UTC for storage to allow simple string handling
- Add new field formatter option to TZ setting to allow for output using value at entry
- Possibly add some predefined date formatter strings that include TZ

Think that summarizes it?

I think further work on this should hold off until #2739290: UTC+12 is broken on date only fields is in.

I made a bit of progress. This adds some options to the formatter configuration for which timezone to use.

This adds support for date range fields as added by #2161337: Add a Date Range field type with support for end date, which looks close to completion. If we want to include support in this same patch, we should probably wait until that gets committed.

For the testbot.

A bot run of #40 doesn't make sense yet, because it references files that are only added by #2161337: Add a Date Range field type with support for end date.

Thanks @anemirovsky, but unfortunately since that issue isn't in yet, it will be nearly impossible to iterate with those changes included.

I'm working on some tests for this now, and hopefully #2161337: Add a Date Range field type with support for end date gets in soon so we can include support here (it could also be done in a follow-up since those are new field plugins).

This adds some additional testing and a few fixes. They are failing because there is still an issue where the timzone submitted by the user needs to be set before the \Drupal\Core\Datetime\Element\Datetime::processDatetime() method sets the timezone. As it is now, that gets called and sets the timezone to the site's timezone, which then gets converted to UTC on storage, and then when formatting, that gets converted to the submitted timezone, causing the date move around each time it is loaded into the form and saved.

Unassigning in case somebody else gets to this before I do again.

We somehow need to hook in before Datetime::valueCallback is called to set $element['#date_timezone'] with the user-selected timezone, since this is then used to construct the date object:

      $timezone = !empty($element['#date_timezone']) ? $element['#date_timezone'] : NULL;

As this patch stands now, that is set to NULL on new field values, and set to the previously selected timezone when editing.

I had an idea that will resolve the issue detailed above, and also simplify implementation: we can add a #expose_timezone to the datetime form element itself. This will also make the reroll cleaner when #2161337: Add a Date Range field type with support for end date lands.

Working on this now.

The new test is still failing, but I think this I've just overlooked something. I think this approach will work well, and it will have the added bonus of letting anybody using the core datetime form element to collect a timezone as needed.

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -78,6 +84,15 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+        // Store the timezone if set.
+        if ($this->getFieldSetting('timezone_handling') === DateTimeItem::TIMEZONE_DATE) {
+          $item['timezone'] = $date->getTimezone()->getName();
+        }
+        else {
+          $item['timezone'] = '';
+        }

This is why the date keeps processing. Since messageFormValues() is called twice (on form validate, and on form submit), the second time it is called, the timzeone on the date object has been set to UTC.

I didn't immediately see a way to set this elsewhere since the only value passed in is the datetime object.

Once this detail is sorted, I think the only thing left here is to write the relatively simple upgrade hook.

I'm optimistically tagging this as a beta target since if we could knock this and the date range issue out, that would allow contrib to make calendars, etc.

Few quick comments, but not an in-depth look yet.

1. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
   @@ -63,18 +89,25 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   -        'value' => array('value'),
   +        'value' => array('value', 'timezone'),
          ),
   

   Probably want to keep the 'value' index and add a 'value_timezone' index instead.

2. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
   @@ -95,6 +128,29 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
   +      '#title' => t('Timezone handling'),
   +      '#options' => array(
   +        static::TIMEZONE_SITE => t("Site's timezone"),
   +        static::TIMEZONE_DATE => t("Date's timezone"),
   +        static::TIMEZONE_USER => t("User's timezone"),
   +        static::TIMEZONE_NONE => t('No timezone conversion'),
   +      ),
   +      '#default_value' => $this->getSetting('timezone_handling'),
   

   Use $this->t() ?

3. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
   @@ -78,6 +84,15 @@ public function massageFormValues(array $values, array $form, FormStateInterface
   +
   +        // Store the timezone if set.
   +        if ($this->getFieldSetting('timezone_handling') === DateTimeItem::TIMEZONE_DATE) {
   +          $item['timezone'] = $date->getTimezone()->getName();
   +        }
   +        else {
   +          $item['timezone'] = '';
   +        }
   +
            // Adjust the date for storage.
   

   Maybe use $form_state->isValidationComplete() to check the phase to avoid the problem you mention in #52?

This doesn't look too hard to merge into #2161337: Add a Date Range field type with support for end date this if lands first, but if that does happen the end date issue will need an upgrade path. Maybe we can get these added at same time to avoid that?

Thanks @mpdonadio for the isValidationComplete() suggestion! This gets the new test passing, and addresses #53. I think the remaining fails are upgrade path related.

Only committers should add the beta target tag. In the case of this issue unfortunately it missed the deadline, and we generally avoid changes requiring upgrade paths in beta except for major and critical bugs. Thanks!

This update hook seems way too complex, but I couldn't find any other helpers that made it easier. If the update schema tests pass, I'll work on an final test for the upgrade path.

This will fix the undefined index tests.

This should fix that last failure. Will still need a dedicated upgrade path test, but this is close I think.

Adding some related issues that are blocking this. Once those are in, I think this can be next.

Since #2786583: Create a base class for DateTimeFieldTest and DateRangeFieldTest and move common code into it, this patch needs a re-roll.

The additional work that needs to be carried out is to support timezone for the datetime_range field.

I think the issue summary can use some clarification as to the purpose of this feature, because as someone who's not super familiar with how timezone handling has worked in D7 with the Date module, I'm a little lost.

Here's what I've gathered:

WITHOUT this feature, any date/times entered into a date field are assumed to be in the user's timezone, or if that's not set, the site's default timezone. The date/time is converted to UTC before being stored in the database. When displaying the date/time, it will be converted to either the user's timezone or the site's default timezone, depending on how the field formatter is configured.

WITH this feature, a timezone can be selected when entering a date/time into a date field. Drupal will assume that the date/time entered is for that selected timezone. The date/time is still converted to UTC before being stored in the database. The timezone is also stored in the database. When displaying the date/time, the formatter can be configured to convert the date/time to the specific timezone that was selected when it was entered, or just convert it to the user's timezone or site's default.

===

It seems like the "win" that this brings is that it allows users to enter date/times for worldwide events in that event locations native timezone. For instance, let's say someone wanted to enter an event taking place at 7:00PM in Chicago (-6). The editor's timezone in Drupal is New York (-5). Without this patch, the editor would have to enter the time as 8:00PM to account for that difference, whereas with this patch, the editor can enter 7:00PM and also select the America/Chicago timezone.

Is that all correct?

#70, yes, that pretty much sums up the user experience when this lands.

There was a missing default_value for the widget settings form. I think there will be test failures since there were a bunch from the last re-roll that weren't fixed.

Here's the interdiff from that last patch

Two problems found in manual testing:

1) The settings summary for the display formatter is not always accurate. If you select a timezone override, it will always display information relating to that override, even if you edit the settings and select a different display type.

2) Similarly, when the actual field data is rendered, the timezone override always takes precedence if you've previously configured one.

We could solve both problems by clearing the value set for "timezone_override" if the user de-selects timezone override from the display types, but probably a better fix it to adjust the logic that determines if the timezone override should be in effect so that it checks for the configured display type, instead of just checking if a value for override is set.

Another bug I found is that when a date field has a specific timezone filled in for it, and you configure the formatter to display it using the USER's timezone instead, it ignores that setting and still displays it with the date item's specific timezone.

I have a fix for that, and #74 above, but I'm going to wait until #2775669: Clean up redundant methods in datetime field formatters is committed since it all touches the same code.

Officially postponing to avoid re-reroll purgatory.

#2775669: Clean up redundant methods in datetime field formatters is in, so I think we can work on this again.

omit: sorry, wrong thread.

I'll work on rebasing #72.

This is a reroll of #72 (the rebase diff is attached as interdiff-2632040-72-82-rebase.txt). I also fixed the php notice errors that were causing the daterange tests to fail. It doesn't address any of the other issues noted above.

Oops, here's the rebase resolution diff noted above

Let me see if I can fix the issues I brought up before.

Here's the fixes to address #74.

I think the timezone logic in all the formatters can be refactored somehow to reduce this code duplication. I think as it stands, the timezone handling in the formatters is pretty confusing to understand. We have a method on DateTimeFormatterBase called "setTimeZone", but timezone overrides are not handled in this method, they are instead handled in each of the formatters formatDate method. I don't think those methods should need to deal with timezones at all.

May take a stab at that...

Also the name of the constant we use for specifying a timezone override is unfortunate. It makes code like this really confusing: if ($this->getSetting('timezone_display') === DateTimeItem::TIMEZONE_NONE && $timezone_override) {, but I don't think we can change the constant.

#85, I haven't looked at this patch in a while, but yeah, ideally this should have minimal impact on formatters. We should all try to review what we have and what we need to do now that we are finally unblocked.

Here's some refactoring based on reasoning I mentioned above.

Ignore #88.

This fixes my test failures.

Quick look; I need to read this applied an play with it for a proper review.

1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
   @@ -54,8 +55,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -    $timezone = $this->getSetting('timezone_override');
   -    return $this->dateFormatter->format($date->getTimestamp(), 'custom', $format, $timezone != '' ? $timezone : NULL);
   +    return $this->dateFormatter->format($date->getTimestamp(), 'custom', $format, $date->getTimezone()->getName());
   

   Hmmm. I think we need to rethink this logic. Even if someone enters a timezone upon input, they may need a different timezone on output?

2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
   @@ -32,8 +33,7 @@ public static function defaultSettings() {
   -    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
   -    return $this->dateFormatter->format($date->getTimestamp(), $format_type, '', $timezone != '' ? $timezone : NULL);
   +    return $this->dateFormatter->format($date->getTimestamp(), $format_type, '', $date->getTimezone()->getName());
   

   Ditto.

3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
   @@ -94,13 +95,38 @@ public static function defaultSettings() {
   +          DateTimeItem::TIMEZONE_USER => $this->t("The user's timezone"),
   +          DateTimeItem::TIMEZONE_NONE => $this->t("Timezone override"),
   

   Not in love with these labels, but don't have better suggestions. We also have four constants; shouldn't all four be options here?

4. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
   @@ -42,6 +43,28 @@ public static function defaultStorageSettings() {
   +   * Timezone uses the site's timezone, regardless of the user's timezone.
   +   */
   +  const TIMEZONE_SITE = 'site';
   +
   +  /**
   +   * Timezone uses the user's timezone.
   +   *
   +   * @see drupal_get_user_timezone()
   +   */
   +  const TIMEZONE_USER = 'user';
   +
   +  /**
   +   * Timezone is set per-date.
   +   */
   +  const TIMEZONE_DATE = 'date';
   +
   +  /**
   +   * No timezone conversion is performed.
   +   */
   +  const TIMEZONE_NONE = 'none';
   +
   +  /**
   

   If the DateTimeItemInterface patch gets in first, we probably want to move these there.

5. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
   @@ -63,18 +89,26 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +          'length' => 50,
   

   Can we double check this against the TZ database for max length?

RE: 1 and 2, the $date field there already has the proper timezone logic applied to it by the time that method is called. Your example of entering the time in a specific timezone and wanting it output in a different one works.

RE: 3, I think we can add some descriptions to both the widget settings and the field settings form that describes the impact of each choice a bit better. And yea, we probably should have all 4 constants there.

Rerolled, a lot of collisions with the short array syntax changes. I hope to address #92 tomorrow.

I'm deep into working on this (reworking formatter settings and tests), hold off on reviewing.

Thanks. Was #94 a straight reroll to account for the array issue, or was there anything else? When you wrap up, make sure you post an interdiff. This touches a lot of places, so seeing the incremental changes really helps.

#94 is a straight reroll PLUS applying the short array syntax to the new patch code. There's 1 unrelated CS issue I fixed by mistake, reverted here.

The latest patch attached is a somewhat rough work in progress.

Responding to @mpdonadio's quick review #92:
#92.1 & 92.2: @bkosborne is right, the patch already allows for what you want.
#92.3 I added 'Site' which was the missing option.
#92.5 Various SO source demonstrate that the longest current timezone name is about 31 characters, the longest ever has been 38, so the specified storage length of 50 looks safe.

ALSO FIXED IN THIS PATCH

Site's default timezone
Issue: Per #92.3 'Site's default timezone' should be offered as an option alongside user's timezone or specific timezone.

Null per-date timezone values
Issue: The formatter needs robust handling for cases where per-date timezones are enabled in the field, but null for a particular date value. This mainly happens to a field with existing values (e.g. in a migration or when updating a field to have per-date timezones), but could also be set that way via the API or a widget.

Previously the patch used the user's timezone if there was no per-date timezone value. But this creates unexpected and undesired results when transitioning from using a manually specified timezone override to using per-date timezones - all existing dates would switch from the previously specified timezone to user timezone, until such time as they were edited and a timezone set for each one.

Solution: The current patch version splits the formatter timezone settings into 3:
- 'Default timezone': a select offering site, user or specified
- a select of timezones, if specifed is selected as the default
- a checkbox for 'Use stored timezone preference'
If the checkbox is checked, then the stored per-date timezone is used unless it is null, in which case it falls back to the default selected.

Need thorough tests of formatter's timezone logic
I added helper methods and used these to systematically test all combinations of settings and circumstances

WHAT'S STILL TO DO:

We face 2 unavoidably entangled issues:

(A) Interpreting user-inputted times in the widget. Previously we always assumed the user's timezone, but that can be problematic.
(B) Storing a timezone preference for each date This is a natural outflow of A), because one you allow people to specify a timezone that should be used to interpret their input of a certain time, it is unhelpful if you don't save that timezone. And once you've stored it, you need to allow for it in the formatter's rendering of the time too.

We've done a good job of (B), storing per-date timezones. But we've not yet fully worked through (A), the basics of how to determine the timezone used to interpret user-entered times.

What we need to do next are:

1) Add to the form element a 'timezone label' (the assumed timezone for time input) to the content editor, that is displayed when the timezone chooser is not. Have this always shown, but make sure it can be easily overridden by themers. With all this flexibility around timezones, it gets dangerously easy to trip up editors.

2) Change the field's per-date storage setting into a boolean 'Allow times to have a preferred timezone'.

3) Offer a setting on the widget, same as on the formatter: 'Default timezone' with site, user, and manually specified as the 3 options.

4) If per-date storage is set in the field's settings, offer a setting on the widget 'Allow users to choose a timezone'.

5) Pass to the form element 2 arguments:
-- '#expose_timezone'. Sets whether the element should show a timezone selector or timezone label, based on widget setting 'Allow user to choose'.
-- '#date_timezone'. The default value of the timezone selector, or the value of the timezone label. The value of this is based on the stored timezone value if present, or the default configured in the widget if the per-date is null or not enabled.

6) Exhaustively test the combinations of settings and circumstances.

7) Sort the terminology: create consistency between the settings labels, settings descriptions, variable names and code comments.

Current patch is good for reviewing the overall approach so far, but not for reviewing the fine details.

I will work on the todo's over the weekend.

@jonathanshaw, thanks for moving this forward. We are making good progress on this.

I took a quick look at this. I am concerned about a few things right now.

We are starting to change a decent amount of strings (labels, titles, etc). If possible, we need to minimize this.

The user facing term and in comments needs to be "time zone", two words. That is the preferred language from the docs team.

My main concern (which I could be wrong about, this is a little hard to review as a patch) is that we are starting to couple the intended output intent (ie, formatter behaviour) upon input, the (A) and (B) above. IOW, we are implying what the formatters should do based on widget input. We need to avoid that.

I will review this applied and play with this over the weekend.

We are starting to change a decent amount of strings (labels, titles, etc). If possible, we need to minimize this.

Understood. I will think in terms of minimising this. I think it's only an issue in the formatters.

My bigger concern though is that it's very difficult to describe things clearly and set user expectations correctly, even with changing strings. I'm leaning towards coining the concept of a 'preferred time zone' or 'stored time zone preference' to refer to the time zone stored per-date in the field, but I need to play with this more, looking at all 3 settings contexts (field, widget and formatter) side by side.

My main concern (which I could be wrong about, this is a little hard to review as a patch) is that we are starting to couple the intended output intent (ie, formatter behaviour) upon input, the (A) and (B) above. IOW, we are implying what the formatters should do based on widget input. We need to avoid that.

Recent iterations of the patch make that coupling an optional (and non-default) behavior. If a timezone is set in the widget, you can choose in the formatter whether to use it or ignore it.

I think we should make addressing #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented a prerequisite for this issue.

I'm close to having a heavily-revised patch ready, hold off on reviews on this issue for now.

#102, do you still think this is too coupled to 2799987? Should we postpone this properly until we figure that out? I haven't had much time to look into either of these issues this week.

Re #103, it's better to do #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented for 2 reasons:
1) to have a good foundation before we add in more complexity
and
2) because of problem 2 in that issue, what I want to do in the widget here doesn't work: you can't get an exposed timezone selector to default (for new values) to something other the user timezone.

However, I don't suggest we need to formally postpone. This patch touches a lot of places, the element is just one small part of it, so there's a lot of reviewing and polishing that can be done here anyway even while that one is still being nailed down.

I have done a bunch more on this, and will upload new patch tomorrow once I've unpicked the merge conflicts with #2780063: Convert web tests to browser tests for datetime and datetime_range modules.

What I've done in this latest patch

A. Added widget settings for timezones, almost identical to those for the formatter and sharing a trait.

B. Reduce the field type setting down to a boolean (store / don't store), as the fancier question of what default you want to assume etc now lives on the widget/formatter.

C. Added widget timezone tests, and tidied up the formatter tests.

D. Mostly got all this timezone handling working for date range fields. The hard part of this was that the exposed timezone selector should only show up once (not on both date elements) but its value needs to be used to interpret the contents of both elements.

E. Harmonised the terminology of the UI text, code comments, array keys, constants and variables.

F. Moved the constants into an interface.

What still needs doing

1. Fix the date range formatters.

2. Extend the current tests to cover the daterange field/formatter/widget.

3. Extend the current tests to cover all widgets and formatters extending the DatetimeBase widget/formatter.

4. Check whether the logic in ConfigurableTimezoneTrait::getDefaultTimezone() is adequate for DateRange fields of the All day type.

5. Check whether we have adequate tests for all day and date-only fields, so that we know if our timezone changes are accidentally impacting them.

6. Add an update path for the formatter's timezone_override setting.

7. Remove -None- as an exposed value in the timezone_override selector (now called "fixed time zone").

8. Default timezone_override to the site's default timezone if there is the stored config setting is ''.

9. Add tests for all update paths.

10. Finish the datetime element once #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented gets fixed.

11. Consider renaming the constant TIMEZONE_NONE to TIMEZONE_FIXED; consider changing its value to 'fixed' not 'none'. This may require an upgrade path if 'none' gets stored anywhere in config, but I don't think it does.

12. Bug: the field setting form allows you to change the timezone storage setting even after values are in the database, but it won;t save your change of setting. Make a test for this.

13. Make sure the date range validation that ensures the start time is before the end time is timezone intelligent - in particular it needs to handle the case where the exposed timezone on the start time is changed and that change needs to be applied to both start and end. Make sure we have tests for this.

14. Widgets and formatters seem to be forgetting their previous settings when disabled and then re-enabled. I'm not sure if this is the standard behavior or a bug in this patch.

15. Bug?: Make sure all the widgets and formatters are available in the UI when they should be. I've noticed an oddity here.

16. Bug: The formatter/widget timezone settings summary are not properly translatable, I think they don't use $this->t() correctly.

17. Bug: The formatter/widget timezone settings summaries are not behaving properly. The per-date settings summary doesn't show up at all for date_range fields, and only refreshes for normal date fields when the "Save" button is pressed, not when the "Update" button is pressed.

18. The field's timezone column description needs updating to reflect the new terminology, for the normal field and the daterange field.

19. The formatter/widget settings summaries may need updating to reflect new terminology.

20. Add tests for the formatter & widgets settings forms and summaries, and apply them to all widgets and formatters for both date and daterange fields.

Most of all, it's tests, tests, tests, tests. Timezones are really hard, and there are a lot of logic paths we're tampering with here all over the place. Unfortunately, I can't undertake to fix all this. I will focus on #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented.

@jonathanshaw, thanks for your work with this, the TZ stuff is a pain. I think we need to see your in progress patch here to assess things (even if it doesn't apply b/c the BTB issue). It looks like you uncovered some new bugs, and identified some new ones. It also looks you may have done some needed work (eg, the interface thing) that we already have an issue for.

I think we need to look new stuff you uncovered, along with the queue, to see what we need to tackle first to make this more manageable.

The biggest thing would probably be refactoring the functional field tests to make them manageable. I also think we need to be sure we aren't making things overly complex; I don't want to get rid of any work that would benefit end users; we need to provide the essential building blocks in core (I think #70 is our MVP), and extra functionality can be punted to datetime_extras. I am still not sure what 'timezone_per_date' is providing us here, especially since exposes more test surface area.

Here's the rebased patch, and a further reroll of #94.

I think we need to look new stuff you uncovered, along with the queue, to see what we need to tackle first to make this more manageable.
The biggest thing would probably be refactoring the functional field tests to make them manageable

If you reckon this patch has become too sprawling too handle, I suggest the way to split it up would be into different issues like this:

1) Create a new TimezoneFieldTest class, which contains test methods that can be applied to any widget/formatter datetime/date_range to test timezone handling. So if we add some new timezone logic, we only have to touch a couple of places in the test and it magically gets tested on all widgets/formatters.

2) Refactor the formatters as suggested by @bkosborne in #88.

3) Move the formatters timezone settings into a trait, with the ['timezone_default] options of user, site and fixed; fixed allows you to choose (what we call for BC reasons) the ['timezone_override'].

4) Apply the trait to the widgets too. All the same options make sense for them. This addressed WidgetCo's first problem from the IS:

A. When creating session entities as part of their annual conference in New York, the times of the sessions as input by the editor are interpreted by Drupal in terms of the time zone of the editor who is creating or updating it, not New York. In this case their sitebuilder needs to be able to control the time zone used by the widget to interpret the times the editors input.

5) Add per-date time zone storage, to solve WidgetCo's second problem:

B. When creating meeting entities to describe meetings held at various locations across the world, the same problem happens. But in this case the intended time zone needs to specified for each meeting by the editor, and that preference needs to be stored with the date/time.

Trying to do 5) before 4) gives us earlier iterations of this patch which have things that should be formatter / widget settings being put as field settings, a mess that would be hard to unpick or build on. Trying to do 4) before 1) and 2) and 3) is painful and error-prone.

This would be 4 new issues and a meta to keep track of it all.

I am still not sure what 'timezone_per_date' is providing us here, especially since exposes more test surface area.

I don't think it's a sure thing either. It would be OK for core to default to always using the per-date if one existed. Especially if we put on the trait

protected function shouldUsePerDateTimezone() {
  return TRUE;
}

as this would make it trivial to change this behavior.

However, there area few reasons I think it may be worth having the setting:

a) It will always be desirable to have the formatter setting to choose the default time zone, even if per-date is enabled in the field, because we have to handle fields with existing values that will have a NULL stored time zone. And having this setting, which will then be ignored most of the time, is slightly misleading for sitebuilders. It would make it more bluntly obvious how things worked if you have the additional checkbox for "If a preferred time zone is stored, use it instead of the default". This drives home the idea there are two different things at work here, a default and something that can override it per-date. The screenshots say it better than I can.

b) I suspect it's quite common to want to sometimes render a time in its canonical (venue) time zone and sometimes in the time zone of the user.

c) I imagine it might not be uncommon to want to hide the time zone selector on a "basic" form but have it exposed on an "advanced" form for superusers.

The reroll, interdiff and test-only in #105 are correct, but the main patch 2632040-105.patch only has about half of the work. Here is the full patch. Sorry about this.

#110 had a print_r statement that was causing AJAX errors when trying to add entities using a complex inline entity form. Here it is with it removed.

#112 is 10% smaller than #110, isn't that suspicious? Maybe a file is missing.

I ran a diff on the 108 and 111 patches. The 111 patch is missing the updates to the files datetime.install, ConfigurableTimezoneInterface.php, and ConfigurableTimezoneTrait.php

Yeah, that was my fault -- I didn't create the patch correctly. Here it is fixed along with an interdiff against #108.

I'm not sure whether the screenshot below (stolen from the issue summary) is still representative of this issue's direction, but in case this hasn't already been considered, I wanted to throw out the possibility that the date range field should provide the ability for the start date and end date to have different timezones from one another (e.g. a travel site needs to store the departure and arrival times of a flight that crosses timezones).

Re #116 and the possibility of different timezones in date ranges I'm mindful of @mpdonadio's frequent injunction that core should provide the basics, not cover every use case.

The truth is that the 2nd timezone selector is naturally present in the widget and I actually had to do some ugly work to suppress it. So supporting it in the widget is easy.

But there's nothing at the moment to support it in the storage and formatter. Worst of all, because most sites would want to use the same timezone for both, we'd have to have setting and be constantly checking it and doing conditional logic based on it. The number of combinations of settings and formatters etc. is already painful, adding another permutation is not a very happy prospect.

On balance this seems like something that should be in datetime_extras, not core. But by all means review the patch and indicate if there is something that makes it unnecessarily hard to do this in contrib, if there's something that would make extending easier.

RE@116. different end timezone.

That is an interesting use case. but could covered by simply using 2 separate combo date fields and a custom formatter..or even a secondary field capturing the expected length of flight..

Certainly an idea that would deserve it's own ticket.

I completely understand the points raised in #119 and #120. I can see how the permutations could be getting a bit nuts.

While this use case could be solved per #120, it does seem cleaner to solve this in core. It is logical to me to defer the additional UI complication to a later date (or contrib) even if it's easy to expose the widget, but would it not make sense to address the storage aspect here to provide contrib with a simpler path to full implementation?

I'm not invested in this use case; I just thought it would be appropriate to address here.

Having a field column sitting there that we never used would be very confusing to anyone looking at the code. If it's in the storage we need to use it.
Let's see what @mpdonadio thinks.

Nice progress here! A few items I found in reviewing the latest patch:

1. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -410,7 +410,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +   * Configures the newly created field for the default view and form modes.
   
   @@ -428,7 +428,7 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
   +   * Configures the newly created field for the default view and form modes.
   

   These changes look unrelated?

2. +++ b/core/modules/datetime/src/Plugin/Field/ConfigurableTimezoneTrait.php
   @@ -0,0 +1,133 @@
   +  ¶
   ...
   +    ¶
   

   Whitespace errors.

3. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
   @@ -156,7 +157,7 @@ public function testEntityDisplaySettings() {
   +    $defaults = ['format_type' => 'fallback', 'timezone_override' => '', 'timezone_default' => DateTimeFormatterBase::TIMEZONE_USER];
   

   While technically ok, this constant should probably reference the ConfigurableTimezoneInterface rather than the base class...

@jhedstrom, what are your thoughts on separate TZ for start/end for daterange? I'm torn on this. I think it is an edge case, but don't want to design ourselves into a corner and end up with a nasty update path (though this wouldn't probably be that hard). My gut says that we should push that feature to contrib.

what are your thoughts on separate TZ for start/end for daterange?

Sine we already have the widget forced to a single TZ selection, even though that isn't the default widget behavior, I think we can push the separate TZ per start/end to contrib and/or a follow-up. There would be additional formatter, widget options etc that we don't need to complicate this already complex issue further with I think. Folks making a site for multi-timezone races will have to wait ;)

@mpdonadio / @jhedstrom I'm waiting for your take on #107, whether you want me to split this up or to proceed in just this issue?

I am not sure if we are going down the right path with this right now.

I am still not convinced about the timezone_storage setting on storage. I am leaning toward always storing the TZ; I have a feeling that the empty column is going to cause problems with REST and API usage, and possibly Views (not 100% sure, but empty values make me uneasy).

For the widgets, I think we can have an option to expose a TZ select, that defaults to drupal_get_user_timezone(). If the option is false, the just store drupal_get_user_timezone() with the field.

And then there would really not need to be any change to the formatters.

I think the current version couples storage / input / output too closely. I think the approach outlined in #34, which I think was was was the blueprint up to #94 or so is still the best bet.

@jhedstrom, thoughts?

I am leaning toward always storing the TZ; I have a feeling that the empty column is going to cause problems with REST and API usage, and possibly Views (not 100% sure, but empty values make me uneasy).

How would you handle existing fields? What value would you have the update path insert into the timezone column in these cases? If you don't want to have empty values presumably you'll have to force it to be the system's default timezone, but have the formatters default to not using the stored timezone?

Even if we don't allow this, perhaps there is still merit in having the timezone default (user/site/fixed) be a setting of the widgets, rather than a setting of the field and so forced to be the same in all widgets.

I am still in favor of the approach outlined in #34.

Existing fields would get the site's timezone set I think, and since the prior to this, formatters only displayed in the site, or in the current user's timezone, this would be ignored by the formatters.

Always storing the timezone

I'm understanding you guys to want the timezone column never empty. This requires AFAICS:
1) In the update hook, create a timezone column for all datetime fields,
2) In the update hook, put the site's default timezone as the value in all existing rows
3) Always store a timezone, including when saving via the API, regardless of the field's settings.
4) The FieldType needs to implement the getDefaultValue() method, so as to provide drupal_get_user_timezone() as the default value for timezone column when saving via the API.

This all seems fine to me. It requires some additions to the patch because so far no one has made the API or update path store a timezone.

The settings

#34 is not explicit about the details of what settings there are on the field, widgets and formatters.
Given you've specified the timezone column will never be empty, we now have 2 possible approaches:

The minimal option:
FieldType: have a checkbox for "Use per-date timezones"
Widget: If "Use per-date timezones" is checked on the FieldType, always have timezone selectors exposed by the widget, and always have these default to drupal_get_user_timezone()
Formatter: If "Use per-date timezones" is checked on the FieldType, always display dates using the stored timezone, do not allow overriding (hide the overrides setting).

More flexible options, which can be adopted in various combinations:

FieldType: Allow the user to choose the default timezone from "Site", "User", or "Fixed".
Widget: Have a select setting called "Timezone to use" with values "User's timezone", "Site default timezone", "Selected by user", and "Fixed". Timezone selectors are only exposed to the editor if the "Selected by User" option is shown. The "Fixed" option allows the sitebuilder to specify a particular timezone.
Formatter: Have a select setting called "Timezone to use" with values "User's timezone", "Site default timezone", "Stored timezone", and "Fixed".
If you have settings like this for the widgets and formatters, then you don't need a "Use per-date timezone" checkbox setting on the FieldType.

I think the idea of a default setting on the FieldType is worth considering seriously (and present in #94), because this affects the API behavior and can't easily be left to datetime_extras.

Overall when it comes to the widgets and formatters we have a choice between:
A) having a checkbox setting "Use per date timezones" on the FieldType, a (sometimes bypassed) "Timezone override" setting in the formatter, and nothing on the widget; OR
B) not having the checkbox setting on the FieldType, on the formatter replacing Timezone override with a broader selection of choices ("User's timezone", "Site default timezone", "Stored timezone", and "Fixed"), and reusing the same logic on widgets.

My sense is that B gives more power to sitebuilders while being not much more complex than A (possibly cleaner, depending on your tastes). But I can see there is a case for keeping core simple and leaving the formatter and widget settings to some more flexible widgets/formatters in datetime_extras.

Re-rolled against 8.4.x head.

The 2632040-131.patch doesn't apply on core 8.4.2.

Looks like the patch is failing in core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php, Here is the rejected file I get from composer:

$ cat core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php.rej
--- modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
+++ modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
@@ -13,12 +14,15 @@
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\datetime\Plugin\Field\FieldType\DateTimeItem;
 use Symfony\Component\DependencyInjection\ContainerInterface;
-
+use Drupal\datetime\Plugin\Field\ConfigurableTimezoneTrait;
+use Drupal\datetime\Plugin\Field\ConfigurableTimezoneInterface;
 
 /**
  * Base class for 'DateTime Field formatter' plugin implementations.
  */
-abstract class DateTimeFormatterBase extends FormatterBase implements ContainerFactoryPluginInterface {
+abstract class DateTimeFormatterBase extends FormatterBase implements ConfigurableTimezoneInterface, ContainerFactoryPluginInterface {
+
+  use ConfigurableTimezoneTrait;
 
   /**
    * The date formatter service.

Re-rolled against 8.4.x head for people using this in production already and fixed an undefined index error.

Not sure if this will apply, but since this is a Feature Request, development should be against 8.5.x.

Looks like we need a reroll for 8.5.x.

Re-rolled for 8.5.x.
Corrected some coding standard errors.

Corrected a few test failures.
Removed a few more coding standards errors.