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

Auto-detect timezone and currency in installer #13092

Merged
merged 3 commits into from Sep 20, 2018

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Jun 21, 2018

To make it easier to install Matomo, the installer should try to auto-detect the timezone and currency.

If PHP or the operating system is configured to use a timezone other than UTC, it is highly likely that this timezone is relevant to use as the default timezone for Matomo. We should preselect this timezone in the timezone selector in the installer, so the user does not have to look for the proper timezone in the huge list of timezones.

Once we have the timezone, we can find the corresponding country and (if the Intl extension is installed) currency. This is likely a better default than the hard-coded default, USD.

@c960657 c960657 changed the title Auto-detect timezone and currency Auto-detect timezone and currency in installer Jun 21, 2018
@mattab mattab added this to the 3.7.0 milestone Jun 26, 2018
@mattab mattab added the Needs Review PRs that need a code review label Jun 26, 2018
// Use server timezone as default. If server timezone is UTC, it is likely
// a default not specified explicitly by the sysadm, so ignore this.
$timezone = PIWIK_DEFAULT_TIMEZONE;
if (in_array($timezone, array('UTC', 'Etc/UTC', 'GMT', 'Etc/GMT'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth it to compare this all Lowercase? Or maybe they cannot be set in different cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It seems that PHP does indeed treat timezones as case-insensitive. I didn't know that.

$timezone = PIWIK_DEFAULT_TIMEZONE;
if (in_array($timezone, array('UTC', 'Etc/UTC', 'GMT', 'Etc/GMT'))) {
$timezone = null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to check whether Matomo "knows" the timezone (present in $timezones)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value determines the default selected timezone in the select field. If an invalid timezone is submitted, QuickForm will just ignore it, so there is no need for further validation.

@tsteur
Copy link
Member

tsteur commented Sep 16, 2018

Awesome 👍 I've just installed the Intl extension, debugged it step by step and worked 👍 so valuable 👍

Be all good to merge but there is one problem... in some of our scripts (they are not in this repository) we are setting the default timezone and currency before creating the website. By applying this PR it would no longer work and they would be overwritten. We would need to add some checks to skip the detection if for some reason a default currency or timezone is already set. Like this:

diff --git a/plugins/Installation/FormFirstWebsiteSetup.php b/plugins/Installation/FormFirstWebsiteSetup.php
index 13d4dbc4e3..e83a246b8f 100644
--- a/plugins/Installation/FormFirstWebsiteSetup.php
+++ b/plugins/Installation/FormFirstWebsiteSetup.php
@@ -15,6 +15,7 @@ use HTML_QuickForm2_Factory;
 use HTML_QuickForm2_Rule;
 use NumberFormatter;
 use Piwik\Access;
+use Piwik\Option;
 use Piwik\Piwik;
 use Piwik\Plugins\SitesManager\API;
 use Piwik\QuickForm2;
@@ -46,6 +47,11 @@ class FormFirstWebsiteSetup extends QuickForm2
             $timezone = null;
         }
 
+        if (Option::get(API::OPTION_DEFAULT_TIMEZONE)) {
+            // a default timezone was already configured
+            $timezone = null;
+        }
+
         $this->addElement('text', 'siteName')
             ->setLabel(Piwik::translate('Installation_SetupWebSiteName'))
             ->addRule('required', Piwik::translate('General_Required', Piwik::translate('Installation_SetupWebSiteName')));
@@ -99,7 +105,7 @@ class Rule_isValidTimezone extends HTML_QuickForm2_Rule
         }
 
         // If intl extension is installed, get default currency from timezone country.
-        if ($timezone && class_exists('NumberFormatter')) {
+        if (!Option::get(API::OPTION_DEFAULT_CURRENCY) && $timezone && class_exists('NumberFormatter')) {
             try {
                 $zone = new DateTimeZone($timezone);
                 $location = $zone->getLocation();

@c960657
Copy link
Contributor Author

c960657 commented Sep 17, 2018

@tsteur Can you provide an example of such a script? If the default timezone/currency is already stored in the database at this point, I might be able to retrieve it using API::getInstance()->getDefaultTimezone() and ->getDefaultCurrency() and use that to bypass the detection logic?

@tsteur
Copy link
Member

tsteur commented Sep 17, 2018

I was thinking of API::getInstance()->getDefaultTimezone() as well initially but then you need to know the default value as it always returns a value like (API::getInstance()->getDefaultTimezone() !== 'UTC') and if this value changes it could break etc. And the default value for currency is USD and you wouldn't know whether it was set manually before or whether it is the default value

The script would do eg

// php timezone might be "Europe/Berlin" for example
$sitesApi->setDefaultTimezone('abcdef...');
$sitesApi->setDefaultCurrency('USD');
$formWebsiteSetup = new FormFirstWebsiteSetup();
...

@c960657
Copy link
Contributor Author

c960657 commented Sep 20, 2018

Done.

@tsteur
Copy link
Member

tsteur commented Sep 20, 2018

Cheers 👍

@tsteur tsteur merged commit e5ba290 into matomo-org:3.x-dev Sep 20, 2018
@c960657 c960657 deleted the detect-timezone-currency branch September 24, 2018 09:29
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Auto-detect timezone and currency

* Compare timezone case-insentively

* Preserve existing defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants