Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              jleyva 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
              jleyva 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
              stronk7 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
              stronk7 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
              jleyva 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
              jleyva 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
              stronk7 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
              stronk7 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
              timb 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
              timb 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
              timb Tim Barker added a comment -

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

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

              test_fix_utf8() would is a different issue.

              Show
              skodak Petr Skoda added a comment - test_fix_utf8() would is a different issue.
              Hide
              timb Tim Barker added a comment - - edited

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

              Show
              timb Tim Barker added a comment - - edited May I pass this then as there were no other problems when running the test?
              Hide
              stronk7 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
              stronk7 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
              timb 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
              timb 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
              timb Tim Barker added a comment -

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

              Show
              timb Tim Barker added a comment - Installed PHP 5.3.2-1ubuntu4.14 with Suhosin-Patch and it still did the same.
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

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

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

              Show
              salvetore Michael de Raadt added a comment - I was able to run this script without any errors appearing.
              Hide
              samhemelryk 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
              samhemelryk 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
              nebgor Aparup Banerjee added a comment -

              ah thanks Sam!

              Show
              nebgor Aparup Banerjee added a comment - ah thanks Sam!
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    14/May/12