Problem/Motivation

I noticed that while my local time-zone is Europe/Amsterdam
Drupal auto-selects the Europe/Paris time-zone.

While technically correct I started digging into why it would not use the Systems time-zone.
I discovered that there is a ECMAScript® Internationalization API that would allow a better timezone selection based on the system or browser reported time-zone.

Proposed resolution

Implement a patch to the core/misc/timezone.es6.js library to first try and auto-detect the time-zone as it should be reported by Intl.DateTimeFormat().resolvedOptions().timeZone;, if unsuccessful or a failure fall back on the already written detection algorithm.

This leads to more accurate results and less processing to find out the users Time-zone.

Remaining tasks

Review is needed of this solution as provided in the patch.

User interface changes

No UI changes, the UX is improved by faster Javascript execution when possible.

API changes

No changes.

Data model changes

The Data model is unaffected.

Release notes snippet

Javascript Timezone default detection improved with ECMAScript Internationalization API implementation

Original report by sysosmaster

CommentFileSizeAuthor
#22 core-js-timezonedetect-3016427-22.patch1.39 KBnod_
#20 default_timezone-3016427-20.patch8.38 KBkunalgautam
#18 3016427-18.patch8.4 KBneslee canil pinto
#2 Default_timezone_selection_incorrect_3016427_1.patch8.4 KBsysosmaster

Comments

sysosmaster created an issue. See original summary.

sysosmaster’s picture

sysosmaster commented
StatusFileSize
new Default_timezone_selection_incorrect_3016427_1.patch8.4 KB

Attached path file.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.6.x-dev » 8.7.x-dev

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Issue tags: +DrupalCampBelarus2019, +Novice

Tagging it for DrupalCamp Belarus - it might need a reroll, but mainly review and test.
https://twitter.com/LKvBuel/status/1127858724205408257

dmitriy.sukhodolsky’s picture

dmitriy.sukhodolsky commented

I tested and it works as expected:
* Drupal shows unrelated timezone during clear installation.
* Drupal shows timezone which was set in my OS settings after I applied the patch.
I did not face any issues or error messages during testing.

sysosmaster’s picture

sysosmaster commented
Status: Needs review » Reviewed & tested by the community

Since @dmitriysukhodolsky reviewed this I will now consider this RTBC and mark it as such.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

@sysosmaster thanks for the fix. This code does look better than HEAD and in fact looking at https://caniuse.com/#search=Date vs https://caniuse.com/#search=Intl you can see your proposal has more support so that's great.

However, by convention we don't rtbc our own patches after a single review that's basically "it works". There are other considerations that need to made - for example does this have test coverage and can we provide better automated test coverage with Drupal 8's Javascript tests.

sysosmaster’s picture

sysosmaster commented

I see, I agree that I jumped ahead a bit with the RTBC. (My main goal was to get this moving, not to jump the queue)

as for testing. I fail to see how you would want to test something as fundamental as detection of timezone in a JavaScript test suite.
I would need a full system test to do see since it involves changing the timezone settings on the machine itself. (which is I believe out of scope for a JavaScript test framework)

Any test that I can think of within a test framework would basically either duplicate this code. Or must be running on a known per-defined timezone. (and could only test that Timezone). If that timezone is one where it does not show a difference between the old and new techniques (like testing this in Germany which would resolve in both cases to Berlin/CE(s)T)

For most other JavaScript I agree it needs to have test. but we have to ask ourselves at what point are we still testing the JavaScript, and at what point are we testing the JavaScript implementation (a.k.a. the JavaScript engine)

This patch also does not remove the old style of detection either. it only utilizes the Browsers knowledge about the environment it is in through the internationalization API.

TL;DR
-because of the small nature and IMHO untestability of this change after the first rreview Iset this to RTBC. (this was incorrect of me I will not do this again)
- This javascript uses fundamental API's of browsers. testing them IMHO is redundent. except if we can controle the Timezone what the test machine is in.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
  1. What's great about this change is that it means one less request to the server during an install. That's nice.
  2. +++ b/core/misc/timezone.es6.js
@@ -15,59 +15,68 @@
-        // which can be interpreted by PHP.

+++ b/core/misc/timezone.js
@@ -10,41 +10,46 @@
+        if (autoDetectedTimezone === undefined || autoDetectedTimezone.length < 2) {

    When is autoDetectedTimezone.length smaller than 2? 

  3. +++ b/core/misc/timezone.js
@@ -10,41 +10,46 @@
+          $timezone.val(autoDetectedTimezone);

    Are we 100% sure that the ECMAScript Internationalization API will definitely return a value that's supported by PHP? Timezones are a bit complicated. For example, timezones get deprecated see https://www.php.net/manual/en/timezones.others.php

    So there's a risk that your browser and PHP are out of sync. Imo we need to ensure that the value is in the select list and if it is not then we need to fallback to the old behaviour.

  4. As for testing we're not testing, you're right we don't need to test Intl.DateTimeFormat().resolvedOptions().timeZone per se but we should test that the logic in Drupal.behaviors.setTimezone() works as expected. And if we don't have an existing automated test then this change runs the risk of breaking it without us knowing.
  5. Noticed something whilst reviewing this issue. We should file an issue to change the JS to not use the class timezone-detect but js-timezone-detect as this class exists purely the javascript to find the select element. (Note this change is out-scope for this issue).
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

We need to change code for #10.3 so setting appropriate status.

sysosmaster’s picture

sysosmaster commented

When is autoDetectedTimezone.length smaller than 2?

Broken Timezone Detection (think really old Javascript Engineds / Some NodeJS engines)
Its basically to catch the defunct timezone awnser of '1' or alike.

Are we 100% sure that the ECMAScript Internationalization API will definitely return a value that's supported by PHP? Timezones are a bit complicated. For example, timezones get deprecated see https://www.php.net/manual/en/timezones.others.php

Anser: Yes, although we have to assume that both Unicode and PHP will keep there defenitions uptodate with ther eupstream source of IANA. (I say we can assume this based on importance and past compliance efferts by both unicode and PHP.) :

The options property timeZone in the DateTimeFormat constructor, provided that the additional acceptable input values are case-insensitive matches of Zone or Link identifiers in the IANA time zone database and are canonicalized to Zone identifiers in the casing used in the database for the timeZone property of the object returned by DateTimeFormat.resolvedOptions, except that "Etc/GMT" shall be canonicalized to "UTC".

Source:ECMAScript Internationalization API Specification

To be clear:

  • IANA Implements RFC6557 for constructing there Database.
  • Javascript uses the IANA definitions
  • PHP uses the Unicode definitons that include the IANA defenitions Source

(I did a API / Defenition / Code / Specification lookup and check to validate this is all true. feel free to double check it)

So there's a risk that your browser and PHP are out of sync. Imo we need to ensure that the value is in the select list and if it is not then we need to fallback to the old behaviour.

Yes there is...
however this requires the Operating system to be out of sync with the Timezone database
(if you use NTP you are required to keep this up to date).
Most OS's have an update mechanism for this.
If this Database is out-of-date the system will also have edge case errors with things like Certificate validation. (So I consider those systems edge case and out of scope for consideration in this issue)

I'll investigate if I can write a test for this, If I can I will add it asap.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@sysosmaster note is we only set a value that's in the list of allowed values already we ensure we're only setting to a value supported by the PHP on the server so if we change the patch around like i suggest then that is covered. We can not assume that browser manufactures and PHP and especially the version PHP on the server all move in sync.

sysosmaster’s picture

sysosmaster commented

How old version. Do you want to support. Your talking about browsers and php versions that are more than 5 years out of date.

TimeZones are changed / added / removed but that’s a slow process and often the timezonedb all ready has them long before there I acted.

We could add some detection and instructions for updating the php timezone definitions as shown here l timezone-version-get

As for browsers being out of date. That would result in an older name Timezone name being send and php will recognize it. (It’s list is bigger than IANA’s and include most of the obsolete definitions only archaic ones are not always there as listed on the php timezones page.

I’ll work on a test function for dB out of date check.

bannss1’s picture

bannss1 commented

This issue is still persistent in the user register form after applying the patch.

Drupal 8.7.3

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

handkerchief’s picture

handkerchief
Primary language German
Location Chur, Switzerland
commented

Any news on this?

neslee canil pinto’s picture

neslee canil pinto
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 3016427-18.patch8.4 KB

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kunalgautam’s picture

kunalgautam commented
StatusFileSize
new default_timezone-3016427-20.patch8.38 KB

This is the patch for Drupal 9.1.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work 
  1. +++ b/core/misc/timezone.es6.js
@@ -15,59 +15,66 @@
+        const autoDetectedTimezone = Intl.DateTimeFormat().resolvedOptions()
+          .timeZone;

    resolvedOptions is a instance method, not a static one. const autoDetectedTimeZone = (new Intl.DateTimeFormat()).resolvedOptions.timeZone; 

  2. +++ b/core/misc/timezone.es6.js
@@ -15,59 +15,66 @@
+        if (autoDetectedTimezone === undefined ||
+          autoDetectedTimezone.length < 2) {
...
+        } else {
+          $timezone.val(autoDetectedTimezone);
+        }

    I'd reverse the if, to avoid splitting the new code all over the place, and probably return early so that we don't have to touch the indentation of existing code.

To be extra sure, we could always check if the value autodetected with JS exists in the select before setting it, and fallback to the old way of doing things.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-timezonedetect-3016427-22.patch1.39 KB

Patch is more readable than interdiff so leaving it out.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Sorry was a bit too fast, didn't see the novice tag, should have waited a bit more before sending a patch.

nod_ credited quicksketch.

nod_’s picture

nod_
Primary language French
Location Lille
commented

credit from duplicate issue

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Status: Needs review » Reviewed & tested by the community 
> new Intl.DateTimeFormat().resolvedOptions().timeZone;
"Europe/London"

Cool! 👍

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 9.0.x-dev
Category: Task » Bug report

Tested locally. This works and save a request to the server during interactive install and also account creation - plus it's more accurate. Given that the effect is the same I don't think this change needs a CR.

Also while the timezone select in the summary is valid it is not correct to reclassifying this as a bug and backporting.

Committed and pushed 13d0584f61 to 9.1.x and 98bbe51036 to 9.0.x. Thanks!

  • alexpott committed 13d0584 on 9.1.x 
    Issue #3016427 by sysosmaster, nod_, kkalashnikov, Neslee Canil Pinto,...

  • alexpott committed 98bbe51 on 9.0.x 
    Issue #3016427 by sysosmaster, nod_, kkalashnikov, Neslee Canil Pinto,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.