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:
    • Rank:
      47465

      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.

        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: