core_user_get_users_by_id - ERRORCODE: invalidresponse

get_list_of_timezones() which is used to populate the timezone list for the user profile edit page appears to return invalid timezones which would be causing invalid timezones to be saved in user records.

The comments around PARAM_TIMZONE say:

PARAM_TIMEZONE - expected timezone. Timezone can be int +(0-13) or float +(0.5-12.5) or string seperated by '/' and can have '-' &/ '_' (eg. America/North_Dakota/New_Salem, America/Port-au-Prince)

The bit I should highlight is the "int +-(0-13)"

Currently get_list_of_timezones() returns some values that are invalid according to the regex for PARAM_TIMEZONE in clean_param.

I knocked up a quick bit of code to test that the values returned are valid:

<?php

define('MOODLE_INTERNAL', 1);                                           
                                                                              
require_once('config.php');                                                
                                             
$timezones = get_list_of_timezones();    
                                                                   
foreach ($timezones as $timezone=>$label) {                             
  if (clean_param($timezone, PARAM_TIMEZONE) != $timezone) {           
    echo "Invalid timezone $timezone<br>";                       
  }                                                              
}

I've included output below showing which ones didn't pass though the regex test:

Invalid timezone -13.0
Invalid timezone -12.0
Invalid timezone -11.0
Invalid timezone -10.0
Invalid timezone 10.0
Invalid timezone 11.0
Invalid timezone 12.0
Invalid timezone 13.0

The regex test is for PARAM_TIMEZONE is this:

/^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

I see two possible solutions:

1) Change get_list_of_timezones() to return valid timezones only (without trailing 0's) and optionally fix up broken data in people's mdl_user table; or
2) Change that regex to allow the timezones returned by get_list_of_timezones(), e.g. /^(([+-]?(0?[0-9](\.[5|0])?|1[0-3]|1[0-2]\.[5|0]))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

Actually that regex in option 2 was bad and would have failed for timezone "13.0".

I've attached a patch which has a better (working) one and updated the unit tests in lib/tests/moodlelib_tests.php

Thanks Nathan, I think too that the regex is missing .0 check. Thanks for the fix, and for the unit test update
Attach a git commit next time, it make it easy to keep record of the commit author.

Hi Jerome - sorry, thought I had attached a commit. Will check next time.

Just looking at you're diff now. Looks like you used my first regex. Its bad because it doesn't validate "-13.0" or "13.0" as timezones which are values returned from get_list_of_timezones().

The one below (from the attached patch) is better because it allows an optional ".0" after any + or - integer-only value but doesn't permit values of "13.5" or "-13.5" which shouldn't pass.

/^(([+-]?(0?[0-9](\.[5|0])?|1[0-3](\.0)?|1[0-2]\.5))|(99)|[[:alnum:]](\/?[[:alpha:]_-]))$/

Ah thanks Nathan. I saw that and got influenced by phpdoc in the way, and forget the main reason I thought supporting all .0 is good. I think we should support all YX.0 in order to be consistent as we support X.0 (in case peerreviewer/integrator wonder why).
Changing the regex.

Sending to peer-review.

Thanks Jerome,
Patch looks perfect. Pushing for integration review.
[y] Syntax
[y] Output
[y] Whitespace
[-] Language
[-] Databases
[y] Testing
[-] Security
[-] Documentation
[y] Git
[y] Sanity check

Thanks Jerome, integrated to 22, 23, 24 and master.

Working fine

Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

Closing, ciao