Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a multiple-choice question where one of the choices ends in a paragraph that is empty except for one non-breaking space.

      2. Preview the question, and verify that it looks OK.

      Show
      1. Create a multiple-choice question where one of the choices ends in a paragraph that is empty except for one non-breaking space. 2. Preview the question, and verify that it looks OK.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      46048

      Description

      In the course of QA testing of quiz and questions in moodle 2.4 I have encountered this weird display in MCQ questions. See attached screenshot.
      Moodle 2.4beta (Build: 20121109)

        Issue Links

          Activity

          Hide
          Joseph Rézeau added a comment -

          incorrect answers are followed on the next line by a Black Diamond with Question Mark

          Show
          Joseph Rézeau added a comment - incorrect answers are followed on the next line by a Black Diamond with Question Mark
          Hide
          Tim Hunt added a comment -

          I just recreated that test question, and it worked.

          Where did your test question come from Joseph?

          Show
          Tim Hunt added a comment - I just recreated that test question, and it worked. Where did your test question come from Joseph?
          Hide
          Joseph Rézeau added a comment -

          Ah, got it! It so happens that I had accidentally typed an empty new paragraph after the text of some of my answers.
          When the question is displayed, In the Choice / Answer field, paragraph tags are changed into line breaks <br>, but empty paragraphs are not discarded, resulting in the weird Black Diamond with Question Mark symbol!

          Suggested fix: in question/type/multichoice/question.php add a line to remove potential empty paragraphs (containing a non-breaking space)
          Note: type the non-breaking space with its ascii code 0160.

          function public function make_html_inline($html) {
                  $html = preg_replace('/\<p\> \<\/p\>/', '', $html);
                  $html = preg_replace('~\s*<p>\s*~', '', $html);
                  $html = preg_replace('~\s*</p>\s*~', '<br />', $html);
                  $html = preg_replace('~<br />$~', '', $html);
                  return $html;
              }
          
          Show
          Joseph Rézeau added a comment - Ah, got it! It so happens that I had accidentally typed an empty new paragraph after the text of some of my answers. When the question is displayed, In the Choice / Answer field, paragraph tags are changed into line breaks <br>, but empty paragraphs are not discarded, resulting in the weird Black Diamond with Question Mark symbol! Suggested fix: in question/type/multichoice/question.php add a line to remove potential empty paragraphs (containing a non-breaking space) Note: type the non-breaking space with its ascii code 0160. function public function make_html_inline($html) { $html = preg_replace('/\<p\> \<\/p\>/', '', $html); $html = preg_replace('~\s*<p>\s*~', '', $html); $html = preg_replace('~\s*</p>\s*~', '<br />', $html); $html = preg_replace('~<br />$~', '', $html); return $html; }
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Joseph,
          The black diamond mark is the sign of an invalid UTF-8 character (but non breaking space #0160 is a VALID UTF-8 character !!)
          Now that you learned me how to type a non breaking space in MDL-36683 I am able to reproduce the problem both in the 23_STABLE branch and in master;
          Clearly the non breaking space character is correctly saved in the database (if you try to re-edit the question everything is OK and you can look in the database to verify there is no problem)
          So my best suggestion is that some code in the question renderer is breaking the correct UTF-8 string in some invalid one ?
          Here also I don't think the way to go is to suppress empty paragraph (after all question creators may want empty paragraph !) we need to find what break the valid UTF-8 character into an invalid one.

          Show
          Jean-Michel Vedrine added a comment - Hello Joseph, The black diamond mark is the sign of an invalid UTF-8 character (but non breaking space #0160 is a VALID UTF-8 character !!) Now that you learned me how to type a non breaking space in MDL-36683 I am able to reproduce the problem both in the 23_STABLE branch and in master; Clearly the non breaking space character is correctly saved in the database (if you try to re-edit the question everything is OK and you can look in the database to verify there is no problem) So my best suggestion is that some code in the question renderer is breaking the correct UTF-8 string in some invalid one ? Here also I don't think the way to go is to suppress empty paragraph (after all question creators may want empty paragraph !) we need to find what break the valid UTF-8 character into an invalid one.
          Hide
          Joseph Rézeau added a comment -

          Upon further exploration of this - fascinating - problem I came upon the final answer on this webpage:
          http://alexander.kirk.at/2011/10/01/preg_match-utf-8-and-whitespace/

          It so happens that the non-breaking space can be encoded with 2 bytes: 0xC2 0xA0.
          source http://en.wikipedia.org/wiki/Non-breaking_space#Encodings

          In function make_html_inline($html) Tim uses the \s regular expressions shorthand character to remove whitespaces (spaces, tabs, and line breaks) from the $html string to be analysed. Unfortunately, this breaks the potential non-breaking spaces into non-UTF-8 characters, as explained above.

          Let's hope a satisfactory solution will be found eventually.

          Show
          Joseph Rézeau added a comment - Upon further exploration of this - fascinating - problem I came upon the final answer on this webpage: http://alexander.kirk.at/2011/10/01/preg_match-utf-8-and-whitespace/ It so happens that the non-breaking space can be encoded with 2 bytes: 0xC2 0xA0. source http://en.wikipedia.org/wiki/Non-breaking_space#Encodings In function make_html_inline($html) Tim uses the \s regular expressions shorthand character to remove whitespaces (spaces, tabs, and line breaks) from the $html string to be analysed. Unfortunately, this breaks the potential non-breaking spaces into non-UTF-8 characters, as explained above. Let's hope a satisfactory solution will be found eventually.
          Hide
          Jean-Michel Vedrine added a comment -

          Joseph,
          it seems to me you not only found the problem but also the solution
          The first link in your post mention the u modifier
          As php help says :
          This modifier turns on additional functionality of PCRE that is incompatible with Perl. Pattern strings are treated as UTF-8. This modifier is available from PHP 4.1.0 or greater on Unix and from PHP 4.2.3 on win32. UTF-8 validity of the pattern is checked since PHP 4.3.5.
          And also
          If the subject contains utf-8 sequences the 'u' modifier should be set, otherwise a pattern such as /./ could match a utf-8 sequence as two to four individual ASCII characters. It is not a requirement, however, as you may have a need to break apart utf-8 sequences into single bytes. Most of the time, though, if you're working with utf-8 strings you should use the 'u' modifier.
          So I suggest to add the u modifier to preg_replace in make_html_inline function
          Maybe it would also be a good thing to look for other preg_replace uses in quiz and question code with the same problem ?

          Show
          Jean-Michel Vedrine added a comment - Joseph, it seems to me you not only found the problem but also the solution The first link in your post mention the u modifier As php help says : This modifier turns on additional functionality of PCRE that is incompatible with Perl. Pattern strings are treated as UTF-8. This modifier is available from PHP 4.1.0 or greater on Unix and from PHP 4.2.3 on win32. UTF-8 validity of the pattern is checked since PHP 4.3.5. And also If the subject contains utf-8 sequences the 'u' modifier should be set, otherwise a pattern such as /./ could match a utf-8 sequence as two to four individual ASCII characters. It is not a requirement, however, as you may have a need to break apart utf-8 sequences into single bytes. Most of the time, though, if you're working with utf-8 strings you should use the 'u' modifier. So I suggest to add the u modifier to preg_replace in make_html_inline function Maybe it would also be a good thing to look for other preg_replace uses in quiz and question code with the same problem ?
          Hide
          Jean-Michel Vedrine added a comment -

          Comments in the php help page also clearly show that the help is misleading when it says "pattern string is treated as UTF-8" : the u modifier should not only be used when the pattern contains UTF-8 characters but also when the subject is an UTF-8 string and the pattern contains bytes that can be part of an UTF-8 character (exactly our case here !)

          Show
          Jean-Michel Vedrine added a comment - Comments in the php help page also clearly show that the help is misleading when it says "pattern string is treated as UTF-8" : the u modifier should not only be used when the pattern contains UTF-8 characters but also when the subject is an UTF-8 string and the pattern contains bytes that can be part of an UTF-8 character (exactly our case here !)
          Hide
          Jean-Michel Vedrine added a comment - - edited

          If I add the u modifier to the 3 preg_replace calls in make_html_inline (not sure it's really usefull to add it everywhere ...)

          • the problem of the weird display disappears
          • the empty paragraph is replaced by a <br /> tag by make_html_inline and the non breaking space is suppressed
          • the phpunit tests are still ok (including test_make_html_inline)
          Show
          Jean-Michel Vedrine added a comment - - edited If I add the u modifier to the 3 preg_replace calls in make_html_inline (not sure it's really usefull to add it everywhere ...) the problem of the weird display disappears the empty paragraph is replaced by a <br /> tag by make_html_inline and the non breaking space is suppressed the phpunit tests are still ok (including test_make_html_inline)
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Well obviously there is no need to add the u modifier to the line

           $html = preg_replace('~<br />$~', '', $html);

          But maybe we should suppress no only a single <br /> tag at the end but all <br /> tags at the end ?

          For instance, the first 2 preg_replace change
          <p>Frog</p><p>non breaking space</p>
          to
          Frog<br /><br />
          And the third line produce
          Frog<br />
          I think the remaining <br /> should be suppressed also because I don't see any valid use of a line break at the end of an answer, but maybe I am missing something ?

          Show
          Jean-Michel Vedrine added a comment - - edited Well obviously there is no need to add the u modifier to the line $html = preg_replace('~<br />$~', '', $html); But maybe we should suppress no only a single <br /> tag at the end but all <br /> tags at the end ? For instance, the first 2 preg_replace change <p>Frog</p><p>non breaking space</p> to Frog<br /><br /> And the third line produce Frog<br /> I think the remaining <br /> should be suppressed also because I don't see any valid use of a line break at the end of an answer, but maybe I am missing something ?
          Hide
          Tim Hunt added a comment -

          Thanks for working this out guys.

          Could one of you test this patch, and verify that it works for you too.

          Show
          Tim Hunt added a comment - Thanks for working this out guys. Could one of you test this patch, and verify that it works for you too.
          Hide
          Tim Hunt added a comment -

          Also, if you could export a sample question as Moodle XML, and attach it, that might make the testing instructions easier. Thanks.

          Show
          Tim Hunt added a comment - Also, if you could export a sample question as Moodle XML, and attach it, that might make the testing instructions easier. Thanks.
          Hide
          Joseph Rézeau added a comment -

          attached mcq question with empty paragraphs for testing purposes, as requested by Tim

          Show
          Joseph Rézeau added a comment - attached mcq question with empty paragraphs for testing purposes, as requested by Tim
          Hide
          Joseph Rézeau added a comment -

          @Tim, just tested your patch MDL-36571 and it works OK for me. Thanks!

          Show
          Joseph Rézeau added a comment - @Tim, just tested your patch MDL-36571 and it works OK for me. Thanks!
          Hide
          Tim Hunt added a comment -

          Thanks Joseph.

          Show
          Tim Hunt added a comment - Thanks Joseph.
          Hide
          Dan Poltawski added a comment -

          Integrated to 22, 23 and master.

          Thanks everyone.

          Show
          Dan Poltawski added a comment - Integrated to 22, 23 and master. Thanks everyone.
          Hide
          David Monllaó added a comment -

          It passes. Tested in 22, 23 and master

          Show
          David Monllaó added a comment - It passes. Tested in 22, 23 and master
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

              • Created:
                Updated:
                Resolved: