Moodle
  1. Moodle
  2. MDL-31244

Problem with images in question bank and quiz if algebra filter is enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Questions, Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Enable the algebra filter.

      2. Type in some maths some where (e.g. a forum post). Suitable maths might be @@1 + 1 = 2@@.

      3. Also copy in paste in a diff inside <pre> tags (not that the pre tags are really important). E.g. the one below http://tracker.moodle.org/browse/MDL-31244?focusedCommentId=174752&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-174752

      4. Submit the post, and ensure the maths is displayed as maths, but the diff is not messed up.

      Show
      1. Enable the algebra filter. 2. Type in some maths some where (e.g. a forum post). Suitable maths might be @@1 + 1 = 2@@. 3. Also copy in paste in a diff inside <pre> tags (not that the pre tags are really important). E.g. the one below http://tracker.moodle.org/browse/MDL-31244?focusedCommentId=174752&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-174752 4. Submit the post, and ensure the maths is displayed as maths, but the diff is not messed up.
    • Workaround:
      Hide

      deactivate algebra filter (why are people still using this now that we have so many wonderfull tools for math is beyond my understanding !)

      Show
      deactivate algebra filter (why are people still using this now that we have so many wonderfull tools for math is beyond my understanding !)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      37704

      Description

      See discussion here : http://moodle.org/mod/forum/discuss.php?d=192615
      The problem is that on several pages, text filters are called with texts containing undecoded images urls, so if the algebra filter is activated the algebra2tex.pl script is called with wrong data, that seems to result in a 500 server error.
      urls should be decoded before calling text filters.
      the problem seems to affect at least
      question/preview.php in all cases if for instance the question text contain an image or for a MC question if any answer contain an image
      mod/quiz/edit.php if the question bank content is displayed and at least a question text contain an image.
      But as my tests weren't extensive ones maybe other pages are also affected. In the forum discussion linked above Robert Fransen report that mod/quiz/startattempt.php can exhibit the same problem

      Of course I have listed Quiz in the Component field because it is affected but this is a Questions bug.

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          I have set this issue as minor because i think very few people are seeing it, but of course for those people it is a major issue because it render the quiz module useless as soon as they want to use images.

          Show
          Jean-Michel Vedrine added a comment - I have set this issue as minor because i think very few people are seeing it, but of course for those people it is a major issue because it render the quiz module useless as soon as they want to use images.
          Hide
          Tim Hunt added a comment -

          Thanks. I will investigate.

          Show
          Tim Hunt added a comment - Thanks. I will investigate.
          Hide
          Jason Hando added a comment -

          I also have just had this issue arise on a Windows server 2008 R2 running IIS, PHP 5.3.8, MySQL 5.0.8. Disabled Algebra filter and now we can edit quiz again. Checked URL for image in question and still undecoded ie. draftfile URL eg. img src="http://moodlesite.com/draftfile.php/22274/user/draft/186767578/image.jpg". I understand that URL should change after question is saved. Need to resolve why URL in question is not being changed ie. as per step 6 in the documentation: http://docs.moodle.org/dev/Using_the_File_API_in_Moodle_forms.

          Show
          Jason Hando added a comment - I also have just had this issue arise on a Windows server 2008 R2 running IIS, PHP 5.3.8, MySQL 5.0.8. Disabled Algebra filter and now we can edit quiz again. Checked URL for image in question and still undecoded ie. draftfile URL eg. img src="http://moodlesite.com/draftfile.php/22274/user/draft/186767578/image.jpg". I understand that URL should change after question is saved. Need to resolve why URL in question is not being changed ie. as per step 6 in the documentation: http://docs.moodle.org/dev/Using_the_File_API_in_Moodle_forms .
          Hide
          Jean-Michel Vedrine added a comment -

          I can testify that the problem is not limited to IIS (so this is not a problem with an IIS option) and that it happend not only with url begining with DRAFTFILE but with PLUGINFILE too

          Show
          Jean-Michel Vedrine added a comment - I can testify that the problem is not limited to IIS (so this is not a problem with an IIS option) and that it happend not only with url begining with DRAFTFILE but with PLUGINFILE too
          Hide
          Iñigo Zendegi added a comment -

          We are having this issue in Moodle 2.1.6 too, in Win2k3 running Apache, PHP 5.4.3 and Oracle 10.2

          We've disabled the algebra filter meanwhile, but anyway, I think this is not a minor issue as it provokes the quiz been unusable.

          Show
          Iñigo Zendegi added a comment - We are having this issue in Moodle 2.1.6 too, in Win2k3 running Apache, PHP 5.4.3 and Oracle 10.2 We've disabled the algebra filter meanwhile, but anyway, I think this is not a minor issue as it provokes the quiz been unusable.
          Hide
          Jean-Michel Vedrine added a comment -

          What really puzzle me is that this issue has only 1 vote and 3 watchers (and this somewhat justify the minor priority) because I would bet that as soon as the algebra filter is activated every Moodle 2.1/2.2/2.3 installation exhibit this problem be it windows or unix, IIS or Apache, ...
          Maybe it's a clear sign that the algebra filter is not widely used ?

          Show
          Jean-Michel Vedrine added a comment - What really puzzle me is that this issue has only 1 vote and 3 watchers (and this somewhat justify the minor priority) because I would bet that as soon as the algebra filter is activated every Moodle 2.1/2.2/2.3 installation exhibit this problem be it windows or unix, IIS or Apache, ... Maybe it's a clear sign that the algebra filter is not widely used ?
          Hide
          Iñigo Zendegi added a comment -

          I agree with you Jean-Michel, maybe the impact of this issue is not so high because there is not many people using the algebra filter, but I think that's the voting system for.

          The priority of an issue, IMHO, should be measured by the symptoms it provokes, and in this case the quiz doesn't load any more while the algebra filter is enabled, so I won't say this is a critical issue, but it shouldn't be a minor issue either.

          Show
          Iñigo Zendegi added a comment - I agree with you Jean-Michel, maybe the impact of this issue is not so high because there is not many people using the algebra filter, but I think that's the voting system for. The priority of an issue, IMHO, should be measured by the symptoms it provokes, and in this case the quiz doesn't load any more while the algebra filter is enabled, so I won't say this is a critical issue, but it shouldn't be a minor issue either.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Iñigo,
          I agree with you : for people affected by this problem it isn't a minor issue, and the worste is that there is no error message so people can't guess that deactivating the algebra filter will make their Moodle working again !
          But I have done some research and I still don't understand why the filter is called with undecoded urls containing @@PLUGINFILE@@ or @@DRAFTFILE@@. I am quite sure it does occur because I have beeen able to verify it, but I still don't understand why.

          Show
          Jean-Michel Vedrine added a comment - Hello Iñigo, I agree with you : for people affected by this problem it isn't a minor issue, and the worste is that there is no error message so people can't guess that deactivating the algebra filter will make their Moodle working again ! But I have done some research and I still don't understand why the filter is called with undecoded urls containing @@PLUGINFILE@@ or @@DRAFTFILE@@. I am quite sure it does occur because I have beeen able to verify it, but I still don't understand why.
          Hide
          Tim Hunt added a comment -

          Ah good Jean-Michel has moved on from arguing about what the priority should be, to trying to work out the cause of the bug and the fix. I will try to look at this once I have fixed all the things that are critical for the 2.3 release.

          Show
          Tim Hunt added a comment - Ah good Jean-Michel has moved on from arguing about what the priority should be, to trying to work out the cause of the bug and the fix. I will try to look at this once I have fixed all the things that are critical for the 2.3 release.
          Hide
          Frédéric Massart added a comment -

          I'm experiencing this issue with workshop module for the submissions.

          Show
          Frédéric Massart added a comment - I'm experiencing this issue with workshop module for the submissions.
          Hide
          Tim Hunt added a comment -

          I have a proposed fix, but I am failing to get the unit tests running on my laptop here, so I can't finish it. Note that, at the moment, one of the unit tests will definitely fail.

          Leaving aside the details, any comments on the general approach I have taken would be welcome.

          Show
          Tim Hunt added a comment - I have a proposed fix, but I am failing to get the unit tests running on my laptop here, so I can't finish it. Note that, at the moment, one of the unit tests will definitely fail. Leaving aside the details, any comments on the general approach I have taken would be welcome.
          Hide
          Frédéric Massart added a comment -

          Hi Tim,

          I have had the PHP Unit tests failing here, the main reason being that the filter adds a white space at the end of the string. Also, the second test algebra_filter_simple will always fail. There was also an error because of the $texcache object not instantiated. I could fix those using the following patch:

          diff --git a/filter/algebra/filter.php b/filter/algebra/filter.php
          index 6beb20b..b49df64 100644
          --- a/filter/algebra/filter.php
          +++ b/filter/algebra/filter.php
          @@ -230,6 +230,7 @@ class filter_algebra extends moodle_text_filter {
                             $texexp = preg_replace('/\\\int\\\left\((.+?d[a-z])\\\right\)/s','\int '. "\$1 ",$texexp);
                             $texexp = preg_replace('/\\\lim\\\left\((.+?),(.+?),(.+?)\\\right\)/s','\lim_'. "{\$2\\to \$3}\$1 ",$texexp);
                             $texexp = str_replace('\mbox', '', $texexp); // now blacklisted in tex, sorry
          +                  $texcache = new stdClass();
                             $texcache->filter = 'algebra';
                             $texcache->version = 1;
                             $texcache->md5key = $md5;
          @@ -244,7 +245,7 @@ class filter_algebra extends moodle_text_filter {
                          $text = str_replace( $matches[0][$i], filter_algebra_image($filename, $texcache->rawtext), $text);
                       }
                   }
          -        return $text;
          +        return trim($text);
               }
           }
           
          diff --git a/filter/algebra/tests/filter_test.php b/filter/algebra/tests/filter_test.php
          index 5717ea6..e923fb9 100644
          --- a/filter/algebra/tests/filter_test.php
          +++ b/filter/algebra/tests/filter_test.php
          @@ -44,7 +44,7 @@ class filter_algebra_testcase extends basic_testcase {
               }
           
               function test_algebra_filter_simple() {
          -        $this->assertEquals('<p>Look no algebra!</p>',
          +        $this->assertNotEquals('<p>@@1 + 1 = 2@@.</p>',
                           $this->filter->filter('<p>@@1 + 1 = 2@@.</p>'));
               }
          

          Other than this the patch looks good. Cheers! \o/

          Show
          Frédéric Massart added a comment - Hi Tim, I have had the PHP Unit tests failing here, the main reason being that the filter adds a white space at the end of the string. Also, the second test algebra_filter_simple will always fail. There was also an error because of the $texcache object not instantiated. I could fix those using the following patch: diff --git a/filter/algebra/filter.php b/filter/algebra/filter.php index 6beb20b..b49df64 100644 --- a/filter/algebra/filter.php +++ b/filter/algebra/filter.php @@ -230,6 +230,7 @@ class filter_algebra extends moodle_text_filter { $texexp = preg_replace('/\\\int\\\left\((.+?d[a-z])\\\right\)/s','\int '. "\$1 ",$texexp); $texexp = preg_replace('/\\\lim\\\left\((.+?),(.+?),(.+?)\\\right\)/s','\lim_'. "{\$2\\to \$3}\$1 ",$texexp); $texexp = str_replace('\mbox', '', $texexp); // now blacklisted in tex, sorry + $texcache = new stdClass(); $texcache->filter = 'algebra'; $texcache->version = 1; $texcache->md5key = $md5; @@ -244,7 +245,7 @@ class filter_algebra extends moodle_text_filter { $text = str_replace( $matches[0][$i], filter_algebra_image($filename, $texcache->rawtext), $text); } } - return $text; + return trim($text); } } diff --git a/filter/algebra/tests/filter_test.php b/filter/algebra/tests/filter_test.php index 5717ea6..e923fb9 100644 --- a/filter/algebra/tests/filter_test.php +++ b/filter/algebra/tests/filter_test.php @@ -44,7 +44,7 @@ class filter_algebra_testcase extends basic_testcase { } function test_algebra_filter_simple() { - $this->assertEquals('<p>Look no algebra!</p>', + $this->assertNotEquals('<p>@@1 + 1 = 2@@.</p>', $this->filter->filter('<p>@@1 + 1 = 2@@.</p>')); } Other than this the patch looks good. Cheers! \o/
          Hide
          Jean-Michel Vedrine added a comment -

          Hello,
          I tested Tim's fix on a site with this problem and it seems to work well. Without the fix : server error and with the fix : pages displayed with no problem.
          During the tests I did on this issue months ago, I have also seen urls with @@DRAFTFILE@@ send undecoded to this filter (I used a modified version of the filter that logged all strings send to it before trying to process them). I don't really know where they were coming but I suggest to also add this test for safety.

          Show
          Jean-Michel Vedrine added a comment - Hello, I tested Tim's fix on a site with this problem and it seems to work well. Without the fix : server error and with the fix : pages displayed with no problem. During the tests I did on this issue months ago, I have also seen urls with @@DRAFTFILE@@ send undecoded to this filter (I used a modified version of the filter that logged all strings send to it before trying to process them). I don't really know where they were coming but I suggest to also add this test for safety.
          Hide
          Tim Hunt added a comment -

          Thanks for the speedy review Frédéric and Jean-Michel.

          Frédéric, you will note in the comment above that I knew the unit tests would fail because I was unable to run them here, but thanks for the patch. I think I will spend more time on it and do better than assertNotEquals.

          Jean-Michel, I searched the entire code for '@@' to see what strings I should consider, and was surprised, considering your previous comments, not to see @@DRAFTFILE@@ anywhere, which is why I did not add it, but perhaps I will now.

          Show
          Tim Hunt added a comment - Thanks for the speedy review Frédéric and Jean-Michel. Frédéric, you will note in the comment above that I knew the unit tests would fail because I was unable to run them here, but thanks for the patch. I think I will spend more time on it and do better than assertNotEquals. Jean-Michel, I searched the entire code for '@@' to see what strings I should consider, and was surprised, considering your previous comments, not to see @@DRAFTFILE@@ anywhere, which is why I did not add it, but perhaps I will now.
          Hide
          Tim Hunt added a comment -

          Submitting for integration review now.

          In the end, I did not make a unit test to demonstrate the filter working, because I could not make the filter work at all on my Windows development box. (It was weird, running the unit tests made windows pop up a dialogue box asking what program I wanted to use to open algebra2tex.pl!) Anyway, I don't want to inflict nonsense like that on other developers.

          Show
          Tim Hunt added a comment - Submitting for integration review now. In the end, I did not make a unit test to demonstrate the filter working, because I could not make the filter work at all on my Windows development box. (It was weird, running the unit tests made windows pop up a dialogue box asking what program I wanted to use to open algebra2tex.pl!) Anyway, I don't want to inflict nonsense like that on other developers.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I se you have added DRAFTFILE. Of course it can't harm, but if you are sure it's nowhere in the code maybe it is not necessary.
          I tried to find the logs I had done some months ago to verify if there where strings with @@DRAFTFILE@@ or not but I don't seem to have them on my laptop.
          Maybe somebody familiar with the file system working can tell us ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I se you have added DRAFTFILE. Of course it can't harm, but if you are sure it's nowhere in the code maybe it is not necessary. I tried to find the logs I had done some months ago to verify if there where strings with @@DRAFTFILE@@ or not but I don't seem to have them on my laptop. Maybe somebody familiar with the file system working can tell us ?
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          Sidenote: don't forget phpunit support was added in 2.3. I've added commit to 22_STABLE taking rid of such tests.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks! Sidenote: don't forget phpunit support was added in 2.3. I've added commit to 22_STABLE taking rid of such tests.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing, unit tests based.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing, unit tests based.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: