Moodle
  1. Moodle
  2. MDL-35412

Date Selector causes a json_encode error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • To be tested on a Windows server
      • Install the Japanese language pack

      Test 1

      1. Go to your home page and switch to English language pack
      2. Go to the calendar, and add a new event
      3. Make sure that the pop-up date selector correctly displays each month
        • Month and days are correctly translated (February is not March, check that you'll never have two similar names while going from one month to another)
        • No errors are displayed during the process

      Test 2

      1. Go to your home page and switch to Japanese
      2. Go to the calendar, and add a new event
      3. Make sure that the pop-up date selector correctly displays each month
        • Month and days are correctly translated
        • No errors are displayed during the process
      Show
      Test pre-requisites To be tested on a Windows server Install the Japanese language pack Test 1 Go to your home page and switch to English language pack Go to the calendar, and add a new event Make sure that the pop-up date selector correctly displays each month Month and days are correctly translated (February is not March, check that you'll never have two similar names while going from one month to another) No errors are displayed during the process Test 2 Go to your home page and switch to Japanese Go to the calendar, and add a new event Make sure that the pop-up date selector correctly displays each month Month and days are correctly translated No errors are displayed during the process
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35412-master
    • Rank:
      44091

      Description

      Opening this issue to investigate and fix the following reported by Yamashita Kōichi on MDL-35171.

      Since Moodle 2.3.2, "json_encode(): Invalid UTF-8 sequence in argument" occurs when editing quizzes etc., on Windows servers with some locales (e.g. Japanese). Changing the code in formslib like below fixes that.

      'mon' => userdate(strtotime("Monday"), '%a'), // 5th Jan 1970 at 12pm
      'january' => userdate(strtotime("January"), '%B'), // 1st Jan 1970 at 12pm

      My environment is: Moodle 2.3.2, Windows 7, IIS 7.5, PHP 5.3.14 Zend Server, debug is set to DEVELOPER.

      I get errors when editing a quiz (date selector is used for Timing > Open/Close the quiz), in some locales (en is OK but ja, fr, de all have errors).
      strftime can return results only in multibyte encoding on Windows so we need to convert back to UTF-8 for use in moodle. (userdate() in moodlelib.php does that.)

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Yamashita, we've got this on our TODO, but if you are willing to go further in helping we would very much appreciate a patch that implements what you are suggesting. Although userdate by itself could lead to other problems and some more parameters should be passed to it. Thank you!

          Show
          Frédéric Massart added a comment - Yamashita, we've got this on our TODO, but if you are willing to go further in helping we would very much appreciate a patch that implements what you are suggesting. Although userdate by itself could lead to other problems and some more parameters should be passed to it. Thank you!
          Hide
          Yamashita Kōichi added a comment -

          Thanks, I'm using this code on my server. Hmm I'd better get a github account and upload a patch?

          'mon' => userdate(strtotime("Monday"), '%a'),
          'tue' => userdate(strtotime("Tuesday"), '%a'),
          'wed' => userdate(strtotime("Wednesday"), '%a'),
          'thu' => userdate(strtotime("Thursday"), '%a'),
          'fri' => userdate(strtotime("Friday"), '%a'),
          'sat' => userdate(strtotime("Saturday"), '%a'),
          'sun' => userdate(strtotime("Sunday"), '%a'),
          'january' => userdate(strtotime("January 15"), '%B'),
          'february' => userdate(strtotime("February 15"), '%B'),
          'march' => userdate(strtotime("March 15"), '%B'),
          'april' => userdate(strtotime("April 15"), '%B'),
          'may' => userdate(strtotime("May 15"), '%B'),
          'june' => userdate(strtotime("June 15"), '%B'),
          'july' => userdate(strtotime("July 15"), '%B'),
          'august' => userdate(strtotime("August 15"), '%B'),
          'september' => userdate(strtotime("September 15"), '%B'),
          'october' => userdate(strtotime("October 15"), '%B'),
          'november' => userdate(strtotime("November 15"), '%B'),
          'december' => userdate(strtotime("December 15"), '%B')

          Show
          Yamashita Kōichi added a comment - Thanks, I'm using this code on my server. Hmm I'd better get a github account and upload a patch? 'mon' => userdate(strtotime("Monday"), '%a'), 'tue' => userdate(strtotime("Tuesday"), '%a'), 'wed' => userdate(strtotime("Wednesday"), '%a'), 'thu' => userdate(strtotime("Thursday"), '%a'), 'fri' => userdate(strtotime("Friday"), '%a'), 'sat' => userdate(strtotime("Saturday"), '%a'), 'sun' => userdate(strtotime("Sunday"), '%a'), 'january' => userdate(strtotime("January 15"), '%B'), 'february' => userdate(strtotime("February 15"), '%B'), 'march' => userdate(strtotime("March 15"), '%B'), 'april' => userdate(strtotime("April 15"), '%B'), 'may' => userdate(strtotime("May 15"), '%B'), 'june' => userdate(strtotime("June 15"), '%B'), 'july' => userdate(strtotime("July 15"), '%B'), 'august' => userdate(strtotime("August 15"), '%B'), 'september' => userdate(strtotime("September 15"), '%B'), 'october' => userdate(strtotime("October 15"), '%B'), 'november' => userdate(strtotime("November 15"), '%B'), 'december' => userdate(strtotime("December 15"), '%B')
          Hide
          Frédéric Massart added a comment -

          Thanks, yes it would be really good if you could upload a branch to a github account, that makes everyone's life a lot easier here.

          Also, your patch surely works on your installation, but might not on others. The problem is that userdate() will try modify the timestamp passed to match the settings of the user. Let's say it is 1am on Friday and the server is based at UTC+8 but the user is at UTC-8, userdate() will edit the timestamp so that it will be Thursday 9 am. It will also handle daylight saving which could also lead to potential random issues.

          Your patch would do the trick for months, but not for the days of the week. Also, passing 99 as the timezone to userdate() is not enough as it could be overwritten by get_user_timezone_offset(). From what I have seen, a less hacky way would perhaps be to create a new function which contains the logic to convert the date format from and to utf-8. We could then use that one instead of userdate() which does too many other things.

          Let me know your thoughts about it.

          Show
          Frédéric Massart added a comment - Thanks, yes it would be really good if you could upload a branch to a github account, that makes everyone's life a lot easier here. Also, your patch surely works on your installation, but might not on others. The problem is that userdate() will try modify the timestamp passed to match the settings of the user. Let's say it is 1am on Friday and the server is based at UTC+8 but the user is at UTC-8, userdate() will edit the timestamp so that it will be Thursday 9 am. It will also handle daylight saving which could also lead to potential random issues. Your patch would do the trick for months, but not for the days of the week. Also, passing 99 as the timezone to userdate() is not enough as it could be overwritten by get_user_timezone_offset(). From what I have seen, a less hacky way would perhaps be to create a new function which contains the logic to convert the date format from and to utf-8. We could then use that one instead of userdate() which does too many other things. Let me know your thoughts about it.
          Hide
          Yamashita Kōichi added a comment -

          Thanks for reply, Frédéric. I found lang/calendar.php defines strings for days of the week for both full forms and abbreviations, that can be fetched like get_string('monday', 'calendar'), get_string('mon', 'calendar').

          I think it's clearly safer and no encoding problem so how about using it for days of the week? I took a look of some relatively minor languages and saw most of them had those strings.

          Months are not defined anywhere and seem to be fetched by time functions. Below seems the code used for fetching months for HTML forms.

          lib/form/datetimeselector.php:102
          for ($i=1; $i<=12; $i++)

          { $months[$i] = userdate(gmmktime(12,0,0,$i,15,2000), "%B"); }
          Show
          Yamashita Kōichi added a comment - Thanks for reply, Frédéric. I found lang/calendar.php defines strings for days of the week for both full forms and abbreviations, that can be fetched like get_string('monday', 'calendar'), get_string('mon', 'calendar'). I think it's clearly safer and no encoding problem so how about using it for days of the week? I took a look of some relatively minor languages and saw most of them had those strings. Months are not defined anywhere and seem to be fetched by time functions. Below seems the code used for fetching months for HTML forms. lib/form/datetimeselector.php:102 for ($i=1; $i<=12; $i++) { $months[$i] = userdate(gmmktime(12,0,0,$i,15,2000), "%B"); }
          Hide
          Frédéric Massart added a comment -

          Hi Yamashita,

          I understand your proposition, but to it looks too hacky as userdate() performs to many different things on the timestamp. Also, we thought about using the language file for the days of the week, but as we are using the system locale for everything else, we should keep on using the locale as well.

          Here is a patch which seem to fix the issue, I'd appreciate if you could test it. Unfortunately I am not (yet) providing a patch for 2.2 as the userdate() function is slightly different, I am still not sure how to fix this on 2.2.

          Show
          Frédéric Massart added a comment - Hi Yamashita, I understand your proposition, but to it looks too hacky as userdate() performs to many different things on the timestamp. Also, we thought about using the language file for the days of the week, but as we are using the system locale for everything else, we should keep on using the locale as well. Here is a patch which seem to fix the issue, I'd appreciate if you could test it. Unfortunately I am not (yet) providing a patch for 2.2 as the userdate() function is slightly different, I am still not sure how to fix this on 2.2.
          Hide
          Frédéric Massart added a comment -

          Adding a branch for 2.2, quite hacky but I think that is still the cleaner way...

          Waiting for feedback!

          Show
          Frédéric Massart added a comment - Adding a branch for 2.2, quite hacky but I think that is still the cleaner way... Waiting for feedback!
          Hide
          Yamashita Kōichi added a comment -

          Hi Frédéric. I tested your patch on 2.4dev (today's weekly) and it works perfectly. Thanks.

          I reported userdate()'s issue before as MDL-32632 and Petr fixed it by the release of 2.3. Another issue for backporting to 2.2 is open so 2.2 still has problems displaying any dates on Windows with Asian locales.

          Show
          Yamashita Kōichi added a comment - Hi Frédéric. I tested your patch on 2.4dev (today's weekly) and it works perfectly. Thanks. I reported userdate()'s issue before as MDL-32632 and Petr fixed it by the release of 2.3. Another issue for backporting to 2.2 is open so 2.2 still has problems displaying any dates on Windows with Asian locales.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Yamashita!

          @ Reviewers, the 2.2 branch should not be integrated as long as MDL-33545 is not. (Or it could but then MDL-33545 should fix implement its fix for this one as well)

          Show
          Frédéric Massart added a comment - - edited Thanks Yamashita! @ Reviewers, the 2.2 branch should not be integrated as long as MDL-33545 is not. (Or it could but then MDL-33545 should fix implement its fix for this one as well)
          Hide
          Jason Fowler added a comment -

          [+] Syntax
          [+] Output
          [+] Whitespace
          [+] Language
          [/] Databases
          [+] Testing
          [+] Security
          [/] Documentation
          [+] Git
          [+] Sanity check

          Code makes sense, just wondering why the diff for 2.2 is so different to the others?

          Show
          Jason Fowler added a comment - [+] Syntax [+] Output [+] Whitespace [+] Language [/] Databases [+] Testing [+] Security [/] Documentation [+] Git [+] Sanity check Code makes sense, just wondering why the diff for 2.2 is so different to the others?
          Hide
          Jason Fowler added a comment -

          Ah, just saw the previous comment ... that makes sense then

          Show
          Jason Fowler added a comment - Ah, just saw the previous comment ... that makes sense then
          Hide
          Frédéric Massart added a comment -

          Thanks Jason. Thinking about the integration, I think this should be backported to 2.2 as the date selector now suffers from the regression caused by the related issues. The problem with this backport is that it is a fake fix. The UTF-8 strings would be handled but incorrect (see MDL-33545).

          Show
          Frédéric Massart added a comment - Thanks Jason. Thinking about the integration, I think this should be backported to 2.2 as the date selector now suffers from the regression caused by the related issues. The problem with this backport is that it is a fake fix. The UTF-8 strings would be handled but incorrect (see MDL-33545 ).
          Hide
          Michael de Raadt added a comment -

          When this is integrated, please check to see if it resolves MDL-35939.

          Show
          Michael de Raadt added a comment - When this is integrated, please check to see if it resolves MDL-35939 .
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          The changes look good thanks.
          There was one thing I noted however, not really a block to integration but perhaps worth cleaning up now prior to integration.
          On the 2.3 and master branches there is the following code within the new date_format_string function there is the following line:

          if ($CFG->ostype == 'WINDOWS' and ($localewincharset = get_string('localewincharset', 'langconfig'))) {
          

          What gets me about this is that get_string always returns a string, if it fails it throws an exception.
          That get_string call should be ideally moved out of the if statement.
          I know that that particular line was there before within the userdate function, however given its now being abstracted out this is the ideal time to fix that.
          Not a blocker as it still works, but it would be nice to clean this up.

          I'll leave this in integration review, could you please look at it and let me know if you have a mo to clean it up.
          If not that is fine this can still land.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, The changes look good thanks. There was one thing I noted however, not really a block to integration but perhaps worth cleaning up now prior to integration. On the 2.3 and master branches there is the following code within the new date_format_string function there is the following line: if ($CFG->ostype == 'WINDOWS' and ($localewincharset = get_string('localewincharset', 'langconfig'))) { What gets me about this is that get_string always returns a string, if it fails it throws an exception. That get_string call should be ideally moved out of the if statement. I know that that particular line was there before within the userdate function, however given its now being abstracted out this is the ideal time to fix that. Not a blocker as it still works, but it would be nice to clean this up. I'll leave this in integration review, could you please look at it and let me know if you have a mo to clean it up. If not that is fine this can still land. Many thanks Sam
          Hide
          Frédéric Massart added a comment -

          Thanks Sam,

          I have added a commit to 2.3 and master to move the get_string() within the if condition.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Thanks Sam, I have added a commit to 2.3 and master to move the get_string() within the if condition. Cheers! Fred
          Hide
          Sam Hemelryk added a comment -

          So here's a fun one.

          This is causing my unit tests to fail!

          Tracked it back down to the locale's. When a locale is not set and I execute the tests (php default of c) it works correctly, however when I run them with my locale properly set up they fail with the following error:

          There was 1 failure:

          1) moodlelib_testcase::test_date_format_string
          Failed asserting that two strings are equal.
          — Expected
          +++ Actual
          @@ @@
          -'Saturday, 01 January 2011, 06:00 pm'
          +'Saturday, 01 January 2011, 06:00 PM'

          To re-run:
          phpunit moodlelib_testcase lib/tests/moodlelib_test.php

          My locale is currently en_AU.UTF-8 btw.

          Sorry Fred, will have to reopen this until that is fixed.

          Show
          Sam Hemelryk added a comment - So here's a fun one. This is causing my unit tests to fail! Tracked it back down to the locale's. When a locale is not set and I execute the tests (php default of c) it works correctly, however when I run them with my locale properly set up they fail with the following error: There was 1 failure: 1) moodlelib_testcase::test_date_format_string Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'Saturday, 01 January 2011, 06:00 pm' +'Saturday, 01 January 2011, 06:00 PM' To re-run: phpunit moodlelib_testcase lib/tests/moodlelib_test.php My locale is currently en_AU.UTF-8 btw. Sorry Fred, will have to reopen this until that is fixed.
          Hide
          Frédéric Massart added a comment -

          Hum, that is strange... I just ran the PHP Unit and didn't have any error. I will investigate, thanks! (But I don't get why it does not fail on test_userdate())

          Show
          Frédéric Massart added a comment - Hum, that is strange... I just ran the PHP Unit and didn't have any error. I will investigate, thanks! (But I don't get why it does not fail on test_userdate() )
          Hide
          Frédéric Massart added a comment -

          Just updated my 2.3 and master branches with a new commit which fixes the issue. The problem being that some systems return PM capitalised, some don't. (I didn't see those lines in test_userdate()). Cheers for spotting this! Also, the diff is quite big, because I thought that it was a bit ridiculous to add textlib::strtolower() in each test, so I made that in a loop.

          Thanks!

          Show
          Frédéric Massart added a comment - Just updated my 2.3 and master branches with a new commit which fixes the issue. The problem being that some systems return PM capitalised, some don't. (I didn't see those lines in test_userdate()). Cheers for spotting this! Also, the diff is quite big, because I thought that it was a bit ridiculous to add textlib::strtolower() in each test, so I made that in a loop. Thanks!
          Hide
          Sam Hemelryk added a comment -

          In it goes, thanks for cleaning that up Fred

          Show
          Sam Hemelryk added a comment - In it goes, thanks for cleaning that up Fred
          Hide
          Frédéric Massart added a comment -

          Assigning test to Michael.

          Show
          Frédéric Massart added a comment - Assigning test to Michael.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          I tested this in 2.2, 2.3 and master.

          Each showed the month in Japanese as 10月, with the month number varying.

          In 2.2 I encountered the error covered by MDL-33545.

          Show
          Michael de Raadt added a comment - Test result: Success! I tested this in 2.2, 2.3 and master. Each showed the month in Japanese as 10月, with the month number varying. In 2.2 I encountered the error covered by MDL-33545 .
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: