Moodle
  1. Moodle
  2. MDL-33424

Images missing when restoring a course (V1.9) or a course / quiz (V2)

    Details

    • Testing Instructions:
      Hide

      Testing difficulty: easy

      1. In Moodle 1.9, create some questions with embedded images in the question bank of a course. To test all areas modified by this patch, you need to include
        • at least one image in question text of one question (any question type will do the job)
        • at least an image in the old "Image" filed that is now deprecated in Moodle 2.x
        • at least one image in general feedback of one question (any question type will do the job)
        • at least one image in the answer text of a multichoice question (Warning: there is no editor for that field in Moodle 1.9, the easiest way to include an image here is to include the image in another field with an editor like question's text, switch to HTML source and copy the HTML fragment related to the image to paste in answer's text field)
        • at least one image in each combined feedback field of a multichoice question (correct, partially correct and incorrect feedback)
        • at least one image in subquestion text of a matching question (use the same trick that for multichoice's answer text).
      2. Create a quiz that uses all the questions you created.
      3. Make a backup of this course including the quiz activity.
      4. You can also use the sample 1.9 backup attached to this issue.
      5. On 23, 24 and master branches
        1. Restore the backup
        2. TEST: Verify that all images are displayed in the questions and are not legacy files (i.e. they are served via pluginfile.php URL).
      Show
      Testing difficulty: easy In Moodle 1.9, create some questions with embedded images in the question bank of a course. To test all areas modified by this patch, you need to include at least one image in question text of one question (any question type will do the job) at least an image in the old "Image" filed that is now deprecated in Moodle 2.x at least one image in general feedback of one question (any question type will do the job) at least one image in the answer text of a multichoice question (Warning: there is no editor for that field in Moodle 1.9, the easiest way to include an image here is to include the image in another field with an editor like question's text, switch to HTML source and copy the HTML fragment related to the image to paste in answer's text field) at least one image in each combined feedback field of a multichoice question (correct, partially correct and incorrect feedback) at least one image in subquestion text of a matching question (use the same trick that for multichoice's answer text). Create a quiz that uses all the questions you created. Make a backup of this course including the quiz activity. You can also use the sample 1.9 backup attached to this issue. On 23, 24 and master branches Restore the backup TEST: Verify that all images are displayed in the questions and are not legacy files (i.e. they are served via pluginfile.php URL).
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      41303

      Description

      Images attached to questions (quiz) lost when restoring a course from platform V1.9 or Course/quiz from V2.1 or V2.2 platforms
      Moodle 2.2.2 + (Build: 20120419).

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          This normally works. Please can you make a small example course (e.g. containing one quiz with one question with images) in 1.9, and back it up, then attach it here. That way I reproduce the problem on my system, and see what is wrong. Thanks.

          Show
          Tim Hunt added a comment - This normally works. Please can you make a small example course (e.g. containing one quiz with one question with images) in 1.9, and back it up, then attach it here. That way I reproduce the problem on my system, and see what is wrong. Thanks.
          Hide
          Mylène POTIER added a comment -

          sorry for the delay, I did a lot of tests before identifying the problem and be able to explain just the conclusion.

          I restore files from V1.9 (and V2 platforms) without use "Legacy course files". Thus links on files (image for example) are created.

          I set "Manage repositorie" to try "race Legacy files". I restored a course. Then I removed that possibility and take away the plugin.
          ("Are you sure you want to remove this repository plugin, its options and all of its instances - Legacy course files?" YES)

          But now when I restore:

          • As a new course: the directory "Legacy race files" is always there!
          • In an existing course: the links for the files are not created.

          I deleted all courses with "Legacy race files", reactivated "Legacy race files" and then removed it to delete the instances. It's the same.

          Show
          Mylène POTIER added a comment - sorry for the delay, I did a lot of tests before identifying the problem and be able to explain just the conclusion. I restore files from V1.9 (and V2 platforms) without use "Legacy course files". Thus links on files (image for example) are created. I set "Manage repositorie" to try "race Legacy files". I restored a course. Then I removed that possibility and take away the plugin. ("Are you sure you want to remove this repository plugin, its options and all of its instances - Legacy course files?" YES) But now when I restore: As a new course: the directory "Legacy race files" is always there! In an existing course: the links for the files are not created. I deleted all courses with "Legacy race files", reactivated "Legacy race files" and then removed it to delete the instances. It's the same.
          Hide
          Tim Hunt added a comment -

          I would still like:

          1. An example backup file that I can use to test this; and
          2. Explicit step-by-step instructions for what I need to do to reproduce the problem.

          Show
          Tim Hunt added a comment - I would still like: 1. An example backup file that I can use to test this; and 2. Explicit step-by-step instructions for what I need to do to reproduce the problem.
          Hide
          Mylène POTIER added a comment -

          1 - Initially, platform V2.1 - upgrade/update regularly
          Restore backups from V1.9 and V2 without racing legacy files Enabling Areas.
          Questions: all links on files were created ex: "<img src="http://xxx/draftfile.php/13/user/draft/116608813/Zen_reg_ch2.jpg"

          2 - Enable the legacy course files repository plugin in Settings > Site administration > Plugins > Repositories > Manage repositories.
          Check the legacyfilesinnewcourses box in the Manage repositories common settings then click the 'Save changes' button.
          Set 'Legacy course files' to Yes in the course settings.

          Then for all backups restored,
          All files link course files area : src="http://xxx/file.php/629/choix_de_sequences.jpg"

          3 - Disable the legacy course files repository plugin in Settings
          Delete all courses restored with "legacy course files repository= ON"

          Backup restored in new course=> legacy course files area is there yet!
          All files link course files area : src="http://xxx/file.php/629/choix_de_sequences.jpg"
          instead of somethink like src="http://xxx/draftfile.php/13/user/draft/116608813/choix_de_sequences.jpg" as before step 2.

          Other test: if restore the same backup in a course created on V2, legacy course files area is not there
          Images either.

          Can't test on demo platform because on "Front page settings" you can see "Legacy site files". And and even if "legacy course files repository= OFF", it's the same than case3.

          How send file ?

          Show
          Mylène POTIER added a comment - 1 - Initially, platform V2.1 - upgrade/update regularly Restore backups from V1.9 and V2 without racing legacy files Enabling Areas. Questions: all links on files were created ex: "<img src="http://xxx/draftfile.php/13/user/draft/116608813/Zen_reg_ch2.jpg" 2 - Enable the legacy course files repository plugin in Settings > Site administration > Plugins > Repositories > Manage repositories. Check the legacyfilesinnewcourses box in the Manage repositories common settings then click the 'Save changes' button. Set 'Legacy course files' to Yes in the course settings. Then for all backups restored, All files link course files area : src="http://xxx/file.php/629/choix_de_sequences.jpg" 3 - Disable the legacy course files repository plugin in Settings Delete all courses restored with "legacy course files repository= ON" Backup restored in new course=> legacy course files area is there yet! All files link course files area : src="http://xxx/file.php/629/choix_de_sequences.jpg" instead of somethink like src="http://xxx/draftfile.php/13/user/draft/116608813/choix_de_sequences.jpg" as before step 2. Other test: if restore the same backup in a course created on V2, legacy course files area is not there Images either. Can't test on demo platform because on "Front page settings" you can see "Legacy site files". And and even if "legacy course files repository= OFF", it's the same than case3. How send file ?
          Hide
          Mylène POTIER added a comment -

          I need a response.
          Do you offer a solution?
          I can't move forward on the platform.

          Show
          Mylène POTIER added a comment - I need a response. Do you offer a solution? I can't move forward on the platform.
          Hide
          CIU Instructional Designer added a comment -

          I am having the same problem as well. I have created a quiz that has an uploaded image that the student must answer questions on on. When I back up and restore the course, the image is not brought over the and is not displayed. The image tag, when in HTML mode, links it to a broken file web link. This is troublesome as we have to constantly go back and update exams and quizzes.

          Show
          CIU Instructional Designer added a comment - I am having the same problem as well. I have created a quiz that has an uploaded image that the student must answer questions on on. When I back up and restore the course, the image is not brought over the and is not displayed. The image tag, when in HTML mode, links it to a broken file web link. This is troublesome as we have to constantly go back and update exams and quizzes.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi!

          I've solved this by adding some lines on function function process_question from class moodle1_question_bank_handler file: backup/converter/moodle1/handlerlib.php

          if ($CFG->texteditors !== 'textarea') {
            [...]
          }
                 
                  //ADD THIS:
                  $this->fileman->contextid = $this->currentcategory['contextid'];
          		$this->fileman->component = 'question';
          		$this->fileman->filearea  = 'questiontext';
          		$this->fileman->itemid    = $data['id'];
                  $data['questiontext'] = moodle1_converter::migrate_referenced_files($data['questiontext'], $this->fileman);
                  //END PATCH
          
          // replay the upgrade step 2010080901 - updating question image
          if (!empty($data['image'])) {
              $textlib = textlib_get_instance();
          

          This may solve also MDL-33817 and maybe MDL-32012

          Hope it helps!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi! I've solved this by adding some lines on function function process_question from class moodle1_question_bank_handler file: backup/converter/moodle1/handlerlib.php if ($CFG->texteditors !== 'textarea') { [...] } //ADD THIS: $ this ->fileman->contextid = $ this ->currentcategory['contextid']; $ this ->fileman->component = 'question'; $ this ->fileman->filearea = 'questiontext'; $ this ->fileman->itemid = $data['id']; $data['questiontext'] = moodle1_converter::migrate_referenced_files($data['questiontext'], $ this ->fileman); //END PATCH // replay the upgrade step 2010080901 - updating question image if (!empty($data['image'])) { $textlib = textlib_get_instance(); This may solve also MDL-33817 and maybe MDL-32012 Hope it helps!
          Hide
          Chris Follin added a comment -

          We have clients experiencing this when trying to import content from textbook publishers' files in 2.1 format. Unfortunately I can't share these as examples because they aren't ours to share.

          We have found that the problem exists when restoring the course into an existing course without selecting "Override course configuration." If restoring with "Override course configuration" or restoring into a new course, the images are maintained.

          Show
          Chris Follin added a comment - We have clients experiencing this when trying to import content from textbook publishers' files in 2.1 format. Unfortunately I can't share these as examples because they aren't ours to share. We have found that the problem exists when restoring the course into an existing course without selecting "Override course configuration." If restoring with "Override course configuration" or restoring into a new course, the images are maintained.
          Hide
          Tim Hunt added a comment -

          I will try to take a look at the proposed fix.

          Show
          Tim Hunt added a comment - I will try to take a look at the proposed fix.
          Hide
          Tim Hunt added a comment -

          I think the fix is good, in so far as it goes, but it does not go far enough.

          We also need to deal with files in:

          • question.generalfeedback
          • question_answers.answer
          • question_answers.feedback
          • any question-type specific fields that may contain images in their HTML.

          Does anyone have time to extend this to cover all the other necessary fields?

          ------

          I do not understand Chris's observation that

          We have found that the problem exists when restoring the course into an existing course without selecting "Override course configuration." If restoring with "Override course configuration" or restoring into a new course, the images are maintained.

          unless it is the case that with those options on, restoring a 1.9 backup turns on the "legacy course files" repository for that course.

          ------

          Finally, I am adding David Mudrak as a watcher, since he understands this part of the code much better than me. I hope he can give his expert review.

          Show
          Tim Hunt added a comment - I think the fix is good, in so far as it goes, but it does not go far enough. We also need to deal with files in: question.generalfeedback question_answers.answer question_answers.feedback any question-type specific fields that may contain images in their HTML. Does anyone have time to extend this to cover all the other necessary fields? ------ I do not understand Chris's observation that We have found that the problem exists when restoring the course into an existing course without selecting "Override course configuration." If restoring with "Override course configuration" or restoring into a new course, the images are maintained. unless it is the case that with those options on, restoring a 1.9 backup turns on the "legacy course files" repository for that course. ------ Finally, I am adding David Mudrak as a watcher, since he understands this part of the code much better than me. I hope he can give his expert review.
          Hide
          David Mudrak added a comment -

          Thanks everybody for their inputs here. When we were writing moodle1 converters, we used the upgrade code as the reference. And as there was nothing explicitly dealing with images embedded into the question HTML fields, we probably just skipped this part. I personally sort of expected images being attached via the special "Question image" field only. As Tim explained to me today, I was apparently wrong.

          To move with this, we would need to test how the 1.9 -> 2.x upgrade code deals with embedded images. Is anybody volunteering to run such test? I have a suspicion that the img src links are just left as they are which might explain a bit how Legacy course files become involved...

          Show
          David Mudrak added a comment - Thanks everybody for their inputs here. When we were writing moodle1 converters, we used the upgrade code as the reference. And as there was nothing explicitly dealing with images embedded into the question HTML fields, we probably just skipped this part. I personally sort of expected images being attached via the special "Question image" field only. As Tim explained to me today, I was apparently wrong. To move with this, we would need to test how the 1.9 -> 2.x upgrade code deals with embedded images. Is anybody volunteering to run such test? I have a suspicion that the img src links are just left as they are which might explain a bit how Legacy course files become involved...
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          For generalfeedback change the previous patch adding two lines:

          if ($CFG->texteditors !== 'textarea') {
            [...]
          }
                 
                  //ADD THIS:
                  $this->fileman->contextid = $this->currentcategory['contextid'];
          		$this->fileman->component = 'question';
          		$this->fileman->filearea  = 'questiontext';
          		$this->fileman->itemid    = $data['id'];
                  $data['questiontext'] = moodle1_converter::migrate_referenced_files($data['questiontext'], $this->fileman);
          
                  $this->fileman->filearea  = 'generalfeedback';
                  $data['generalfeedback'] = moodle1_converter::migrate_referenced_files($data['generalfeedback'], $this->fileman);
                  //END PATCH
          
          // replay the upgrade step 2010080901 - updating question image
          if (!empty($data['image'])) {
              $textlib = textlib_get_instance();
          

          I'll look for the solution of the answers

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - For generalfeedback change the previous patch adding two lines: if ($CFG->texteditors !== 'textarea') { [...] } //ADD THIS: $ this ->fileman->contextid = $ this ->currentcategory['contextid']; $ this ->fileman->component = 'question'; $ this ->fileman->filearea = 'questiontext'; $ this ->fileman->itemid = $data['id']; $data['questiontext'] = moodle1_converter::migrate_referenced_files($data['questiontext'], $ this ->fileman); $ this ->fileman->filearea = 'generalfeedback'; $data['generalfeedback'] = moodle1_converter::migrate_referenced_files($data['generalfeedback'], $ this ->fileman); //END PATCH // replay the upgrade step 2010080901 - updating question image if (!empty($data['image'])) { $textlib = textlib_get_instance(); I'll look for the solution of the answers
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          For answers feedback, in the same file, function convert_answer on moodle1_qtype_handler class

          ...
                  //ADD THIS
                  $fileman =  $this->converter->get_file_manager();
                  $fileman->contextid = $this->qbankhandler->currentcategory['contextid'];
                  $fileman->component = 'question';
                  $fileman->itemid    = $old['id'];
                  $fileman->filearea  = 'answerfeedback';
                  $new['feedback'] = moodle1_converter::migrate_referenced_files($new['feedback'], $fileman);
                  //END PATCH
          
                  return $new;
          

          But it has one real problem, on moodle1_question_bank_handler the property $currentcategory is declared as protected and this patch requires to be public.

          I don't think question_answers.answer have to be migrated, I've tested with one shortanwser and it does not worked but the patch is this:

          //Add to the patch above
          $fileman->filearea  = 'answer';
          $new['answertext'] = moodle1_converter::migrate_referenced_files($new['answertext'], $fileman);
          
          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - For answers feedback, in the same file, function convert_answer on moodle1_qtype_handler class ... //ADD THIS $fileman = $ this ->converter->get_file_manager(); $fileman->contextid = $ this ->qbankhandler->currentcategory['contextid']; $fileman->component = 'question'; $fileman->itemid = $old['id']; $fileman->filearea = 'answerfeedback'; $ new ['feedback'] = moodle1_converter::migrate_referenced_files($ new ['feedback'], $fileman); //END PATCH return $ new ; But it has one real problem, on moodle1_question_bank_handler the property $currentcategory is declared as protected and this patch requires to be public. I don't think question_answers.answer have to be migrated, I've tested with one shortanwser and it does not worked but the patch is this: //Add to the patch above $fileman->filearea = 'answer'; $ new ['answertext'] = moodle1_converter::migrate_referenced_files($ new ['answertext'], $fileman);
          Hide
          Chris Follin added a comment -

          Hi. This has been in peer review for a month. Is there any update on this? Thanks!

          Show
          Chris Follin added a comment - Hi. This has been in peer review for a month. Is there any update on this? Thanks!
          Hide
          Tim Hunt added a comment -

          Err. Yes. Look at all those comments above, including several comments from me asking for example files, clarification, or further changes to the patch, that no one has responded to.

          What a pity that Moodlerooms does not employ any developers who could help get this resolved.

          Show
          Tim Hunt added a comment - Err. Yes. Look at all those comments above, including several comments from me asking for example files, clarification, or further changes to the patch, that no one has responded to. What a pity that Moodlerooms does not employ any developers who could help get this resolved.
          Hide
          Martin Dougiamas added a comment -

          Mylène, you can add files here using the "More actions" menu on this page, and then "Attach files".

          Show
          Martin Dougiamas added a comment - Mylène, you can add files here using the "More actions" menu on this page, and then "Attach files".
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          I am interested in testing what you said "To move with this, we would need to test how the 1.9 -> 2.x upgrade code deals with embedded images. Is anybody volunteering to run such test? I have a suspicion that the img src links are just left as they are which might explain a bit how Legacy course files become involved..."
          I think your suspicion is right ... Do you have any idea of specific tests I can make (apart of course creating questions with images in various parts in a 1.9 site and testing upgrade) ?
          If you are right and if embedded images are not migrated during upgrade, as I understand it we need to fix both the upgrade code (in MOODLE_22_STABLE) and the moodle1 converter (in all branchs), Right ?

          Show
          Jean-Michel Vedrine added a comment - Hello David, I am interested in testing what you said "To move with this, we would need to test how the 1.9 -> 2.x upgrade code deals with embedded images. Is anybody volunteering to run such test? I have a suspicion that the img src links are just left as they are which might explain a bit how Legacy course files become involved..." I think your suspicion is right ... Do you have any idea of specific tests I can make (apart of course creating questions with images in various parts in a 1.9 site and testing upgrade) ? If you are right and if embedded images are not migrated during upgrade, as I understand it we need to fix both the upgrade code (in MOODLE_22_STABLE) and the moodle1 converter (in all branchs), Right ?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          I installed latest Moodle 1.9 and created some questions with images in various fields. Then I made some backups.
          I upgraded to Moodle 2.2 latest and I can't see any difference between images urls in various activities (forum, glossary, labels, ...) and the one in questions. they are all of the same form (and they work). Of course they are all using legacy files but this seems quite normal to me.
          Maybe I am wrong but I don't see any bug here

          Show
          Jean-Michel Vedrine added a comment - Hello David, I installed latest Moodle 1.9 and created some questions with images in various fields. Then I made some backups. I upgraded to Moodle 2.2 latest and I can't see any difference between images urls in various activities (forum, glossary, labels, ...) and the one in questions. they are all of the same form (and they work). Of course they are all using legacy files but this seems quite normal to me. Maybe I am wrong but I don't see any bug here
          Hide
          Jean-Michel Vedrine added a comment -

          Chris clients problem seems that these publishers files even if they are Moodle 2.x backups are in fact courses upgraded from Moodle 1.9 so they use legacy course files. If they are restored in an existing course without selecting "Override course configuration." of course there is a problem because legacy course files stay unactivated.
          But Pau Ferrer Ocaña's patch would certainly not solve that case !!

          Show
          Jean-Michel Vedrine added a comment - Chris clients problem seems that these publishers files even if they are Moodle 2.x backups are in fact courses upgraded from Moodle 1.9 so they use legacy course files. If they are restored in an existing course without selecting "Override course configuration." of course there is a problem because legacy course files stay unactivated. But Pau Ferrer Ocaña's patch would certainly not solve that case !!
          Hide
          Jean-Michel Vedrine added a comment -

          After a lot of tests, here is my current thinking (Remember I am certainly not a backup/restore expert so this is mostly for David to correct me when I am wrong)

          • This issue is not at all a quiz issue, because I didn't manage to find a situation where the quiz behave differently than other activities during backup/restoration of images.
          • There is no point in "fixing" quiz moodle1 converters because the problem is not limited to 1.9 backups restored in Moodle 2.x (Moodle 2.x backups with files in legacy course files suffer from exactly the same problem) and also the problem is not limited to the quiz module. So that means I personnaly think that Pau Ferrer Ocaña's patch is not the right solution to this problem

          The problem has been correctly diagnosed by Chris. To reproduce :

          • take a 1.9 or a 2.x backup with some activities having images in legacy course files (the easiest way is of course that these activities originated from a Moodle 1.9 website upgraded or not, but if they have been created in a 2.x course with legacy course files enabled the result is absolutely the same.
          • restore this course in a 2.x website in an existing course where legacy course files is not activated and letting "Override course configuration" off (all these points are important as Chris said)
          • result : activities are restored but all images links are now broken.

          But what I fail to see is what should be the correct Moodle behaviour ? Images can't be restored as legacy course files because it is not activated and this can't be changed without modifying course configuration !! Maybe the right solution is just to warn the user that the backup he is trying to restore contains legacy course files and that they will be lost ?
          Another possibility would be to take Pau Ferrer Ocaña's road and decide that in that case all urls are recoded and images are restored as non legacy documents, but I repeat :

          • this should not be done in moodle1 converters as this problem is not limited to 1.9 backups
          • this should not be done for quiz only as the problem is not limited to quiz

          Additionnaly this is a complete change in the way Moodle manage documents in pre-2.0 activities

          If my analysis is correct, my +1 to simply warn users when they try to restore a course with legacy files in an existing course not able to accept them.

          Show
          Jean-Michel Vedrine added a comment - After a lot of tests, here is my current thinking (Remember I am certainly not a backup/restore expert so this is mostly for David to correct me when I am wrong) This issue is not at all a quiz issue, because I didn't manage to find a situation where the quiz behave differently than other activities during backup/restoration of images. There is no point in "fixing" quiz moodle1 converters because the problem is not limited to 1.9 backups restored in Moodle 2.x (Moodle 2.x backups with files in legacy course files suffer from exactly the same problem) and also the problem is not limited to the quiz module. So that means I personnaly think that Pau Ferrer Ocaña's patch is not the right solution to this problem The problem has been correctly diagnosed by Chris. To reproduce : take a 1.9 or a 2.x backup with some activities having images in legacy course files (the easiest way is of course that these activities originated from a Moodle 1.9 website upgraded or not, but if they have been created in a 2.x course with legacy course files enabled the result is absolutely the same. restore this course in a 2.x website in an existing course where legacy course files is not activated and letting "Override course configuration" off (all these points are important as Chris said) result : activities are restored but all images links are now broken. But what I fail to see is what should be the correct Moodle behaviour ? Images can't be restored as legacy course files because it is not activated and this can't be changed without modifying course configuration !! Maybe the right solution is just to warn the user that the backup he is trying to restore contains legacy course files and that they will be lost ? Another possibility would be to take Pau Ferrer Ocaña's road and decide that in that case all urls are recoded and images are restored as non legacy documents, but I repeat : this should not be done in moodle1 converters as this problem is not limited to 1.9 backups this should not be done for quiz only as the problem is not limited to quiz Additionnaly this is a complete change in the way Moodle manage documents in pre-2.0 activities If my analysis is correct, my +1 to simply warn users when they try to restore a course with legacy files in an existing course not able to accept them.
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to answer Tim : "unless it is the case that with those options on, restoring a 1.9 backup turns on the "legacy course files" repository for that course."
          This is what happens if "Override course configuration" is set to yes, in that case legacy course files is activated during the restore and images are correctly restored, so to reproduce the problem you both need that legacy course files was not activated prior to the restore and that "Override course configuration" is set to "no" during the restore.

          Show
          Jean-Michel Vedrine added a comment - I forgot to answer Tim : "unless it is the case that with those options on, restoring a 1.9 backup turns on the "legacy course files" repository for that course." This is what happens if "Override course configuration" is set to yes, in that case legacy course files is activated during the restore and images are correctly restored, so to reproduce the problem you both need that legacy course files was not activated prior to the restore and that "Override course configuration" is set to "no" during the restore.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I have a question for David (assuming my analysis is right ) : Why was this system choosen when Moodle 2.0 was conceived rather than really migrate all documents embedded in html fragments to a proper " .../pluginfile.php?... " url ??

          Show
          Jean-Michel Vedrine added a comment - - edited I have a question for David (assuming my analysis is right ) : Why was this system choosen when Moodle 2.0 was conceived rather than really migrate all documents embedded in html fragments to a proper " .../pluginfile.php?... " url ??
          Hide
          David Mudrak added a comment -

          Thanks Jean-Michel for the great analysis of the problem. It seems to clarify a lot of things.

          To answer your questions / comment in reverse chronological order:

          • The original plan was to convert all links to use proper pluginfile.php during the upgrade. However, this was not achieved. Some places have been converted but many of them were just left relying on this Legacy files support. So, simply said, it was not done intentionally but more as a "at least something" solution.
          • I tend to like the solution of warning the user. But we'll have to address this one day anyway. We can't live with Legacy files forever so we need upgrade steps that would convert embedded files. Such an upgrade step has already been done in the Book module, for example. We just need to gradually do it everywhere... (well, there are other issues involved like MDL-28094 still).
          Show
          David Mudrak added a comment - Thanks Jean-Michel for the great analysis of the problem. It seems to clarify a lot of things. To answer your questions / comment in reverse chronological order: The original plan was to convert all links to use proper pluginfile.php during the upgrade. However, this was not achieved. Some places have been converted but many of them were just left relying on this Legacy files support. So, simply said, it was not done intentionally but more as a "at least something" solution. I tend to like the solution of warning the user. But we'll have to address this one day anyway. We can't live with Legacy files forever so we need upgrade steps that would convert embedded files. Such an upgrade step has already been done in the Book module, for example. We just need to gradually do it everywhere... (well, there are other issues involved like MDL-28094 still).
          Hide
          David Mudrak added a comment -

          Adding Petr and Eloy as watchers. Guys, your wisdom will be appreciated. Thanks.

          Show
          David Mudrak added a comment - Adding Petr and Eloy as watchers. Guys, your wisdom will be appreciated. Thanks.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Thanks David for the infos. It brings a lot more light on the subject !
          I looked at the book module but the only relevant code I found was the one in the moodle1 converter. Is there anything else I missed ?
          So looking at the code in mod/book/backup/moodle1/lib.php, it's clear I was wrong and Pau Ferrer Ocaña's patch is needed to at least solve the problem for 1.9 backups restored in 2.x.
          But this doesn't solve the problem for all existing questions (or other activities, and maybe other parts of Moodle, ...) having texts with legacy embedded documents. This also doesn't solve the problem for all 2.x backups having texts with legacy documents embedded and what to do when you restore such a file.
          Will all Moodle site upgraded from 1.9 have to live with legacy course files forever ?
          Could an admin tool to migrate all those documents be the solution ?
          Of course to be usable on very big sites such an admin tool would have to be rather complex with a lot of options so that upgrade can be fractionned and scheduled. Something like what Tim has done in qeupgradehelper that worked even on sites with millions of attempts.
          The problem of third party plugins must also be solved so this tool will probably have to rely on a library in each plugin and an API must be decided.
          But, as you said, deciding what to do and closing MDL-28094 must absolutely been done before all that work can begin.

          Show
          Jean-Michel Vedrine added a comment - - edited Thanks David for the infos. It brings a lot more light on the subject ! I looked at the book module but the only relevant code I found was the one in the moodle1 converter. Is there anything else I missed ? So looking at the code in mod/book/backup/moodle1/lib.php, it's clear I was wrong and Pau Ferrer Ocaña's patch is needed to at least solve the problem for 1.9 backups restored in 2.x. But this doesn't solve the problem for all existing questions (or other activities, and maybe other parts of Moodle, ...) having texts with legacy embedded documents. This also doesn't solve the problem for all 2.x backups having texts with legacy documents embedded and what to do when you restore such a file. Will all Moodle site upgraded from 1.9 have to live with legacy course files forever ? Could an admin tool to migrate all those documents be the solution ? Of course to be usable on very big sites such an admin tool would have to be rather complex with a lot of options so that upgrade can be fractionned and scheduled. Something like what Tim has done in qeupgradehelper that worked even on sites with millions of attempts. The problem of third party plugins must also be solved so this tool will probably have to rely on a library in each plugin and an API must be decided. But, as you said, deciding what to do and closing MDL-28094 must absolutely been done before all that work can begin.
          Hide
          David Mudrak added a comment -

          Oh sorry I did not mention. The Book code I was referring to can be seen at

          https://github.com/skodak/moodle-mod_book/blob/MOODLE_22_STABLE/db/upgradelib.php

          It has been removed from MOODLE_23_STABLE by an accident.

          Show
          David Mudrak added a comment - Oh sorry I did not mention. The Book code I was referring to can be seen at https://github.com/skodak/moodle-mod_book/blob/MOODLE_22_STABLE/db/upgradelib.php It has been removed from MOODLE_23_STABLE by an accident.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks David,
          I understand why I missed it
          Silly question : if it was "removed by accident", why was it not prompty re added ?
          Also it seems that restoration of images in book from 1.9 backups is broken too because of an easy to fix bug , see MDL-35032. Is anybody working on fixing it ?

          Show
          Jean-Michel Vedrine added a comment - Thanks David, I understand why I missed it Silly question : if it was "removed by accident", why was it not prompty re added ? Also it seems that restoration of images in book from 1.9 backups is broken too because of an easy to fix bug , see MDL-35032 . Is anybody working on fixing it ?
          Hide
          Jean-Michel Vedrine added a comment -

          Oh yes I understand why it wasn't re-added, the idea was that all the upgrade stuff dealing with pre-2.X things is useless in 2.3 as you can't upgrade from 1.9 to 2.3 directly, but in the case of book that has been integrated in core with 2.3 release, it surely has caused some trouble to users upgrading to Moodle 2.3 and not having the latest book 2.2 release !

          Show
          Jean-Michel Vedrine added a comment - Oh yes I understand why it wasn't re-added, the idea was that all the upgrade stuff dealing with pre-2.X things is useless in 2.3 as you can't upgrade from 1.9 to 2.3 directly, but in the case of book that has been integrated in core with 2.3 release, it surely has caused some trouble to users upgrading to Moodle 2.3 and not having the latest book 2.2 release !
          Hide
          Jean-Michel Vedrine added a comment -

          I made a full search of fields in core 1.9 questions tables that can contain images and are able to be migrated (I made a list of all text fields in 1.9 question's tables and suppressed all that don't have an associated format and filearea in 2.2). Here they are :
          question
          questiontext
          generalfeedback

          question_answers
          answer
          feedback

          question_match_sub
          questiontext

          question_multichoice
          correctfeedback
          partiallycorrectfeedback
          incorrectfeedback
          If nobody is able to do it before, during my next vacations in 2 weeks, I can prepare git branchs for all stables and master implementing code in moodle1 converters to migrate files when a Moodle 1.9 backup is restored. I will also look at how it can be done for third party questiontypes when they have similar fields. This will be a first step.

          Show
          Jean-Michel Vedrine added a comment - I made a full search of fields in core 1.9 questions tables that can contain images and are able to be migrated (I made a list of all text fields in 1.9 question's tables and suppressed all that don't have an associated format and filearea in 2.2). Here they are : question questiontext generalfeedback question_answers answer feedback question_match_sub questiontext question_multichoice correctfeedback partiallycorrectfeedback incorrectfeedback If nobody is able to do it before, during my next vacations in 2 weeks, I can prepare git branchs for all stables and master implementing code in moodle1 converters to migrate files when a Moodle 1.9 backup is restored. I will also look at how it can be done for third party questiontypes when they have similar fields. This will be a first step.
          Hide
          David Mudrak added a comment -

          Exactly Jean-Michael. The situation in the Book upgrade code was a bit complex and yes, it has caused troubles to some users. It would be great if you could prepare Git branches that add necessary changes to moodle1. I don't think I'll find to work on it personally.

          Show
          David Mudrak added a comment - Exactly Jean-Michael. The situation in the Book upgrade code was a bit complex and yes, it has caused troubles to some users. It would be great if you could prepare Git branches that add necessary changes to moodle1. I don't think I'll find to work on it personally.
          Hide
          Tim Hunt added a comment -

          Just updating tracker workflow status to accurately reflect what is going on.

          Show
          Tim Hunt added a comment - Just updating tracker workflow status to accurately reflect what is going on.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello,
          Sorry it took so long , but work on this has not stopped. I have some trouble finding a solution for qtypes.
          First problem
          to be able to migrate their own files, I think that moodle1_qtype_handler need to have access to their qbankhandler->currentcategory (to know question contextid) and qbankhandler->fileman properties but currently they are protected in the moodle1_question_bank_handler class. My proposal is to make them public.
          But I Would like David and Tim approval before doing that. Or maybe there is another better solution ?

          Show
          Jean-Michel Vedrine added a comment - - edited Hello, Sorry it took so long , but work on this has not stopped. I have some trouble finding a solution for qtypes. First problem to be able to migrate their own files, I think that moodle1_qtype_handler need to have access to their qbankhandler->currentcategory (to know question contextid) and qbankhandler->fileman properties but currently they are protected in the moodle1_question_bank_handler class. My proposal is to make them public. But I Would like David and Tim approval before doing that. Or maybe there is another better solution ?
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, hello David,
          I have made a git branch with the actual state of my work.
          The idea is that third party question types will have to add calls to $this->restore_files for their relevant fields to implement this change. I have tested on some of my own qtype plugins and it works. A qtype plugin without these calls will still work using legacy course files, and a plugin with $this->restore_files calls will restore images as non legacy files.

          There are some remaining questions for core questions types, the main one is that the code in convert_answer seems wrong to me : answerformat should be FORMAT_HTML and not FORMAT_MOODLE for multichoice, and feedbackformat should be FORMAT_HTML for all qtypes not just essay.

          Also I want to make a lot more tests in a lot more situations before writing testing instructions and asking for peer review.
          But creating a git branch so that you can look at it and make comments seemed like a good solution.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, hello David, I have made a git branch with the actual state of my work. The idea is that third party question types will have to add calls to $this->restore_files for their relevant fields to implement this change. I have tested on some of my own qtype plugins and it works. A qtype plugin without these calls will still work using legacy course files, and a plugin with $this->restore_files calls will restore images as non legacy files. There are some remaining questions for core questions types, the main one is that the code in convert_answer seems wrong to me : answerformat should be FORMAT_HTML and not FORMAT_MOODLE for multichoice, and feedbackformat should be FORMAT_HTML for all qtypes not just essay. Also I want to make a lot more tests in a lot more situations before writing testing instructions and asking for peer review. But creating a git branch so that you can look at it and make comments seemed like a good solution.
          Hide
          Jean-Michel Vedrine added a comment -

          All my tests seems to show that the patch is working as it should.
          Now I will look at adding an upgrade step to migrate legacy files similar to the book module's code. Unfortunately plugins qtypes will also have to do that to migrate their own files. I need to find a way so that can be done in the less painful way for qtypes authors (as I am one of them )
          Something bother me :
          Obviously a big amount of the code I will use for that could also be used to do the same thing for other parts of Moodle (see Petr's comment "Migrate one area, this should be probably part of moodle core..." in the phpdoc block for mod_book_migrate_area function) should I try to write general code and put it somewhere it could be reused (where ?) ? Or should I stick with the goal of migrating the question bank files only ?
          Petr, you surely have good ideas about this. I would really appreciate your opinion.
          All my contributions to Moodle so far where only aimed at the question or quiz components it's in fact the first time I may write code that could be potentially used in a lot of places in Moodle so i am a bit frightened.

          Show
          Jean-Michel Vedrine added a comment - All my tests seems to show that the patch is working as it should. Now I will look at adding an upgrade step to migrate legacy files similar to the book module's code. Unfortunately plugins qtypes will also have to do that to migrate their own files. I need to find a way so that can be done in the less painful way for qtypes authors (as I am one of them ) Something bother me : Obviously a big amount of the code I will use for that could also be used to do the same thing for other parts of Moodle (see Petr's comment "Migrate one area, this should be probably part of moodle core..." in the phpdoc block for mod_book_migrate_area function) should I try to write general code and put it somewhere it could be reused (where ?) ? Or should I stick with the goal of migrating the question bank files only ? Petr, you surely have good ideas about this. I would really appreciate your opinion. All my contributions to Moodle so far where only aimed at the question or quiz components it's in fact the first time I may write code that could be potentially used in a lot of places in Moodle so i am a bit frightened.
          Hide
          Jean-Michel Vedrine added a comment -

          In fact Petr's function mod_book_migrate_area seems quite general !! What do you think of calling it simply migrate_area and putting it somewhere any component or plugin could use it ?
          There must be something obvious I don't see preventing this or it would have been done since a long time ?
          I am puzzled !

          Show
          Jean-Michel Vedrine added a comment - In fact Petr's function mod_book_migrate_area seems quite general !! What do you think of calling it simply migrate_area and putting it somewhere any component or plugin could use it ? There must be something obvious I don't see preventing this or it would have been done since a long time ? I am puzzled !
          Hide
          Tim Hunt added a comment -

          If the function is really generic, then it would be good to move it to one fo the files in backup/converter/moodle1. (I don't know which of the three would be best.)

          Petr may have put the function in mod/book because when he wrote the code it was CONTRIB, and not Moodle core code, and he wanted it to be self-contained.

          Show
          Tim Hunt added a comment - If the function is really generic, then it would be good to move it to one fo the files in backup/converter/moodle1. (I don't know which of the three would be best.) Petr may have put the function in mod/book because when he wrote the code it was CONTRIB, and not Moodle core code, and he wanted it to be self-contained.
          Hide
          Tim Hunt added a comment -

          If we want to add this function to core, to help contrib qtypes, then we should try to add the function to as many stable branches as possible, so it may be best to create a separate issue for that.

          Show
          Tim Hunt added a comment - If we want to add this function to core, to help contrib qtypes, then we should try to add the function to as many stable branches as possible, so it may be best to create a separate issue for that.
          Hide
          Jean-Michel Vedrine added a comment -

          I think you are right Tim, better to only do one thing at a time. I will create another issue to integrate migrate_area in core and a follow-up to use it in question upgrade script.
          So putting this in peer review. Maybe additionally to your comments it would be interesting to have David's one as a backup expert ?
          As notification seems broken since JIRA upgrade, I am not sure he will be aware of the progress on this issue.

          Show
          Jean-Michel Vedrine added a comment - I think you are right Tim, better to only do one thing at a time. I will create another issue to integrate migrate_area in core and a follow-up to use it in question upgrade script. So putting this in peer review. Maybe additionally to your comments it would be interesting to have David's one as a backup expert ? As notification seems broken since JIRA upgrade, I am not sure he will be aware of the progress on this issue.
          Hide
          David Mudrak added a comment -

          Many thanks Jean-Michel for the patch. I really appreciate. I haven't found anything that should block integrating this. Although, you might want to consider introducing a method like moodle1_question_bank_handler::get_file_manager() that would simply return $this->fileman. Similarly, the other method moodle1_question_bank_handler::get_current_category_context() would return the required context for the question. I would prefer that over making relevant protected properties public. The other thing is that I would not use the "restore" word in the name of the restore_files() method. As you know, the files are not actually restored (that is registered in the filepool) by moodle1. All it does is to convert moodle1 (ZIP) format to moodle2 (MBZ). Using the word "restore" might be confusing. What about "migrate_files" or so?

          Show
          David Mudrak added a comment - Many thanks Jean-Michel for the patch. I really appreciate. I haven't found anything that should block integrating this. Although, you might want to consider introducing a method like moodle1_question_bank_handler::get_file_manager() that would simply return $this->fileman. Similarly, the other method moodle1_question_bank_handler::get_current_category_context() would return the required context for the question. I would prefer that over making relevant protected properties public. The other thing is that I would not use the "restore" word in the name of the restore_files() method. As you know, the files are not actually restored (that is registered in the filepool) by moodle1. All it does is to convert moodle1 (ZIP) format to moodle2 (MBZ). Using the word "restore" might be confusing. What about "migrate_files" or so?
          Hide
          Tim Hunt added a comment -

          Just fixing workflow state.

          Show
          Tim Hunt added a comment - Just fixing workflow state.
          Hide
          Jean-Michel Vedrine added a comment -

          I am aware that some tests are failing with the current code but not had a chance to look at it. Will do as soon as I can.

          Show
          Jean-Michel Vedrine added a comment - I am aware that some tests are failing with the current code but not had a chance to look at it. Will do as soon as I can.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          After studying the failed tests I realized that in fact they are failing on an unmodified Moodle master download on my install (so this patch is not reason why they are failing). Why would some tests no be OK on my system ? I am attaching the description of the errors and failures. This worry me because I think my install of phpunit is not OK if some tests are failing with master branch "out of the box" and I can't really test that everything is OK after my changes if everything is not OK before

          Show
          Jean-Michel Vedrine added a comment - - edited After studying the failed tests I realized that in fact they are failing on an unmodified Moodle master download on my install (so this patch is not reason why they are failing). Why would some tests no be OK on my system ? I am attaching the description of the errors and failures. This worry me because I think my install of phpunit is not OK if some tests are failing with master branch "out of the box" and I can't really test that everything is OK after my changes if everything is not OK before
          Hide
          Jean-Michel Vedrine added a comment -

          phpunit errors for moodle master on my system

          Show
          Jean-Michel Vedrine added a comment - phpunit errors for moodle master on my system
          Hide
          Tim Hunt added a comment -

          Some of the failures are known issues with MySQL. (Changing the MySQL collation to utf8-bin might fix them.)

          Some seem to be windows-specific problems, because I see them too at work, but I have not managed to work out why.

          The moodle1 backup failures only started happening after last week for me.

          Show
          Tim Hunt added a comment - Some of the failures are known issues with MySQL. (Changing the MySQL collation to utf8-bin might fix them.) Some seem to be windows-specific problems, because I see them too at work, but I have not managed to work out why. The moodle1 backup failures only started happening after last week for me.
          Hide
          Jean-Michel Vedrine added a comment -

          For the best I can see all the phpunit errors I see are not related to my changes (but to be sure it should be tested with another OS than Windows which I am not able to)
          I think I have taken into account David's remarks but not quite sure that what I did for get_file_manager and get_current_category_context is what he wanted ?
          So I put this in peer review to get some more feedback.

          Show
          Jean-Michel Vedrine added a comment - For the best I can see all the phpunit errors I see are not related to my changes (but to be sure it should be tested with another OS than Windows which I am not able to) I think I have taken into account David's remarks but not quite sure that what I did for get_file_manager and get_current_category_context is what he wanted ? So I put this in peer review to get some more feedback.
          Hide
          Tim Hunt added a comment -

          David, I am hoping that you can peer review this, since I don't understand at all. Thanks.

          It would help to have some testing instructions here.

          Show
          Tim Hunt added a comment - David, I am hoping that you can peer review this, since I don't understand at all. Thanks. It would help to have some testing instructions here.
          Hide
          David Mudrak added a comment -

          Behold! Peer review in progress!

          Show
          David Mudrak added a comment - Behold! Peer review in progress!
          Hide
          David Mudrak added a comment -

          I like the patch! Thanks Jean-Michel a lot for it. I haven't tested on my own but the logic is clear and fits the moodle1 converter design.

          The only thing I would like to get amended yet is the phpDoc block for the two new methods introduced in the patch. The get_file_manager() should have a line like

          @return moodle1_file_manager
          

          added. And the doc block for the get_current_category_context() should be clarified, for example as

          /**
           * Returns the information about the question category context being currently parsed
           *
           * @return array with keys contextid, contextlevel and contextinstanceid
           */
          

          Jean-Michel, if the patch works for you (as you stated), this gets my +1. I am sure you are able to provide testing instructions for this as well.

          Thanks again once more for this nice work.

          Show
          David Mudrak added a comment - I like the patch! Thanks Jean-Michel a lot for it. I haven't tested on my own but the logic is clear and fits the moodle1 converter design. The only thing I would like to get amended yet is the phpDoc block for the two new methods introduced in the patch. The get_file_manager() should have a line like @ return moodle1_file_manager added. And the doc block for the get_current_category_context() should be clarified, for example as /** * Returns the information about the question category context being currently parsed * * @ return array with keys contextid, contextlevel and contextinstanceid */ Jean-Michel, if the patch works for you (as you stated), this gets my +1. I am sure you are able to provide testing instructions for this as well. Thanks again once more for this nice work.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Thanks David.
          Backup/restore is a complicated area of Moodle and I am really amazed by this component, so your compliments are really appreciated.
          I have fixed phpDoc blocks and squashed all commits in one.
          I will write testing instructions.
          In all the tests I have done questions images were correctly restored as non legacy files.

          Show
          Jean-Michel Vedrine added a comment - - edited Thanks David. Backup/restore is a complicated area of Moodle and I am really amazed by this component, so your compliments are really appreciated. I have fixed phpDoc blocks and squashed all commits in one. I will write testing instructions. In all the tests I have done questions images were correctly restored as non legacy files.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          I have created testing instructions but not sure if they are good enough ?

          Show
          Jean-Michel Vedrine added a comment - Hello David, I have created testing instructions but not sure if they are good enough ?
          Hide
          David Mudrak added a comment -

          Thanks Jean-Michel! Testing instructions were completely ok. I just reformatted them a bit in a way that I personally found pretty useful for the tester to follow.

          Show
          David Mudrak added a comment - Thanks Jean-Michel! Testing instructions were completely ok. I just reformatted them a bit in a way that I personally found pretty useful for the tester to follow.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks David this is better and clearer.
          Should I do other branchs before this is send to integration or do you think this should be master only ?

          Show
          Jean-Michel Vedrine added a comment - Thanks David this is better and clearer. Should I do other branchs before this is send to integration or do you think this should be master only ?
          Hide
          Tim Hunt added a comment -

          This is a bug fix, so it should be fixed on all stable branches. Once we have those branches in git, I think this is ready for integration.

          Show
          Tim Hunt added a comment - This is a bug fix, so it should be fixed on all stable branches. Once we have those branches in git, I think this is ready for integration.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, I have done other branch, but please don't send this to integration now, I wanted to do a final test on all branchs and I have some problems. I will tell you ASAP if I made a dumb mistake in my latest tests or if this need some more work.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have done other branch, but please don't send this to integration now, I wanted to do a final test on all branchs and I have some problems. I will tell you ASAP if I made a dumb mistake in my latest tests or if this need some more work.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          As far as I can see it seems that the code in MDL-36977 has broken the code in this issue, specially the change in migrate_file function in backup/converter/moodle1/lib.php

                  if ($sourcefullpath !== clean_param($sourcefullpath, PARAM_PATH)) {
                      throw new moodle1_convert_exception('file_invalid_path', $sourcefullpath);
                  }
          

          My problem is that I can see that issue as it seems labelled as a security one (I get permission violation) so it's a little difficult for me to guess how to correct the problem.
          On my windows machine $sourcefullpath could be something like C:\wamp\moodledata_24/temp/backup/558ddee60068ee6442620f5d17a91d34/course_files\bullet-1.gif
          but as cleanparam($sourcefullpath, PARAM_PATH) is C/wamp/moodledata_24/temp/backup/558ddee60068ee6442620f5d17a91d34/course_files/bullet-1.gif obviously I get an exception.
          I will try to understand how I can correct the problem (most probably I need to sanitize some paths in my code ?) but any light you can bring would help. Thanks.

          Show
          Jean-Michel Vedrine added a comment - Hello David, As far as I can see it seems that the code in MDL-36977 has broken the code in this issue, specially the change in migrate_file function in backup/converter/moodle1/lib.php if ($sourcefullpath !== clean_param($sourcefullpath, PARAM_PATH)) { throw new moodle1_convert_exception('file_invalid_path', $sourcefullpath); } My problem is that I can see that issue as it seems labelled as a security one (I get permission violation) so it's a little difficult for me to guess how to correct the problem. On my windows machine $sourcefullpath could be something like C:\wamp\moodledata_24/temp/backup/558ddee60068ee6442620f5d17a91d34/course_files\bullet-1.gif but as cleanparam($sourcefullpath, PARAM_PATH) is C/wamp/moodledata_24/temp/backup/558ddee60068ee6442620f5d17a91d34/course_files/bullet-1.gif obviously I get an exception. I will try to understand how I can correct the problem (most probably I need to sanitize some paths in my code ?) but any light you can bring would help. Thanks.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello David,
          In fact this doesn't seems related at all to my changes in MDL-33424.
          Using an unmodified latest MOODLE_24_STABLE I am not able to restore any of my Moodle 1.9 backups that were restoring OK a few weeks ago.
          In all cases I get an exception (file_invalid_path) caused by the same line above
          Either there is something wrong that I need to correct on my setup (windows 7) or MDL-36977 has caused a regression somewhere.

          Show
          Jean-Michel Vedrine added a comment - Hello David, In fact this doesn't seems related at all to my changes in MDL-33424 . Using an unmodified latest MOODLE_24_STABLE I am not able to restore any of my Moodle 1.9 backups that were restoring OK a few weeks ago. In all cases I get an exception (file_invalid_path) caused by the same line above Either there is something wrong that I need to correct on my setup (windows 7) or MDL-36977 has caused a regression somewhere.
          Hide
          Jean-Michel Vedrine added a comment -

          Good news:

          I am not able to reproduce my yesterday problems when restoring 1.9 backups. Maybe it's because I was doing something stupid or ???

          Bad news:

          I have made more careful tests and in fact I have problems with images in 2 areas

          • text of questions answers in multichoice questions.
            This is because my code in write_answers is:
                            // Migrate images in answertext.
                            if ($answer['answerformat'] == FORMAT_HTML) {
                                $answer['answertext'] = $this->migrate_files($answer['answertext'], 'question', 'answer', $answer['id']);
                            }
                            // Migrate images in feedback.
                            if ($answer['feedbackformat'] == FORMAT_HTML) {
                                $answer['feedback'] = $this->migrate_files($answer['feedback'], 'question', 'answerfeedback', $answer['id']);
                            }
            

            This works for answer's feedback because in 1.9 backups their format is FORMAT_HTML but for answer's text this doesn't work because their format is not FORMAT_HTML but FORMAT_MOODLE. The problem is that I know a lot of teachers (including me) were including images here (either copying/pasting the url or writing it) so I think it is absolutely necessary to migrate files in answers texts of multichoice questions. But I need to be careful because a lot of other questions types use answers too.

          • combined feedback (correct, partially correct and incorrect) There is a bug in my code because I get broken images links

          Summary

          Need more work !

          Show
          Jean-Michel Vedrine added a comment - Good news: I am not able to reproduce my yesterday problems when restoring 1.9 backups. Maybe it's because I was doing something stupid or ??? Bad news: I have made more careful tests and in fact I have problems with images in 2 areas text of questions answers in multichoice questions. This is because my code in write_answers is: // Migrate images in answertext. if ($answer['answerformat'] == FORMAT_HTML) { $answer['answertext'] = $ this ->migrate_files($answer['answertext'], 'question', 'answer', $answer['id']); } // Migrate images in feedback. if ($answer['feedbackformat'] == FORMAT_HTML) { $answer['feedback'] = $ this ->migrate_files($answer['feedback'], 'question', 'answerfeedback', $answer['id']); } This works for answer's feedback because in 1.9 backups their format is FORMAT_HTML but for answer's text this doesn't work because their format is not FORMAT_HTML but FORMAT_MOODLE. The problem is that I know a lot of teachers (including me) were including images here (either copying/pasting the url or writing it) so I think it is absolutely necessary to migrate files in answers texts of multichoice questions. But I need to be careful because a lot of other questions types use answers too. combined feedback (correct, partially correct and incorrect) There is a bug in my code because I get broken images links Summary Need more work !
          Hide
          Jean-Michel Vedrine added a comment - - edited

          For multichoice combined feedback I understand the problem but don't yet have a solution
          My code in question/type/multichoice/backup/moodle1/lib.php:

                      $multichoice['correctfeedback'] = $this->migrate_files(
                              $multichoice['correctfeedback'], 'question', 'correctfeedback', $multichoice['id']);
                      $multichoice['partiallycorrectfeedback'] = $this->migrate_files(
                              $multichoice['partiallycorrectfeedback'], 'question', 'partiallycorrectfeedback', $multichoice['id']);
                      $multichoice['incorrectfeedback'] = $this->migrate_files(
                              $multichoice['incorrectfeedback'], 'question', 'incorrectfeedback', $multichoice['id']);
          

          is wrong because $multichoice['id'] has no meaning (it was just created a few lines above).
          In fact the last parameter of the 3 migrate_files calls should be the id of the multichoice question itself but I don't know how to get it at this point of the code in the write_multichoice function (apart of course the obvious solution to add it as a supplementary parameter when process_question call write_multichoice but isn't this an API change that would certainly preclude backporting this useful fix to stable branchs ?) I need some help.

          Show
          Jean-Michel Vedrine added a comment - - edited For multichoice combined feedback I understand the problem but don't yet have a solution My code in question/type/multichoice/backup/moodle1/lib.php: $multichoice['correctfeedback'] = $ this ->migrate_files( $multichoice['correctfeedback'], 'question', 'correctfeedback', $multichoice['id']); $multichoice['partiallycorrectfeedback'] = $ this ->migrate_files( $multichoice['partiallycorrectfeedback'], 'question', 'partiallycorrectfeedback', $multichoice['id']); $multichoice['incorrectfeedback'] = $ this ->migrate_files( $multichoice['incorrectfeedback'], 'question', 'incorrectfeedback', $multichoice['id']); is wrong because $multichoice ['id'] has no meaning (it was just created a few lines above). In fact the last parameter of the 3 migrate_files calls should be the id of the multichoice question itself but I don't know how to get it at this point of the code in the write_multichoice function (apart of course the obvious solution to add it as a supplementary parameter when process_question call write_multichoice but isn't this an API change that would certainly preclude backporting this useful fix to stable branchs ?) I need some help.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Well I have a solution but I am not proud of it. It can surely be improved. The idea is to migrate combined feedback files not in write_multichoice but in process question, doing

                  foreach ($data['multichoice'] as $key => $multichoice) {
                      $data['multichoice'][$key]['correctfeedback'] = $this->migrate_files(
                              $multichoice['correctfeedback'], 'question', 'correctfeedback', $data['id']);
                      $data['multichoice'][$key]['partiallycorrectfeedback'] = $this->migrate_files(
                              $multichoice['partiallycorrectfeedback'], 'question', 'partiallycorrectfeedback', $data['id']);
                      $data['multichoice'][$key]['incorrectfeedback'] = $this->migrate_files(
                              $multichoice['incorrectfeedback'], 'question', 'incorrectfeedback', $data['id']);
                  }
          

          Of course it works but it is not good looking : why is there a write_multichoice function to process multichoice options if we process multichoice combined feedback files in process question ? the code structuration is completely broken. there must be a better solution.

          Show
          Jean-Michel Vedrine added a comment - - edited Well I have a solution but I am not proud of it. It can surely be improved. The idea is to migrate combined feedback files not in write_multichoice but in process question, doing foreach ($data['multichoice'] as $key => $multichoice) { $data['multichoice'][$key]['correctfeedback'] = $ this ->migrate_files( $multichoice['correctfeedback'], 'question', 'correctfeedback', $data['id']); $data['multichoice'][$key]['partiallycorrectfeedback'] = $ this ->migrate_files( $multichoice['partiallycorrectfeedback'], 'question', 'partiallycorrectfeedback', $data['id']); $data['multichoice'][$key]['incorrectfeedback'] = $ this ->migrate_files( $multichoice['incorrectfeedback'], 'question', 'incorrectfeedback', $data['id']); } Of course it works but it is not good looking : why is there a write_multichoice function to process multichoice options if we process multichoice combined feedback files in process question ? the code structuration is completely broken. there must be a better solution.
          Hide
          Tim Hunt added a comment -

          I think you need to wait for David Mudrak to explain, because I don't know about this. Sorry.

          Show
          Tim Hunt added a comment - I think you need to wait for David Mudrak to explain, because I don't know about this. Sorry.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello,
          Yes I think we need David Mudrak help
          Sort summary for David:

          First problem

          Line 1768-1770 of backup/converter/moodle1/handlerlib.php my code

                          // Migrate images in answertext.
                          if ($answer['answerformat'] == FORMAT_HTML) {
                          $answer['answertext'] = $this->migrate_files($answer['answertext'], 'question', 'answer', $answer['id']);
          

          is not working for multichoice questions because answerformat is never equal to FORMAT_HTML for multichoice questions. But I am quite sure a lot ot teachers were pasting images links here for multichoice questions so IMHO we must migrate these images too.

          second problem

          The code for combined feedback in multichoice questions is wrong because it uses $data['id'] and that should be question id unfortunately question id is not available at this place of the code.

          Show
          Jean-Michel Vedrine added a comment - Hello, Yes I think we need David Mudrak help Sort summary for David: First problem Line 1768-1770 of backup/converter/moodle1/handlerlib.php my code // Migrate images in answertext. if ($answer['answerformat'] == FORMAT_HTML) { $answer['answertext'] = $ this ->migrate_files($answer['answertext'], 'question', 'answer', $answer['id']); is not working for multichoice questions because answerformat is never equal to FORMAT_HTML for multichoice questions. But I am quite sure a lot ot teachers were pasting images links here for multichoice questions so IMHO we must migrate these images too. second problem The code for combined feedback in multichoice questions is wrong because it uses $data ['id'] and that should be question id unfortunately question id is not available at this place of the code.
          Hide
          Jean-Michel Vedrine added a comment -

          To solve the first problem, I think that in fact we should change lines 1957-1962

                  // replay upgrade step 2010080901
                  if ($qtype !== 'multichoice') {
                      $new['answerformat'] = FORMAT_PLAIN;
                  } else {
                      $new['answerformat'] = FORMAT_MOODLE;
                  }
          

          of function convert_answer in backup/converters/moodle1/handlerlib.php so that answer's format for multichoice questions is FORMAT_HTML and no more FORMAT_MOODLE, because in my opinion FORMAT_MOODLE makes no sense as there was no editor for that field in Moodle 1.9 so teachers were most likely typing HTML tags and not markdown, isn't it ?
          So my suggestion is to change FORMAT_MOODLE to FORMAT_HTML and that would be enough to make migration of images working for answers without changing anything else.
          Tim and David, can you comment on that suggestion ?

          Show
          Jean-Michel Vedrine added a comment - To solve the first problem, I think that in fact we should change lines 1957-1962 // replay upgrade step 2010080901 if ($qtype !== 'multichoice') { $ new ['answerformat'] = FORMAT_PLAIN; } else { $ new ['answerformat'] = FORMAT_MOODLE; } of function convert_answer in backup/converters/moodle1/handlerlib.php so that answer's format for multichoice questions is FORMAT_HTML and no more FORMAT_MOODLE, because in my opinion FORMAT_MOODLE makes no sense as there was no editor for that field in Moodle 1.9 so teachers were most likely typing HTML tags and not markdown, isn't it ? So my suggestion is to change FORMAT_MOODLE to FORMAT_HTML and that would be enough to make migration of images working for answers without changing anything else. Tim and David, can you comment on that suggestion ?
          Hide
          David Mudrak added a comment -

          Sorry for the delay, just looking at it.

          Show
          David Mudrak added a comment - Sorry for the delay, just looking at it.
          Hide
          David Mudrak added a comment -

          Re: First problem

          I can see two possible solutions. Each of them having some consequences so it's really up to you to decide.

          • Call your $this->migrate_files() regardless the actual format of the text. Note that the underlying moodle1_converter::migrate_referenced_files() does not actually require HTML format. It just searches for URLs. All Moodle format, Markdown and even Plain text use full URLs so it should work well for them. On the other hand, if you keep the FORMAT_MOODLE, you will not have the TinyMCE available for editing these fields after the restore as it is used only for texts with FORMAT_HTML.
          • If you decide to change the format, you should also call text_to_html(). See for example what is done by moodle1_question_bank_handler::process_question()
            if ($CFG->texteditors !== 'textarea') {
                $data['questiontext'] = text_to_html($data['questiontext'], false, false, true);
                $data['questiontextformat'] = FORMAT_HTML;
                $data['generalfeedback'] = text_to_html($data['generalfeedback'], false, false, true);
                $data['generalfeedbackformat'] = FORMAT_HTML;
            }
            

            This may lead to some re-formatting but people would have TinyMCE available. The general approach to decide this was: look at how the given field looks like if it is created from scratch in 2.x. If TinyMCE is available there, people will just expect it in restored 1.9 courses, too. So it is better (read: less evil) to convert it to FORMAT_HTML. Markdown fans will complain, but there are not that many of us

          Re: Second problem

          Well, but the question id is available in moodle1_qtype_multichoice_handler::process_question() defined in question/type/multichoice/backup/moodle1/lib.php. So you can either put the image migration into that method, or extend the signature of moodle1_qtype_multichoice_handler::write_multichoice() so that it accepts it as the third parameter and the caller passes it.

          I do not consider this as an API change because the write_multichoice() is not part of the question type handler API, it is an implementation detail. Or are you aware of some add-on that would actually extend moodle1_qtype_multichoice_handler class?

          Show
          David Mudrak added a comment - Re: First problem I can see two possible solutions. Each of them having some consequences so it's really up to you to decide. Call your $this->migrate_files() regardless the actual format of the text. Note that the underlying moodle1_converter::migrate_referenced_files() does not actually require HTML format. It just searches for URLs. All Moodle format, Markdown and even Plain text use full URLs so it should work well for them. On the other hand, if you keep the FORMAT_MOODLE, you will not have the TinyMCE available for editing these fields after the restore as it is used only for texts with FORMAT_HTML. If you decide to change the format, you should also call text_to_html(). See for example what is done by moodle1_question_bank_handler::process_question() if ($CFG->texteditors !== 'textarea') { $data['questiontext'] = text_to_html($data['questiontext'], false , false , true ); $data['questiontextformat'] = FORMAT_HTML; $data['generalfeedback'] = text_to_html($data['generalfeedback'], false , false , true ); $data['generalfeedbackformat'] = FORMAT_HTML; } This may lead to some re-formatting but people would have TinyMCE available. The general approach to decide this was: look at how the given field looks like if it is created from scratch in 2.x. If TinyMCE is available there, people will just expect it in restored 1.9 courses, too. So it is better (read: less evil) to convert it to FORMAT_HTML. Markdown fans will complain, but there are not that many of us Re: Second problem Well, but the question id is available in moodle1_qtype_multichoice_handler::process_question() defined in question/type/multichoice/backup/moodle1/lib.php. So you can either put the image migration into that method, or extend the signature of moodle1_qtype_multichoice_handler::write_multichoice() so that it accepts it as the third parameter and the caller passes it. I do not consider this as an API change because the write_multichoice() is not part of the question type handler API, it is an implementation detail. Or are you aware of some add-on that would actually extend moodle1_qtype_multichoice_handler class?
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks a lot David, This perfectly answers all my questions !

          1. As in all supported Moodle versions TinYMCE is available for multichoice answers (it wasn't in Moodle 2.0) I think people expect to have it available in restored questions. So I will set format to FORMAT_HTML and use text_to_html.
          2. My only fear was that adding a third parameter to write_multichoice would be considered as an API change and would prevent backporting this to all stable branchs. But as you say this is not the case I will go this way because I think the code is better if image migration is part of write_multichoice.

          I will make the necessary changes, squash commits, test again and submit for peer review.

          Show
          Jean-Michel Vedrine added a comment - Thanks a lot David, This perfectly answers all my questions ! As in all supported Moodle versions TinYMCE is available for multichoice answers (it wasn't in Moodle 2.0) I think people expect to have it available in restored questions. So I will set format to FORMAT_HTML and use text_to_html. My only fear was that adding a third parameter to write_multichoice would be considered as an API change and would prevent backporting this to all stable branchs. But as you say this is not the case I will go this way because I think the code is better if image migration is part of write_multichoice. I will make the necessary changes, squash commits, test again and submit for peer review.
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to say I don't know any plugin that extend write_multichoice but maybe Tim does ?

          Show
          Jean-Michel Vedrine added a comment - I forgot to say I don't know any plugin that extend write_multichoice but maybe Tim does ?
          Hide
          Jean-Michel Vedrine added a comment -

          Added changes discussed with David. squashed commits. Done for all branchs.
          According to my tests all images are now correctly restored as non legacy images.
          I will slightly change testing instructions because

          • I think the restore part of the testing should be done in all branches to be sure this don't break anything on stable branchs.
          • for some images there is no editor in Moodle 1.9 (namely multichoice's answer text and match subquestions) I will make clear that for these fields the tester should use the "old trick" to copy/paste an HTML fragment from a field with an editor (the one that we were using at that time but is prohibited now in Moodle 2.x ).
          Show
          Jean-Michel Vedrine added a comment - Added changes discussed with David. squashed commits. Done for all branchs. According to my tests all images are now correctly restored as non legacy images. I will slightly change testing instructions because I think the restore part of the testing should be done in all branches to be sure this don't break anything on stable branchs. for some images there is no editor in Moodle 1.9 (namely multichoice's answer text and match subquestions) I will make clear that for these fields the tester should use the "old trick" to copy/paste an HTML fragment from a field with an editor (the one that we were using at that time but is prohibited now in Moodle 2.x ).
          Hide
          Jean-Michel Vedrine added a comment -

          Putting in pear review as all seems to be working now. I hope I didn't forget anything !

          Show
          Jean-Michel Vedrine added a comment - Putting in pear review as all seems to be working now. I hope I didn't forget anything !
          Hide
          David Mudrak added a comment -

          Mike Delta Lima Tree Tree Fower Too Fower, you are clear to land!

          Thanks Jean-Michel! Nice and clean patch. +1 from me.

          Show
          David Mudrak added a comment - Mike Delta Lima Tree Tree Fower Too Fower, you are clear to land! Thanks Jean-Michel! Nice and clean patch. +1 from me.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks David,
          Tim can you send this to integration, if you think it's ready ?

          Show
          Jean-Michel Vedrine added a comment - Thanks David, Tim can you send this to integration, if you think it's ready ?
          Hide
          Tim Hunt added a comment -

          Pushing the integration button, since Jean-Michel does not have permission.

          Show
          Tim Hunt added a comment - Pushing the integration button, since Jean-Michel does not have permission.
          Hide
          Damyon Wiese added a comment -

          Just a note:

          It would have been nicer if there was a 1.9 backup file attached that could be used for testing this issue.

          Show
          Damyon Wiese added a comment - Just a note: It would have been nicer if there was a 1.9 backup file attached that could be used for testing this issue.
          Hide
          Damyon Wiese added a comment -

          Thanks Jean-Michel,

          This patch looks good to me, thanks for working on this issue. Also thanks for posting updates in the comments on this issue - it helped me a lot when reviewing the solution.

          I've integrated this for master, 24 and 23.

          Show
          Damyon Wiese added a comment - Thanks Jean-Michel, This patch looks good to me, thanks for working on this issue. Also thanks for posting updates in the comments on this issue - it helped me a lot when reviewing the solution. I've integrated this for master, 24 and 23.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Damyon,
          Sorry I didn't had the idea to attach a sample 1.9 backup. I will do it today.
          But given backup/restore is a complex area of Moodle, I would feel better if the tester can make sure this is working as expected on another system than mine !
          Also I just realised that testing instructions are missing a test for the migration of the old "image field in 1.9 question. I will add it and my sample test file will also include this case.

          Show
          Jean-Michel Vedrine added a comment - Hello Damyon, Sorry I didn't had the idea to attach a sample 1.9 backup. I will do it today. But given backup/restore is a complex area of Moodle, I would feel better if the tester can make sure this is working as expected on another system than mine ! Also I just realised that testing instructions are missing a test for the migration of the old "image field in 1.9 question. I will add it and my sample test file will also include this case.
          Hide
          Jean-Michel Vedrine added a comment -

          Attaching a small sample Moodle 1.9 backup with questions including images in all fields that are handled by MDL-33424

          Show
          Jean-Michel Vedrine added a comment - Attaching a small sample Moodle 1.9 backup with questions including images in all fields that are handled by MDL-33424
          Hide
          Jason Fowler added a comment -

          Thanks for this Jean-Michel, it works nicely!

          Show
          Jason Fowler added a comment - Thanks for this Jean-Michel, it works nicely!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

            People

            • Votes:
              13 Vote for this issue
              Watchers:
              22 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: