Moodle
  1. Moodle
  2. MDL-42033

Files in subdirs not exported correctly by question XML format

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Create some questions with images in subfolders.
      An easy way to do that in recent Moodle versions is to use Marina's managefiles TinyMCE plugin.
      Export this question bank category using Moodle XML format.
      Import them back in another course.
      Verify no image is missing.
      Verify that the same XML file can be imported in a Moodle version before this patch and that apart broken images (to be expected) there is no other problem.
      Note: As I said in comments, I don't really know how people get images in subdirs (I don't suppose they use Marina's plugin ) but I can testify it happen most probably by restoring courses from an old Moodle version ?

      Show
      Create some questions with images in subfolders. An easy way to do that in recent Moodle versions is to use Marina's managefiles TinyMCE plugin. Export this question bank category using Moodle XML format. Import them back in another course. Verify no image is missing. Verify that the same XML file can be imported in a Moodle version before this patch and that apart broken images (to be expected) there is no other problem. Note: As I said in comments, I don't really know how people get images in subdirs (I don't suppose they use Marina's plugin ) but I can testify it happen most probably by restoring courses from an old Moodle version ?
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
    • Pull Master Branch:

      Description

      If a question contains some embedded file with a non empty path(tested on non legacy files but I will test for legacy files later), the resulting Moodle XML file is broken because the path is included in the text for instance

      <img src="@@PLUGINFILE@@/images/mcq3.jpg" />

      but not in the file tag

      <file name="mcq3.jpg" encoding="base64">/9j/4AAQ...
      

      So if you import this file back, you get missing images in questions.
      I think the problem is caused by this line : http://git.moodle.org/gw?p=moodle.git;a=blob;f=question/format/xml/format.php;h=1c2edafdd2e62ebb53bf397cc41ea26b741a2d12;hb=HEAD#l1091 but I am at work so I will submit a fix later.
      I am quite sure master branch is affected but is it not the only one because the website where I discovered the problem is running Moodle 2.4.5. I am not sure when these non legacy images with subdirs were created (this website started as Moodle 1.3 and was upgraded since) but obviously if it happen on one of my websites it can happen elsewhere so I think we must make a fix for all supported stable branches.

        Gliffy Diagrams

          Activity

          Hide
          Jean-Michel Vedrine added a comment - - edited

          Well it was a little more difficult than anticipated to write a fix because XML import of files must be modified too as the "/" path was hardcoded.
          I think my solution preserve compatibility: an XML file created after the fix can still be imported in a non pached Moodle website and the result is as it was before (including of course images in subdirs being broken in that case ).
          I will write detailed testing instructions to test this, and also import of "old" XML files without any path in a patched Moodle.
          To summarize after the patch the only cases of questions with broken images should be:

          • import questions with images in subdirs from a file created before the patch (the file is broken)
          • import questions with images in subdirs from a file created after the patch in an unpatched Moodle website (people need to upgrade to correct the problem)

          Of course people may object that it should be possible to "repair" the broken file in #1 as the path is included in the text, but I don't think so:
          Imagine a question text with 2 images having the same name "myimage.jpg", one is in a "/myimages/" subdir and the other isn't.
          Prior to the fix, such a text was exported as

          bla bla <img src="@@PLUGINFILE@@/myimages/myimage.jpg" /> bla bla <img src="@@PLUGINFILE@@/myimage.jpg" />
          <file name="myimage.jpg" encoding="base64">...
          <file name="myimage.jpg" encoding="base64">...
          

          And it is quite difficult to repair it automatically !
          So I suggest we don't care too much about this problem.
          After studying the problem images in subdirs occurs :

          • when you restore 1.9 backups or upgrade a website from Moodle 1.9 (this was in fact the case on the website where I discovered the problem)
          • when you use Marina' "manage embedded files" tinyMCE and "embedded files" repository plugins, but these plugins will only be standard in Moodle 2.6 so I think only a few Moodle users have them installed.
          Show
          Jean-Michel Vedrine added a comment - - edited Well it was a little more difficult than anticipated to write a fix because XML import of files must be modified too as the "/" path was hardcoded. I think my solution preserve compatibility: an XML file created after the fix can still be imported in a non pached Moodle website and the result is as it was before (including of course images in subdirs being broken in that case ). I will write detailed testing instructions to test this, and also import of "old" XML files without any path in a patched Moodle. To summarize after the patch the only cases of questions with broken images should be: import questions with images in subdirs from a file created before the patch (the file is broken) import questions with images in subdirs from a file created after the patch in an unpatched Moodle website (people need to upgrade to correct the problem) Of course people may object that it should be possible to "repair" the broken file in #1 as the path is included in the text, but I don't think so: Imagine a question text with 2 images having the same name "myimage.jpg", one is in a "/myimages/" subdir and the other isn't. Prior to the fix, such a text was exported as bla bla <img src="@@PLUGINFILE@@/myimages/myimage.jpg" /> bla bla <img src="@@PLUGINFILE@@/myimage.jpg" /> <file name="myimage.jpg" encoding="base64">... <file name="myimage.jpg" encoding="base64">... And it is quite difficult to repair it automatically ! So I suggest we don't care too much about this problem. After studying the problem images in subdirs occurs : when you restore 1.9 backups or upgrade a website from Moodle 1.9 (this was in fact the case on the website where I discovered the problem) when you use Marina' "manage embedded files" tinyMCE and "embedded files" repository plugins, but these plugins will only be standard in Moodle 2.6 so I think only a few Moodle users have them installed.
          Hide
          Jean-Michel Vedrine added a comment -

          hum,
          when I try to attach a file to this issue I get:
          Cannot attach file sample1.xml: Exception trying to establish attachment directory. Check that the application server and JIRA have permissions to write to it: com.atlassian.jira.web.util.AttachmentException: Cannot write to attachment directory. Check that the application server and JIRA have permissions to write to: /opt/jirahome/data/attachments/MDL/MDL-42033
          Maybe I should report the problem to MDLSITE ?

          Show
          Jean-Michel Vedrine added a comment - hum, when I try to attach a file to this issue I get: Cannot attach file sample1.xml: Exception trying to establish attachment directory. Check that the application server and JIRA have permissions to write to it: com.atlassian.jira.web.util.AttachmentException: Cannot write to attachment directory. Check that the application server and JIRA have permissions to write to: /opt/jirahome/data/attachments/MDL/ MDL-42033 Maybe I should report the problem to MDLSITE ?
          Hide
          Jean-Michel Vedrine added a comment -

          Anyway, even if I can't upload my sample file, here is my problem: my code seems to work for all questions fields except question text and general feedback !
          Of course we both know these fields are managed in a special way but I fail to understand what prevent images in subdirs to be imported in these 2 fields : I can verify the draft file is successfully created but later the file is missing in the question text or general feedback.
          To be continued ...

          Show
          Jean-Michel Vedrine added a comment - Anyway, even if I can't upload my sample file, here is my problem: my code seems to work for all questions fields except question text and general feedback ! Of course we both know these fields are managed in a special way but I fail to understand what prevent images in subdirs to be imported in these 2 fields : I can verify the draft file is successfully created but later the file is missing in the question text or general feedback. To be continued ...
          Hide
          Tim Hunt added a comment -

          I just reviewed the diff, and it looks good to me.

          Do we have any unit tests for the file import code?

          Show
          Tim Hunt added a comment - I just reviewed the diff, and it looks good to me. Do we have any unit tests for the file import code?
          Hide
          Jean-Michel Vedrine added a comment -

          Bingo,
          There is 'subdirs' => false, in question/format.php line 390 problem solved I think

          Show
          Jean-Michel Vedrine added a comment - Bingo, There is 'subdirs' => false, in question/format.php line 390 problem solved I think
          Hide
          Jean-Michel Vedrine added a comment -

          No I don't think there is any unit test for file import maybe this is a good occasion to add some ? I seem to remember we discussed how to do it but I don't remember your advices.

          Show
          Jean-Michel Vedrine added a comment - No I don't think there is any unit test for file import maybe this is a good occasion to add some ? I seem to remember we discussed how to do it but I don't remember your advices.
          Hide
          Tim Hunt added a comment -

          I guess there are two approaches to writing unit tests with files. 1. Just check what has been put into the draft file areas; or 2. Actually do a full import, including whatever it is calls save_question, and then check what is in the real file area.

          2. is probably a better test.

          In either case you need to subclass advanced_testcase. One example test using the database is test_question_saving_true in question/type/truefalse/tests/questiontype_test.php.

          Show
          Tim Hunt added a comment - I guess there are two approaches to writing unit tests with files. 1. Just check what has been put into the draft file areas; or 2. Actually do a full import, including whatever it is calls save_question, and then check what is in the real file area. 2. is probably a better test. In either case you need to subclass advanced_testcase. One example test using the database is test_question_saving_true in question/type/truefalse/tests/questiontype_test.php.
          Hide
          Jean-Michel Vedrine added a comment -

          In fact I was wrong, there is already a unit test: test_import_files_as_draft in question/format/xml/tests/xmlformat_test.php and it uses approach #1

          Show
          Jean-Michel Vedrine added a comment - In fact I was wrong, there is already a unit test: test_import_files_as_draft in question/format/xml/tests/xmlformat_test.php and it uses approach #1
          Hide
          Jean-Michel Vedrine added a comment -

          I wonder if approach #2 is easy given that importprocess doesn't (unfortunately ) uses save_question ?
          Surely the test can't mimic importprocess without duplicating a lot of it's code, unless your idea wasn't to mimic importprocess at all but rather what is done when a question is saved after editing or creation ? But that would of course not be a real test of the import process.

          Show
          Jean-Michel Vedrine added a comment - I wonder if approach #2 is easy given that importprocess doesn't (unfortunately ) uses save_question ? Surely the test can't mimic importprocess without duplicating a lot of it's code, unless your idea wasn't to mimic importprocess at all but rather what is done when a question is saved after editing or creation ? But that would of course not be a real test of the import process.
          Hide
          Tim Hunt added a comment -

          Then, continue with approach 1, I think.

          Show
          Tim Hunt added a comment - Then, continue with approach 1, I think.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I have added one small phpunit test following approach 1. Do you think it will be enough to submit this to integration ?
          This is an useful little change that I think will make people restoring courses originated bach from Moodle 1.9 having a lot less missing images.
          It is backward compatible: XML export files made after this change can be imported in previous Moodle versions with no problem (but of course the missing images problem will not be corrected in previous Moodle versions, the situation will be unchanged).

          Show
          Jean-Michel Vedrine added a comment - - edited I have added one small phpunit test following approach 1. Do you think it will be enough to submit this to integration ? This is an useful little change that I think will make people restoring courses originated bach from Moodle 1.9 having a lot less missing images. It is backward compatible: XML export files made after this change can be imported in previous Moodle versions with no problem (but of course the missing images problem will not be corrected in previous Moodle versions, the situation will be unchanged).
          Hide
          Jean-Michel Vedrine added a comment -

          I will backport to stable branchs once we agree this is good for integration.

          Show
          Jean-Michel Vedrine added a comment - I will backport to stable branchs once we agree this is good for integration.
          Hide
          Jean-Michel Vedrine added a comment -

          Can you peer review this Tim ?

          Show
          Jean-Michel Vedrine added a comment - Can you peer review this Tim ?
          Hide
          CiBoT added a comment -

          Results for MDL-42033

          • Remote repository: git://github.com/jmvedrine/moodle.git
          Show
          CiBoT added a comment - Results for MDL-42033 Remote repository: git://github.com/jmvedrine/moodle.git Remote branch MDL-42033 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1391 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1391/artifact/work/smurf.html
          Hide
          Tim Hunt added a comment -

          +1 this looks good. Please rebase down to a single commit, and backport.

          Show
          Tim Hunt added a comment - +1 this looks good. Please rebase down to a single commit, and backport.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Done, but I think this will have to wait next week, the integration queue is already overcrowded this week ! Integrators have all my sympathy.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Done, but I think this will have to wait next week, the integration queue is already overcrowded this week ! Integrators have all my sympathy.
          Hide
          Tim Hunt added a comment -

          Well, I am going to throw it on the integration pile, and let them decide when they want to integrate it. As lease we are sending them all this code now, and not just before the 2.7 code-freeze.

          Show
          Tim Hunt added a comment - Well, I am going to throw it on the integration pile, and let them decide when they want to integrate it. As lease we are sending them all this code now, and not just before the 2.7 code-freeze.
          Hide
          Jean-Michel Vedrine added a comment -

          Yes that's a lot better

          Show
          Jean-Michel Vedrine added a comment - Yes that's a lot better
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 26 and 25 - thanks Jean-Michel!

          (Thanks for considering our workload! But yep, just keep the patches coming at us, thanks!)

          Show
          Dan Poltawski added a comment - Integrated to master, 26 and 25 - thanks Jean-Michel! (Thanks for considering our workload! But yep, just keep the patches coming at us, thanks!)
          Hide
          Andrew Nicols added a comment -

          Thanks Jean-Michel,

          This all works as expected.

          I couldn't find a way of creating files in subfolders for 2.5 either, so I ended up restoring my 2.6 export into 2.5 and then running the tests based on that.

          Passing Thanks all.

          Show
          Andrew Nicols added a comment - Thanks Jean-Michel, This all works as expected. I couldn't find a way of creating files in subfolders for 2.5 either, so I ended up restoring my 2.6 export into 2.5 and then running the tests based on that. Passing Thanks all.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I claim to be a simple individual
          liable to err like any other fellow mortal.
          I own, however, that I have humility enough
          to confess my errors and to retrace my steps.

          Mahatma Gandhi

          Your awesome code has met upstream, closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: