Skip to content

Commit

Permalink
MDL-50704 user: Do not validate timezones in user objects
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
Frederic Massart committed May 9, 2016
1 parent 88474db commit 16825c4
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 18 deletions.
6 changes: 3 additions & 3 deletions lib/classes/user.php
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,8 @@ protected static function fill_properties_cache() {
'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);
Expand Down Expand Up @@ -528,7 +528,7 @@ public static function get_property_null($property) {
* 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.
Expand Down
18 changes: 6 additions & 12 deletions lib/tests/user_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -363,15 +363,6 @@ public function test_get_property_choices() {
$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);
Expand All @@ -386,6 +377,7 @@ public function test_get_property_choices() {
*/
public function test_get_property_default() {
global $CFG;
$this->resetAfterTest();

$country = core_user::get_property_default('country');
$this->assertEquals($CFG->country, $country);
Expand All @@ -400,12 +392,14 @@ public function test_get_property_default() {
$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');
Expand Down
6 changes: 3 additions & 3 deletions user/tests/userlib_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public function test_user_update_user() {
$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);
Expand Down Expand Up @@ -182,15 +182,15 @@ public function test_create_users() {
$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);
$this->assertDebuggingCalledCount(6, $debugmessages);
$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';
Expand Down

0 comments on commit 16825c4

Please sign in to comment.