Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configurable implicit tz and expose to formatting, dev/core #2122 #18841

Closed
wants to merge 1 commit into from

Conversation

adixon
Copy link
Contributor

@adixon adixon commented Oct 22, 2020

Overview

Provide a configurable implicit timezone that can be used in date formatting configuration.

Before

Event dates (and others) were stored and displayed without a timezone.

After

Site administrators can configure and display an implicit timezone for event date/times.

Comments

Could be improved by creating/using a pseudo constant for allowable timezones (i.e. the ones here https://en.wikipedia.org/wiki/List_of_tz_database_time_zones in the tz database name, which are also available from
php as https://www.php.net/manual/en/timezones.php

This code will convert that region/location timezone into the current more readable format "%T", i.e. something like 'EDT'.

This PR is intended to provide a simple improvement pending a more complex leap that could involve adding timezones to the date times themselves, or potentially displaying timezones in the timezone of the visitor.

It could also be leveraged for other improvements in the handling of dates (e.g. the handling of logging date-times).

Original issue inspiration here: https://lab.civicrm.org/dev/core/-/issues/2122

@civibot
Copy link

civibot bot commented Oct 22, 2020

(Standard links)

@civibot civibot bot added the master label Oct 22, 2020
@adixon adixon changed the title Expose tz 2122 Allow configurable implicit tz and expose to formatting, dev/core #2122 Oct 22, 2020
@adixon
Copy link
Contributor Author

adixon commented Oct 22, 2020

Here's the issue: https://lab.civicrm.org/dev/core/-/issues/2122

@mlutfy
Copy link
Member

mlutfy commented Oct 22, 2020

It's an interesting short-term solution. Just double-checking:

  • By default, the Etc/GMT TZ will be used, instead of the current TZ already set in PHP by the CMS?
  • On the other hand, the default TZ gets applied only if the %T token is used, so the previous point maybe isn't much of a concern.
  • but this might cause issues on Drupal, for example, which allows setting a per-user timezone?

Maybe we don't need to set the TZ?

I'm thinking of an extension that does a browser geolocation check on Event pages in order to set the TZ to display time in the locale of the user, and having the visual feedback of %T would make that easier.

@adixon
Copy link
Contributor Author

adixon commented Oct 22, 2020

Yeah, I used Etc/GMT as lame default because I was lazy - pulling in the default from the current tz might be smarter. Although for some installations, that current tz might also be some system default that isn't what's wanted either, so at least Etc/GMT is a reasonable fallback in the absence of any information and is obviously a default.

And yes, the default won't be displayed anyway by default. In fact, calling it a 'default' is kind of confusing, calling it the 'implicit' tz is a more accurate description of what it is.

The key point is that some dates are stored without a timezone, and this setting allows us to add an implicit one, which is sometimes what is wanted.

The issue of user-configured timezones: that's a supplementary opportunity to convert the timezone of the stored date/time into a more user-centric view of that field.

@MegaphoneJon
Copy link
Contributor

I like the general idea, but I'm concerned about creating a new "default timezone" setting that's separate from the default timezone of the CMS. Could we use CRM_Utils_System_Base::getTimeZoneString() to populate the template instead?

Besides the duplication, I'm concerned about what happens when someone has reason to believe they've set the timezone on the event correctly, but the Scheduled Reminders to go out 2 hours before the event go out at the wrong time because Scheduled Reminders is relying on CRM_Utils_System_Base::getTimeZoneString().

@adixon
Copy link
Contributor Author

adixon commented Oct 23, 2020

I looked at that function but I don't think it will work - it's pulling "date_default_timezone_get" by default which will vary by user (at least in Drupal 8, by my testing). I think it probably works for cron jobs, but not for authenticated sessions.

We could use that as a default on the form, but the point is to store something as a setting that is shared/common for all, not something that depends on who's looking (which is not to say we might not want to change the timezone display depending on who's looking, but the implicit timezone of date values without a timezone should not depend on who's looking!).

@totten
Copy link
Member

totten commented Nov 11, 2020

It seems like the word "default" is ambiguous in this context. Apologies that this comment dives into semantics... but... "default" suggests a deference to some general authority, and it suggests there is one value from the general authority.

The problem here is that we actually have two competing authorities that we can plausibly defer to -- i.e. the user (what timezone is the user in; this typically tracks to the PHP-session/MySQL-session/CMS-session) and the organization (what timezone is socially designated by the org for most business).

@adixon
Copy link
Contributor Author

adixon commented Nov 12, 2020

Agree that "default" is a confusing term here. I prefer to use the word 'implicit' to describe the setting I've introduced, since all it's doing is providing an implicit timezone to date/times that don't have one. It doesn't provide a default when creating new date/time values.

My use of the word default above was in the narrow context of an administrator setting their implicit timezone - i.e. what that value should be when it hasn't yet been configured, and whether that default should be pulled form one of those existing competing authorities, or whether it should provide a value that is clearly unset. My conclusion was 'use the lame default' to encourage the administrator to not accept it blindly. Of course, it could also be an empty string, which is what the system will / should use if it's not set at all, but putting in the lame default at least prompts the administrator to understand what kind of string they are supposed to use.

FWIW, this is not an original or easy problem. In Drupal 7 if you don't set the 'default' timezone for the system, everyone gets somewhere in Africa as their timezone (presumably it's picking the first entry alphabetically at +00 offset from GMT). Which has a kind of anti-colonial justice feel to it.

@seamuslee001
Copy link
Contributor

Just noting on this ticket as well but even tho we are using date_default_timezone_get for Drupal 8 / 9, it will respect the timezone set on the user record as per this Drupal ticket https://www.drupal.org/project/drupal/issues/3009377, (tl/dr they added a Symphony listener that modifies the output)

@jaapjansma
Copy link
Contributor

@adixon or @seamuslee001 what is the status of this PR? Does this require additional work or is it waiting for a review? It is unclear to us (@BettyDolfing and @kainuk and I).

@adixon
Copy link
Contributor Author

adixon commented Dec 1, 2021

I've been using this on my production sites, but I believe the consensus is moving to a more complex/comprehensive solution (linked above). So this is really just a POC + discussion.

https://lab.civicrm.org/dev/core/-/issues/2122

@mlutfy
Copy link
Member

mlutfy commented Dec 2, 2021

Closing, since as Alan says there is more consensus towards #20476 and we can always re-open this PR if necessary.

@mlutfy mlutfy closed this Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants