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

Fix Date options (notably credit card exp month) for users in Whitehorse Yukon #25066

Closed
wants to merge 1 commit into from

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Nov 28, 2022

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

Overview

Found an interesting bug that only happens with users who have chose the America/Whitehorse timezone (-0700). It does not happen if we chose another similar timezone (America/Vancouver, America/Yellowknife, etc).

https://lab.civicrm.org/dev/core/uploads/ea61f948003f258501391a3c90e2a6e3/image.png

To reproduce on dmaster / drupal7:

  • login as the demo user
  • change the user's timezone to America/Whitehorse

Then go to a contribution page:
https://dmaster.demo.civicrm.org/civicrm/contribute/transact?reset=1&id=2

Before

Months are incorrectly "translated" (due to the timezone offset)

After

Fixed

Technical Details

Date formatting does not need to take timezones into account, so using UTC seems like the safest option.

@civibot
Copy link

civibot bot commented Nov 28, 2022

(Standard links)

@civibot civibot bot added the master label Nov 28, 2022
@demeritcowboy
Copy link
Contributor

I'm not sure about this. We should check what it looks like for somebody far east because there'll be a mismatch between strtotime and UTC. I wonder what's different about Whitehorse.

If this doesn't work out then perhaps there's a solution using UTC and gmmktime() instead of strtotime().

@demeritcowboy
Copy link
Contributor

If I set drupal timezone to Australia/Sydney, then it looks like this, where Dec has value 1.

Untitled3

@mlutfy mlutfy added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Nov 28, 2022
@mlutfy
Copy link
Member Author

mlutfy commented Nov 28, 2022

hmm oh yikes.. this is weird.

Just poking around, even passing the local timezone does not work, or using DatTime with timezone. Haven't tested gmtime yet:

<?php

$test = [
  'America/Whitehorse', // UTC-7
  'America/Vancouver', // UTC-8
  'Australia/Sydney', // UTC+11
];

foreach ($test as $tz) {
  date_default_timezone_set($tz);

  $date = new DateTime('1 January', new DateTimeZone($tz));
  $intl_formatter = IntlDateFormatter::create('en_US', IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, NULL, IntlDateFormatter::GREGORIAN, 'MMM');
  echo $tz . ' with NULL strtotime: ' . $intl_formatter->format(strtotime('1 January')) . "\n";
  echo $tz . ' with NULL gmmktime: ' . $intl_formatter->format(gmmktime(0, 0, 0, 1, 1)) . "\n";
  echo $tz . ' with NULL DateTimeZone: ' . $intl_formatter->format($date) . "\n";

  $intl_formatter = IntlDateFormatter::create('en_US', IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, 'UTC', IntlDateFormatter::GREGORIAN, 'MMM');
  echo $tz . ' with UTC strtotime: ' . $intl_formatter->format(strtotime('1 January')) . "\n";
  echo $tz . ' with UTC gmmktime: ' . $intl_formatter->format(gmmktime(0, 0, 0, 1, 1)) . "\n";
  echo $tz . ' with UTC DateTimeZone: ' . $intl_formatter->format($date) . "\n";

  $intl_formatter = IntlDateFormatter::create('en_US', IntlDateFormatter::MEDIUM, IntlDateFormatter::MEDIUM, $tz, IntlDateFormatter::GREGORIAN, 'MMM');
  echo $tz . ' with ' . $tz . ': ' . $intl_formatter->format(strtotime('1 January')) . "\n";
  echo $tz . ' with ' . $tz . ' gmmktime: ' . $intl_formatter->format(gmmktime(0, 0, 0, 1, 1)) . "\n";
  echo $tz . ' with ' . $tz . ' DateTimeZone: ' . $intl_formatter->format($date) . "\n";
}

output:

America/Whitehorse with NULL strtotime: Dec
America/Whitehorse with NULL gmmktime: Dec
America/Whitehorse with NULL DateTimeZone: Dec
America/Whitehorse with UTC strtotime: Jan
America/Whitehorse with UTC gmmktime: Jan                  <--------------------------- OK
America/Whitehorse with UTC DateTimeZone: Jan
America/Whitehorse with America/Whitehorse: Dec
America/Whitehorse with America/Whitehorse gmmktime: Dec
America/Whitehorse with America/Whitehorse DateTimeZone: Dec

America/Vancouver with NULL strtotime: Jan
America/Vancouver with NULL gmmktime: Dec
America/Vancouver with NULL DateTimeZone: Jan
America/Vancouver with UTC strtotime: Jan
America/Vancouver with UTC gmmktime: Jan                 <--------------------------- OK
America/Vancouver with UTC DateTimeZone: Jan
America/Vancouver with America/Vancouver: Jan
America/Vancouver with America/Vancouver gmmktime: Dec
America/Vancouver with America/Vancouver DateTimeZone: Jan

Australia/Sydney with NULL strtotime: Jan
Australia/Sydney with NULL gmmktime: Jan
Australia/Sydney with NULL DateTimeZone: Jan
Australia/Sydney with UTC strtotime: Dec
Australia/Sydney with UTC gmmktime: Jan                      <--------------------------- OK
Australia/Sydney with UTC DateTimeZone: Dec
Australia/Sydney with Australia/Sydney: Jan
Australia/Sydney with Australia/Sydney gmmktime: Jan
Australia/Sydney with Australia/Sydney DateTimeZone: Jan

@mlutfy
Copy link
Member Author

mlutfy commented Nov 28, 2022

ah gmmktime and UTC seems to work. I updated my previous comment.

but .. I'm thinking this is silly. In #21157 we replaced those list of months with a hardcoded list. It's boring, but it works, and does not rely on the operating system locale.

In this case, it will break translations until they are updated, but I think the impact is fairly limited (expiry months in particular, and only if using an old-school payment processor).

@demeritcowboy
Copy link
Contributor

One other thing was we ran into chicken/egg with tests, where you can't test the translation of something until it's available in the translation files. The idea with intl was then it's already translated and "Sunday" is a pretty standard thing to translate that doesn't need civi-specific context. And then also it's already translated in all langs.

But yes it does seem a little crazy that it's so hard to just output these known words.

@mlutfy
Copy link
Member Author

mlutfy commented Nov 28, 2022

Updated the PR to hardcore strings and use ts. Since they are short strings, I used the context param, which is rarely used, but I think here is it important to avoid collisions because of words like "May".

I looked at how WordPress and Drupal9 handled this, and opted for Drupal's method (and context naming), because WordPress only relied on code comments for context.

Tested civistrings and it seems OK:

#: CRM/Utils/Date.php
msgctxt "Abbreviated month name"
msgid "Aug"
msgstr ""

@mlutfy
Copy link
Member Author

mlutfy commented Nov 28, 2022

I agree it's kind of silly to re-translate strings that have already been translated in PHP, but it does require that the operating system have the locale enabled, which is why most systems tend to re-implement them (including Drupal and WordPress).

I didn't touch the week-names for now, but they probably have the same bug, it's just that we don't have admins (for now) in Whitehorse.

I can update Transifex asap so that tests will pass.

@demeritcowboy
Copy link
Contributor

intl doesn't require the OS to have locales enabled. But I'm not going to push back much here.

@mlutfy
Copy link
Member Author

mlutfy commented Nov 28, 2022

oh ok, hmm, I don't have strong opinions about it. I just felt like playing with those functions was a bit like trying a bunch of random things until one worked :)

@totten
Copy link
Member

totten commented Dec 12, 2022

(@bgm) I agree it's kind of silly to re-translate strings that have already been translated in PHP, but it does require that the operating system have the locale enabled

@eileenmcnaughton and I had a similar round of discussion a few months ago. I initially/instinctively felt that the *.mo were a simpler data-source. As I recall, her point had been the opposite of yours -- that IntlDateFormatter provided a bigger/more reliable set of strings than Civi. Using it would mean better behavior in partially-supported locales -- e.g. suppose you enable th_TH or bn_BD as a valid option for preferred_language, and then send an email with some date-tokens. The date-tokens should match the language of the email (th_TH or bn_BD), but that would be impossible with *.mo data.

So it sounds like we have 3-ish sources of translated month names - and some differing experiences with each:

Source Strengths Limits/Critiques
strftime() Relies on APIs that have been around forever Depends too much on libc/distro/OS configuration
l10n/xx_XX/civicrm.mo Supports everything that's in the standard Civi UI Doesn't support site-builders who add their own prose for other locales
l10n extension (IntlDateFormatter) Supports largest list of locales Makes l10n mandatory ?? (Is it already mandatory?)

If IntlDateFormatter had the same issues as strftime(), then I think that would make this tougher.

FWIW, anecdotally, I've never done anything to install th_TH or bn_BD support on my workstation, but IntlDateFormatter does give translations for those locales.

felt like playing with those functions was a bit like trying a bunch of random things until one worked :)

Ah, yes, the Scientific Method. 😃

I like your idea of using UTC when requesting bare names. Would #25154 be a way to adopt UTC (and fix TZ subtleties for Whitehorse Yukon) while still using IntlDateFormatter's larger dataset?

@demeritcowboy
Copy link
Contributor

Merged #25154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants