Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37746

normalizer_normalize is screwing up shortanswer questions for some people

    Details

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

          Attachments

            Issue Links

              Activity

              Hide
              odyx 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
              odyx 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
              timhunt 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
              timhunt 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
              jamiesensei 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
              jamiesensei 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt Tim Hunt added a comment -

              OK. Fix done, and submitted for integration.

              Show
              timhunt Tim Hunt added a comment - OK. Fix done, and submitted for integration.
              Hide
              stronk7 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
              stronk7 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
              poltawski Dan Poltawski added a comment -

              Integrated to master, 24 and 23. Thanks Tim!

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

              Unit tests are currently all passing. Passing.

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

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

              Regards, Damyon

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

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

              Show
              timhunt 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:
                    Fix Release Date:
                    11/Mar/13