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
Translate timezone names #12901
Translate timezone names #12901
Conversation
c49303e
to
6a7a52f
Compare
b66b6f0
to
8e20917
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ksort($return); | ||
foreach ($return as $continent => $countries) { | ||
asort($return[$continent]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think you'll need to iterate by reference for the asort()
to work (ie, foreach ($return as $continent => &$countries)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could need the &
if I did asort($countries)
. But here I just sort the value in the outer array by reference, so I believe it's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, misread this line.
} | ||
} | ||
} catch (\Exception $e) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What specifically can throw an exception here? Would be good put the try-catch around only the code we're expecting could throw, so other exceptions aren't silenced. (Same for some try-catches below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only new DateTimeZone()
will throw. I have changed the try/catch block to include only that, and do skip the rest of the loop if an exception is thrown.
plugins/SitesManager/API.php
Outdated
@@ -337,6 +338,10 @@ public function getSitesWithAdminAccess($fetchAliasUrls = false, $pattern = fals | |||
} | |||
} | |||
|
|||
foreach ($sites as &$site) { | |||
$site['timezone_name'] = $this->getTimezoneName($site['timezone']); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, this property should be added to other getSitesWith...
methods. Would be good to put it in getSitesFromIds()
& then a for loop after the above call to getPatternMatchSites()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
$resultWanted = array( | ||
0 => array("idsite" => 1, "name" => "site1", "main_url" => "http://piwik.net", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0), | ||
1 => array("idsite" => 3, "name" => "site3", "main_url" => "http://piwik.org", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0), | ||
0 => array("idsite" => 1, "name" => "site1", "main_url" => "http://piwik.net", "ecommerce" => 0, "excluded_ips" => "", 'sitesearch' => 1, 'sitesearch_keyword_parameters' => '', 'sitesearch_category_parameters' => '', 'excluded_parameters' => '', 'excluded_user_agents' => '', 'timezone' => 'UTC', 'timezone_name' => 'GMT', 'currency' => 'USD', 'group' => '', 'keep_url_fragment' => 0, 'type' => 'website', 'exclude_unknown_urls' => 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timezone name for UTC
should be UTC
and not GMT
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, CLDR uses GMT. This translation is used in most locales. I don't have much opinion on the specific translation, but I generally think we should follow CLDR (a high-quality source backed by large industry players such as Microsoft, Google and Apple) unless there are strong arguments against it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an astronomy pedantic I have to mention that UTC and GMT are not the same, even though they are quite similar:
For most purposes, UTC is considered interchangeable with Greenwich Mean Time (GMT) but GMT is no longer precisely defined by the scientific community.
https://en.wikipedia.org/wiki/Coordinated_Universal_Time
But to be fair the differences don't really matter, even though I think most people expect reading UTC in an IT-context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, it's important to write "UTC" everywhere (and "GMT" should not be visible anywhere) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is consensus that we want this, I'll skip importing translations for “UTC” from CLDR and fetch them from Transifex instead. However, I would much prefer if we could outsource such decisions to CLDR, so we can focus on matters specific to web analytics.
Even though I tend to agree that UTC is more prevalent these days and is technically more correct, I would say that both are acceptable. E.g. Google Analytics uses GMT.
FWIW, Microsoft used GMT in previous versions of Windows, but now they use UTC, at least in the English version. Microsoft are active contributors to CLDR, but it seems they have chosen to deviate from the CLDR data her. They may wish to support a proposal to change the CLDR translation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @c960657 we want to see UTC everywhere (and nowhere GMT)
$this->markTestSkipped('timezones needs to be supported'); | ||
} | ||
|
||
Translate::loadAllTranslations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to not use translations in this test and instead assert that the translation keys are what we expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. This would make e.g. “UTC+1” be returned as “Intl_GmtFormat” (the offset placeholder would be ignores), and we wouldn't be able to test the distinction between known and unknown cities (Asia/Foo_Bar vs. America/New_York).
* @group APITest | ||
* @group Plugins | ||
*/ | ||
class APITest extends \PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for adding a test!
8e20917
to
be3e706
Compare
Thanks for the review :) |
The translation for UTC is now fetched from plugins/SitesManager/lang/en.json instead. |
* Translate timezone names * Add translations * Remove unnecessary help text * Wrap less code in try/catch * Always populate timezone_name * Use "UTC" instead of "GMT" from CLDR * Update screenshot
The timezone selector in the sites manager is not localised but shows all city names in English.
When using Matomo in Italian, a user would expect the list to contain “Roma”, not “Rome”. And a user using Matomo in Russian would look for “Москва́”, not “Moscow”. Even an English user would expect “Dumont d’Urville” instead of “DumontDUrville” (for
Antarctica/DumontDUrville
).CLDR contains translations of all city names. However, the CLDR spec suggests that timezone selectors should allow users to select the country name, not the city name. City names should only be shown for countries spanning multiple timezones. Of course, this is just a UI recommendation, not a requirement of any sort, but I think it makes sense.
You quite often see this long list of cities in timezone selectors in various software, and for a developer who is familiar with the tz database it may seem natural. I suspect that this UI is popular, because it is easy to construct the list based on the timezone identifiers alone. From and end-user point of view I think that a list of countries is the natural choice. For some countries it is not obvious which city is the one used in the tz database (the capital may not be the largest city), and for some regions the tz identifier is not a city name but the name of an island. Countries are well-defined and used in many contexts.
This PR implements a UI as suggested in the CLDR spec. It looks like this (excerpt):
The timezones are now grouped by the same continent definition as used elsewhere in Matomo, instead of the definition used in the tz database. This is more consistent from an end-user point of view.