Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.16, 2.0.7, 2.1.4, 2.2.1
    • Fix Version/s: 2.2.3
    • Component/s: General
    • Labels:
    • Rank:
      38306

      Description

      The global function shorten_text should be UTF8 safe

      Currently it uses strlen and substr for perform strings operations over UTF8 text

      This may cause some problems and wrong results

      I.e:

      shorten_text($text, 34) function for the following texts:

      Mañana leña cigüeña camión pequeño returns Mañana leña cigüeña ...
      Manana lena ciguena camion pequeno returns Manana lena ciguena camion pequeno

        Issue Links

          Activity

          Hide
          Juan Leyva added a comment -

          This is a quick test I created:

          
          require_once('config.php');
          
          $text1 = "Mañana leña cigüeña camión pequeño";
          
          echo shorten_text($text1, 34);
          
          $text2 = "Manana lena ciguena camion pequeno";
          
          echo shorten_text($text2, 34);
          
          
          Show
          Juan Leyva added a comment - This is a quick test I created: require_once('config.php'); $text1 = "Mañana leña cigüeña camión pequeño" ; echo shorten_text($text1, 34); $text2 = "Manana lena ciguena camion pequeno" ; echo shorten_text($text2, 34);
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note: I'd test with the utf-8 string above (with and without whitespace) and the shorten_text() function at different lengths (say from 20 to 34) to see if, at any of them, a breakage of utf-8 sequences happen, leading to garbled results.

          Because... one thing is not being utf8-safe (aka, breaking badly in the middle of utf8 chars)... and another is being inaccurate (that this function is, for sure, because it counts bytes instead of chars to determine where to short).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side note: I'd test with the utf-8 string above (with and without whitespace) and the shorten_text() function at different lengths (say from 20 to 34) to see if, at any of them, a breakage of utf-8 sequences happen, leading to garbled results. Because... one thing is not being utf8-safe (aka, breaking badly in the middle of utf8 chars)... and another is being inaccurate (that this function is, for sure, because it counts bytes instead of chars to determine where to short). Ciao
          Hide
          Juan Leyva added a comment - - edited

          New test that makes the function unsafe:

          
          require_once('config.php');
          
          $text1 = "Nó";
          
          echo shorten_text($text1, 2);
          
          $text2 = "No";
          
          echo shorten_text($text2, 2);
          
          
          

          The result is:

          N�...No

          Show
          Juan Leyva added a comment - - edited New test that makes the function unsafe: require_once('config.php'); $text1 = "Nó" ; echo shorten_text($text1, 2); $text2 = "No" ; echo shorten_text($text2, 2); The result is: N�...No
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Integrated, thanks (22 and master). Not backported to 21 because of textlib changes.

          Ciao

          Edited: I've added some more assertions trying to break it. I didn't achieve my goal.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Integrated, thanks (22 and master). Not backported to 21 because of textlib changes. Ciao Edited: I've added some more assertions trying to break it. I didn't achieve my goal.
          Hide
          Tim Barker added a comment -

          Exception: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_fix_utf8
          Unexpected PHP error [iconv(): Detected an illegal character in input string] severity [E_NOTICE] in [/home/tim/webs/moodle_2.2/lib/moodlelib.php line 1130]

          Does this mean the test passes or fails?

          Show
          Tim Barker added a comment - Exception: lib/simpletest/testmoodlelib.php / ► moodlelib_test / ► test_fix_utf8 Unexpected PHP error [iconv(): Detected an illegal character in input string] severity [E_NOTICE] in [/home/tim/webs/moodle_2.2/lib/moodlelib.php line 1130] Does this mean the test passes or fails?
          Hide
          Tim Barker added a comment -

          Sorry, I mean does the unit test failing mean that the issue retest has passed or failed?

          Show
          Tim Barker added a comment - Sorry, I mean does the unit test failing mean that the issue retest has passed or failed?
          Hide
          Petr Škoda added a comment -

          test_fix_utf8() would is a different issue.

          Show
          Petr Škoda added a comment - test_fix_utf8() would is a different issue.
          Hide
          Tim Barker added a comment - - edited

          May I pass this then as there were no other problems when running the test?

          Show
          Tim Barker added a comment - - edited May I pass this then as there were no other problems when running the test?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the records, I get, under master:

          1/1 test cases complete: 499 passes, 0 fails and 0 exceptions.

          and, under 22_STABLE:

          1/1 test cases complete: 467 passes, 0 fails and 0 exceptions.

          for lib/simpletest/testmoodlelib.php

          which OS/PHP are you getting that iconv() problem, Tim?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the records, I get, under master: 1/1 test cases complete: 499 passes, 0 fails and 0 exceptions. and, under 22_STABLE: 1/1 test cases complete: 467 passes, 0 fails and 0 exceptions. for lib/simpletest/testmoodlelib.php which OS/PHP are you getting that iconv() problem, Tim? Ciao
          Hide
          Tim Barker added a comment -

          Hey Eloy, hows things?

          OS: Ubuntu 10.4 LTS
          PHP: Do you mean what version on the webserver? I did php -version and it tells me that php is not installed so it's whatever apache2 uses as default.

          There were no other errors with tests in lib/simpletest/testmoodlelib.php relating to UTF. U think I may have found a corner case?

          Show
          Tim Barker added a comment - Hey Eloy, hows things? OS: Ubuntu 10.4 LTS PHP: Do you mean what version on the webserver? I did php -version and it tells me that php is not installed so it's whatever apache2 uses as default. There were no other errors with tests in lib/simpletest/testmoodlelib.php relating to UTF. U think I may have found a corner case?
          Hide
          Tim Barker added a comment -

          Installed PHP 5.3.2-1ubuntu4.14 with Suhosin-Patch and it still did the same.

          Show
          Tim Barker added a comment - Installed PHP 5.3.2-1ubuntu4.14 with Suhosin-Patch and it still did the same.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, iconv uses to have its own particularities, and depending of the exact libiconv used, has different bugs here and there. That has happened various times in the past.

          So surely it would be worth looking if your ubuntu has any update to the (lib)iconv available or, alternatively, any update to the php (because some times the iconv used by php is not the OS one but its one used on compile time).

          I don't know the details of ubuntu, but I bet this is one of those cases of "buggy" iconv being used (@ OS or @ PHP). Moving forward (updating) uses to help.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, iconv uses to have its own particularities, and depending of the exact libiconv used, has different bugs here and there. That has happened various times in the past. So surely it would be worth looking if your ubuntu has any update to the (lib)iconv available or, alternatively, any update to the php (because some times the iconv used by php is not the OS one but its one used on compile time). I don't know the details of ubuntu, but I bet this is one of those cases of "buggy" iconv being used (@ OS or @ PHP). Moving forward (updating) uses to help. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm PHP 5.3.8 with libiconv 1.14, for the records.

          Show
          Eloy Lafuente (stronk7) added a comment - I'm PHP 5.3.8 with libiconv 1.14, for the records.
          Hide
          Michael de Raadt added a comment -

          I was able to run this script without any errors appearing.

          Show
          Michael de Raadt added a comment - I was able to run this script without any errors appearing.
          Hide
          Sam Hemelryk added a comment -

          I am running Ubuntu 10.10 + PHP 5.3.6 + Iconv 2.13 and I'm seeing the same error as Tim when running the the unit tests.
          However I can confirm I get that same error BEFORE these changes, so as its got no worse and presumably better I am passing this now.

          Show
          Sam Hemelryk added a comment - I am running Ubuntu 10.10 + PHP 5.3.6 + Iconv 2.13 and I'm seeing the same error as Tim when running the the unit tests. However I can confirm I get that same error BEFORE these changes, so as its got no worse and presumably better I am passing this now.
          Hide
          Aparup Banerjee added a comment -

          ah thanks Sam!

          Show
          Aparup Banerjee added a comment - ah thanks Sam!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          FCT (fixed, closing, thanks). Ciao

          "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
          ~ Benjamin Disraeli

          Show
          Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

            People

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

              Dates

              • Created:
                Updated:
                Resolved: