Moodle
  1. Moodle
  2. MDL-26753 DST issues
  3. MDL-27577

/lib/moodlelib.php functions userdate() and make_timestamp() were applying dst_offset for numeric timezones, i.e. 0 (UTC)

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.12, 2.0
    • Fix Version/s: 1.9.13, 2.0.4
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1. Login as admin
      2. Update timezone (Site administration -> location -> update timezone)
      3. Run testmoodlelib.php simpletest.

      Instructions to run testmoodlelib.php simpletest
      On Moodle 2.xx
      1. Navigate to unit test (Development -> unit test)
      2. enter "lib/simpletest/testmoodlelib.php" in textbox.
      3. click "Run test"

      On Moodle 1.9x
      1. Navigate to unit test (Reports -> unit test)
      2. enter "lib/simpletest/testmoodlelib.php" in textbox.
      3. click "Run test"

      Show
      1. Login as admin 2. Update timezone (Site administration -> location -> update timezone) 3. Run testmoodlelib.php simpletest. Instructions to run testmoodlelib.php simpletest On Moodle 2.xx 1. Navigate to unit test (Development -> unit test) 2. enter "lib/simpletest/testmoodlelib.php" in textbox. 3. click "Run test" On Moodle 1.9x 1. Navigate to unit test (Reports -> unit test) 2. enter "lib/simpletest/testmoodlelib.php" in textbox. 3. click "Run test"
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Pull Master Branch:
      wip-mdl-27577-master
    • Rank:
      17237

      Description

      /lib/moodlelib.php functions userdate() and make_timestamp() were applying dst_offset for numeric timezones, i.e. 0 (UTC)
      Our remedy:

      function make_timestamp($year, $month=1, $day=1, $hour=0, $minute=0, $second=0, $timezone=99, $applydst=true) {
      
          $passedtimezone = $timezone;
          $strtimezone = NULL;
          if (!is_numeric($timezone)) {
              $strtimezone = $timezone;
          }
      
          $timezone = get_user_timezone_offset($timezone);
      
          if (abs($timezone) > 13) {
              $time = mktime((int)$hour, (int)$minute, (int)$second, (int)$month, (int)$day, (int)$year);
          } else {
              $time = gmmktime((int)$hour, (int)$minute, (int)$second, (int)$month, (int)$day, (int)$year);
              $time = usertime($time, $timezone);
              if ($applydst && ($passedtimezone == 99 || !is_numeric($passedtimezone))) {
                  $time -= dst_offset_on($time, $strtimezone);
              }
          }
      
          return $time;
      
      }
      
      in userdate(): 
      ... // added if clause
          if ($timezone == 99 || !is_numeric($timezone)) {
              $date += dst_offset_on($date, $strtimezone);
          }
      ...
      
      1. test_mdl27577.php
        1.0 kB
        Brent Boghosian
      2. test_mdl27577.php
        0.9 kB
        Brent Boghosian

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Hi Rajesh. A lot of the new unit tests seem to be failing. I'm working in the master branch. The output I get when running the unit tests is below.

          Pass: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Friday, 1 July 2011, 10:00 AM => Actual: Friday, 1 July 2011, 10:00 AM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Friday, 1 July 2011, 07:00 AM => Actual: Friday, 1 July 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Friday, 1 July 2011, 07:00 AM => Actual: Friday, 1 July 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Pass: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Saturday, 1 January 2011, 10:00 AM => Actual: Saturday, 1 January 2011, 10:00 AM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Saturday, 1 January 2011, 06:00 AM => Actual: Saturday, 1 January 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate
          Expected: Saturday, 1 January 2011, 06:00 AM => Actual: Saturday, 1 January 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp
          Expected: 1309528800 => Actual: 1309514400 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp
          Expected: 1309528800 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp
          Expected: 1309525200 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932]
          Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp
          Expected: 1309525200 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932]

          Show
          Andrew Davis added a comment - Hi Rajesh. A lot of the new unit tests seem to be failing. I'm working in the master branch. The output I get when running the unit tests is below. Pass: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Friday, 1 July 2011, 10:00 AM => Actual: Friday, 1 July 2011, 10:00 AM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Friday, 1 July 2011, 07:00 AM => Actual: Friday, 1 July 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Friday, 1 July 2011, 07:00 AM => Actual: Friday, 1 July 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Pass: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Saturday, 1 January 2011, 10:00 AM => Actual: Saturday, 1 January 2011, 10:00 AM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Saturday, 1 January 2011, 06:00 AM => Actual: Saturday, 1 January 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_userdate Expected: Saturday, 1 January 2011, 06:00 AM => Actual: Saturday, 1 January 2011, 06:00 PM at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 834] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp Expected: 1309528800 => Actual: 1309514400 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp Expected: 1309528800 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp Expected: 1309525200 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932] Fail: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_make_timestamp Expected: 1309525200 => Actual: 1309485600 at [/var/www/reviewmaster/lib/simpletest/testmoodlelib.php line 932]
          Hide
          Andrew Davis added a comment - - edited

          1) Can 'usertimezone' be a numeric value instead of the string representation of the time zone? If so add a few of these to your tests.

          2) Can 'timezone' be anything other than 0, 99 or a string time zone name? Possibly add a few other tests there to.

          3) Finally, for extra thoroughness consider adding some tests where you supply bad data. Time zone names that don't exist, numbers that don't make sense, that sort of thing. Just to check that we do something somewhat sensible when invalid data is received.

          We just need to cover all the nasty edge cases that can never happen (until they do)

          Show
          Andrew Davis added a comment - - edited 1) Can 'usertimezone' be a numeric value instead of the string representation of the time zone? If so add a few of these to your tests. 2) Can 'timezone' be anything other than 0, 99 or a string time zone name? Possibly add a few other tests there to. 3) Finally, for extra thoroughness consider adding some tests where you supply bad data. Time zone names that don't exist, numbers that don't make sense, that sort of thing. Just to check that we do something somewhat sensible when invalid data is received. We just need to cover all the nasty edge cases that can never happen (until they do)
          Hide
          Andrew Davis added a comment -

          Ok, now Ive actually read the instructions (sorry) and updated my timezones the tests all pass. I've suggested that the error string output when the tests fail include a suggestion to go do that. Both for people like me who don't read the instructions properly and also for anyone running the unit tests in future.

          Show
          Andrew Davis added a comment - Ok, now Ive actually read the instructions (sorry) and updated my timezones the tests all pass. I've suggested that the error string output when the tests fail include a suggestion to go do that. Both for people like me who don't read the instructions properly and also for anyone running the unit tests in future.
          Hide
          Rajesh Taneja added a comment -

          As discussed, the above errors are because of timezone not being updated. Now the fail error will notify that.
          Added few more testcases to increase test coverage.

          Show
          Rajesh Taneja added a comment - As discussed, the above errors are because of timezone not being updated. Now the fail error will notify that. Added few more testcases to increase test coverage.
          Hide
          Andrew Davis added a comment -

          Looks good

          Show
          Andrew Davis added a comment - Looks good
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm,

          I'm passing the test because it seems that the offset is properly calculated always... but I'm getting this errors in my Mac:

          Expected: Friday, 1 July 2011, 10:00 AM => Actual: Friday, 1 July 2011, 10:00 am
          

          So really tests aren't passing here, because my local return the AM/PM information lowercased.

          So I'd suggest to create new issue to make moodle_strtolower() comparison or, alternatively, take out any literal from the format returned and use instead YYYY-MM-DD HH:MM:SS or so.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, I'm passing the test because it seems that the offset is properly calculated always... but I'm getting this errors in my Mac: Expected: Friday, 1 July 2011, 10:00 AM => Actual: Friday, 1 July 2011, 10:00 am So really tests aren't passing here, because my local return the AM/PM information lowercased. So I'd suggest to create new issue to make moodle_strtolower() comparison or, alternatively, take out any literal from the format returned and use instead YYYY-MM-DD HH:MM:SS or so. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now upstream, yay! Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rajesh, I've been playing with this a bit more and I think that some of your tests are only going to work in sites being Perth/Australia @ OS, because test results are dependent of that.

          To be able to reproduce that, just add this line:

          date_default_timezone_set('Europe/Madrid');

          in the very fist lines of test_userdate() and you will get the offending ones. I think they are the ones calculating based on server time. I get 4 errors here.

          Ciao

          PS: If you set 'Australia/Perth' above, I think it's your case and then they work. Test should be independent of that IMO.

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rajesh, I've been playing with this a bit more and I think that some of your tests are only going to work in sites being Perth/Australia @ OS, because test results are dependent of that. To be able to reproduce that, just add this line: date_default_timezone_set('Europe/Madrid'); in the very fist lines of test_userdate() and you will get the offending ones. I think they are the ones calculating based on server time. I get 4 errors here. Ciao PS: If you set 'Australia/Perth' above, I think it's your case and then they work. Test should be independent of that IMO.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Also, I've here the lowercase comparison ready for commit (because some OSs handle the %p differently), once tests are ok I'll be happy to apply my patch (unless you do that before).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Also, I've here the lowercase comparison ready for commit (because some OSs handle the %p differently), once tests are ok I'll be happy to apply my patch (unless you do that before). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is the commit that makes the tests case-insensitive. It also prints the offending data when one of the loop-tests fail. You will notice that the ones failing are those implying one server timezone ('Australia/Perth') as commented above.

          Feel free to add it to your own work...ciao

          PS: Of course, if you want, one new MDL issue can be created to handle this problem with the unittests +1

          Show
          Eloy Lafuente (stronk7) added a comment - This is the commit that makes the tests case-insensitive. It also prints the offending data when one of the loop-tests fail. You will notice that the ones failing are those implying one server timezone ('Australia/Perth') as commented above. Feel free to add it to your own work...ciao PS: Of course, if you want, one new MDL issue can be created to handle this problem with the unittests +1
          Hide
          Rajesh Taneja added a comment -

          As this has been integrated, I have created another bug MDL-27863 to fix both issues (Case insensitive and servertime other then Australia/Perth)
          Hope that is fine with you

          Show
          Rajesh Taneja added a comment - As this has been integrated, I have created another bug MDL-27863 to fix both issues (Case insensitive and servertime other then Australia/Perth) Hope that is fine with you
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, perfect, thanks Rajesh!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, perfect, thanks Rajesh!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: