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

          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