MDL-50704 user: Do not validate timezones in user objects
authorFrederic Massart <fred@moodle.com>
Mon, 9 May 2016 08:03:42 +0000 (16:03 +0800)
committerFrederic Massart <fred@moodle.com>
Mon, 9 May 2016 09:28:38 +0000 (17:28 +0800)
The validation of the timezone field should not occur, especially
when it is automatically cleaned. Timezones can be volatile, we
must try hard to fallback on real timezones and must not lose reset
the values arbitrarily.

"There is absolutely no need to change $CFG->timezone and user timezones
in database - the timezones may come and go. If you change the value in
upgrade or on the fly you would not be able to get it back. This is the
reason why I implemented the "invalid timezone" thing in server and
user settings instead." - Petr Skoda (MDL-49684)

lib/classes/user.php
lib/tests/user_test.php
user/tests/userlib_test.php

index c06a84c..a730cc0 100644 (file)
@@ -337,8 +337,8 @@ class core_user {
                 'choices' => array_merge(array('' => ''), \core_calendar\type_factory::get_list_of_calendar_types()));
         $fields['theme'] = array('type' => PARAM_THEME, 'null' => NULL_NOT_ALLOWED,
                 'default' => theme_config::DEFAULT_THEME, 'choices' => array_merge(array('' => ''), get_list_of_themes()));
-        $fields['timezone'] = array('type' => PARAM_TIMEZONE, 'null' => NULL_NOT_ALLOWED, 'default' => $CFG->timezone,
-                'choices' => core_date::get_list_of_timezones(null, true));
+        $fields['timezone'] = array('type' => PARAM_TIMEZONE, 'null' => NULL_NOT_ALLOWED,
+                'default' => core_date::get_server_timezone()); // Must not use choices here: timezones can come and go.
         $fields['firstaccess'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED);
         $fields['lastaccess'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED);
         $fields['lastlogin'] = array('type' => PARAM_INT, 'null' => NULL_NOT_ALLOWED);
@@ -528,7 +528,7 @@ class core_user {
      * Get the choices of the property.
      *
      * This is a helper method to validate a value against a list of acceptable choices.
-     * For instance: country, timezone, language, themes and etc.
+     * For instance: country, language, themes and etc.
      *
      * @param string $property property name to be retrieved.
      * @throws coding_exception if the requested property name is invalid or if it does not has a list of choices.
index cc64ba0..03192a5 100644 (file)
@@ -363,15 +363,6 @@ class core_user_testcase extends advanced_testcase {
         $this->assertArrayNotHasKey('unknowntheme', $choices);
         $this->assertArrayNotHasKey('wrongtheme', $choices);
 
-        // Test against timezone property choices.
-        $choices = core_user::get_property_choices('timezone');
-        $this->assertArrayHasKey('America/Sao_Paulo', $choices);
-        $this->assertArrayHasKey('Australia/Perth', $choices);
-        $this->assertArrayHasKey('99', $choices);
-        $this->assertArrayHasKey('UTC', $choices);
-        $this->assertArrayNotHasKey('North Korea', $choices);
-        $this->assertArrayNotHasKey('New york', $choices);
-
         // Try to fetch type of a non-existent properties.
         $nonexistingproperty = 'language';
         $this->setExpectedException('coding_exception', 'Invalid property requested: ' . $nonexistingproperty);
@@ -386,6 +377,7 @@ class core_user_testcase extends advanced_testcase {
      */
     public function test_get_property_default() {
         global $CFG;
+        $this->resetAfterTest();
 
         $country = core_user::get_property_default('country');
         $this->assertEquals($CFG->country, $country);
@@ -400,12 +392,14 @@ class core_user_testcase extends advanced_testcase {
         $lang = core_user::get_property_default('lang');
         $this->assertEquals($CFG->lang, $lang);
 
+        $this->setTimezone('Europe/London', 'Pacific/Auckland');
+        core_user::reset_caches();
         $timezone = core_user::get_property_default('timezone');
-        $this->assertEquals($CFG->timezone, $timezone);
-        set_config('timezone', 99);
+        $this->assertEquals('Europe/London', $timezone);
+        $this->setTimezone('99', 'Pacific/Auckland');
         core_user::reset_caches();
         $timezone = core_user::get_property_default('timezone');
-        $this->assertEquals(99, $timezone);
+        $this->assertEquals('Pacific/Auckland', $timezone);
 
         $this->setExpectedException('coding_exception', 'Invalid property requested, or the property does not has a default value.');
         core_user::get_property_default('firstname');
index edf2273..77b410e 100644 (file)
@@ -99,7 +99,7 @@ class core_userliblib_testcase extends advanced_testcase {
         $user->country = 'WW';
         $user->lang = 'xy';
         $user->theme = 'somewrongthemename';
-        $user->timezone = 'Paris';
+        $user->timezone = '30.5';
         $user->url = 'wwww.somewrong@#$url.com.aus';
         $debugmessages = $this->getDebuggingMessages();
         user_update_user($user, true, false);
@@ -182,7 +182,7 @@ class core_userliblib_testcase extends advanced_testcase {
         $user['country'] = 'WW';
         $user['lang'] = 'xy';
         $user['theme'] = 'somewrongthemename';
-        $user['timezone'] = 'Paris';
+        $user['timezone'] = '-30.5';
         $user['url'] = 'wwww.somewrong@#$url.com.aus';
         $debugmessages = $this->getDebuggingMessages();
         $user['id'] = user_create_user($user, true, false);
@@ -190,7 +190,7 @@ class core_userliblib_testcase extends advanced_testcase {
         $dbuser = $DB->get_record('user', array('id' => $user['id']));
         $this->assertEquals($dbuser->country, 0);
         $this->assertEquals($dbuser->lang, 'en');
-        $this->assertEquals($dbuser->timezone, 'Australia/Perth');
+        $this->assertEquals($dbuser->timezone, '');
 
         // Now, with valid user data.
         $user['username'] = 'johndoe321';