Moodle
  1. Moodle
  2. MDL-37746

normalizer_normalize is screwing up shortanswer questions for some people

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Run the unit tests.

      2. If you can work out how to trigger the PHP bug, then test that by previewing a short-answer question, but that does not seem to be possible.

      Show
      1. Run the unit tests. 2. If you can work out how to trigger the PHP bug, then test that by previewing a short-answer question, but that does not seem to be possible.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The API is:

      Return Values: The normalized string or NULL if an error occurred.

      (See http://php.net/manual/en/normalizer.normalize.php and https://bugs.php.net/bug.php?id=64090)

      So, if something goes wrong, as it did in https://moodle.org/mod/forum/discuss.php?d=220722, then the input data gets eaten.

      The code needs to be changed to check for the NULL return value, and in that case output a debugging message, and the use the unmodified string.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Didier Raboud added a comment -

            I suspect a bug or installation problem in php-intl for that to happen.

            Still, that might be something that can be circumvented.

            What about something like that?

            From 248d08792d17de82bfa5fdcac61a128cb2f17119 Mon Sep 17 00:00:00 2001
            From: Didier Raboud <didier.raboud@liip.ch>
            Date: Tue, 29 Jan 2013 09:42:44 +0100
            Subject: [PATCH] MDL-37746 Circumvent the possibility for
             normalizer_normalize to return NULL in case of error
             
            ---
             question/type/shortanswer/question.php |    7 +++++--
             1 file changed, 5 insertions(+), 2 deletions(-)
             
            diff --git a/question/type/shortanswer/question.php b/question/type/shortanswer/question.php
            index d46f4e0..804f8b4 100644
            --- a/question/type/shortanswer/question.php
            +++ b/question/type/shortanswer/question.php
            @@ -103,8 +103,11 @@ class qtype_shortanswer_question extends question_graded_by_strategy
                     }
             
                     if (function_exists('normalizer_normalize')) {
            -            $regexp = normalizer_normalize($regexp, Normalizer::FORM_C);
            -            $string = normalizer_normalize($string, Normalizer::FORM_C);
            +            $regexp_norm = normalizer_normalize($regexp, Normalizer::FORM_C);
            +            $string_norm = normalizer_normalize($string, Normalizer::FORM_C);
            +
            +            $string = is_null($string_norm) ? $string : $string_norm;
            +            $regexp = is_null($regexp_norm) ? $regexp : $regexp_norm;
                     }
             
                     return preg_match($regexp, trim($string));
            -- 
            1.7.10.4
            

            Show
            Didier Raboud added a comment - I suspect a bug or installation problem in php-intl for that to happen. Still, that might be something that can be circumvented. What about something like that? From 248d08792d17de82bfa5fdcac61a128cb2f17119 Mon Sep 17 00:00:00 2001 From: Didier Raboud <didier.raboud@liip.ch> Date: Tue, 29 Jan 2013 09:42:44 +0100 Subject: [PATCH] MDL-37746 Circumvent the possibility for normalizer_normalize to return NULL in case of error   --- question/type/shortanswer/question.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)   diff --git a/question/type/shortanswer/question.php b/question/type/shortanswer/question.php index d46f4e0..804f8b4 100644 --- a/question/type/shortanswer/question.php +++ b/question/type/shortanswer/question.php @@ -103,8 +103,11 @@ class qtype_shortanswer_question extends question_graded_by_strategy }   if (function_exists('normalizer_normalize')) { - $regexp = normalizer_normalize($regexp, Normalizer::FORM_C); - $string = normalizer_normalize($string, Normalizer::FORM_C); + $regexp_norm = normalizer_normalize($regexp, Normalizer::FORM_C); + $string_norm = normalizer_normalize($string, Normalizer::FORM_C); + + $string = is_null($string_norm) ? $string : $string_norm; + $regexp = is_null($regexp_norm) ? $regexp : $regexp_norm; }   return preg_match($regexp, trim($string)); -- 1.7.10.4
            Hide
            Tim Hunt added a comment -

            Yes, that is the kind of thing I was planning to do, though I was actually planning to add a safe_normalize method to avoid code duplication.

            Show
            Tim Hunt added a comment - Yes, that is the kind of thing I was planning to do, though I was actually planning to add a safe_normalize method to avoid code duplication.
            Hide
            Jamie Pratt added a comment -

            I noticed the following in the comments on http://php.net/manual/en/normalizer.normalize.php

            I tested the php and indeed it does output false.

            But I think it is unlikely that the error reported is due to malformed utf-8 as they said it is every input string that is being marked wrong.

            The 'safe_normalize' method should probably do something sensible though if a false is returned.

            This method/function will return boolean false if $input is not a valid utf-8-string, e.g.

            <?php
            var_dump(Normalizer::normalize("\xFF"));
            // prints "bool(false)"
            ?>

            Show
            Jamie Pratt added a comment - I noticed the following in the comments on http://php.net/manual/en/normalizer.normalize.php I tested the php and indeed it does output false. But I think it is unlikely that the error reported is due to malformed utf-8 as they said it is every input string that is being marked wrong. The 'safe_normalize' method should probably do something sensible though if a false is returned. — This method/function will return boolean false if $input is not a valid utf-8-string, e.g. <?php var_dump(Normalizer::normalize("\xFF")); // prints "bool(false)" ?>
            Hide
            Tim Hunt added a comment -

            Oh, I see that is an even more crazy API than I thought. I have written my code. I am just waiting for the unit tests to run.

            Show
            Tim Hunt added a comment - Oh, I see that is an even more crazy API than I thought. I have written my code. I am just waiting for the unit tests to run.
            Hide
            Tim Hunt added a comment -

            I am still waiting for the tests, but here is my proposed fix: https://github.com/timhunt/moodle/compare/master...MDL-37746

            Show
            Tim Hunt added a comment - I am still waiting for the tests, but here is my proposed fix: https://github.com/timhunt/moodle/compare/master...MDL-37746
            Hide
            Tim Hunt added a comment -

            OK. Fix done, and submitted for integration.

            Show
            Tim Hunt added a comment - OK. Fix done, and submitted for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Dan Poltawski added a comment -

            Integrated to master, 24 and 23. Thanks Tim!

            Show
            Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Tim!
            Hide
            Andrew Davis added a comment -

            Unit tests are currently all passing. Passing.

            Show
            Andrew Davis added a comment - Unit tests are currently all passing. Passing.
            Hide
            Damyon Wiese added a comment -

            Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

            Regards, Damyon

            Show
            Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon
            Hide
            Tim Hunt added a comment -

            Ironically, my safe_normalise method was screwing up when passed '0'. See MDL-38542

            Show
            Tim Hunt added a comment - Ironically, my safe_normalise method was screwing up when passed '0'. See MDL-38542

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: