Moodle
  1. Moodle
  2. MDL-26892

Question images lost during upgrade

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.2.3
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. As administrator in Moodle 1.9.x, Create a Course
      2. Upload an Image to Site Files
      3. Add a question to the Default System Category
      4. Select the file uploaded
      5. Preview to see the question and image
      6. Backup Database and Moodle Data
      7. Complete Upgrade to Moodle 2.2.3
      8. Preview the question again and verify the image is still present, and now stored according to the File API.
      Show
      As administrator in Moodle 1.9.x, Create a Course Upload an Image to Site Files Add a question to the Default System Category Select the file uploaded Preview to see the question and image Backup Database and Moodle Data Complete Upgrade to Moodle 2.2.3 Preview the question again and verify the image is still present, and now stored according to the File API.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Rank:
      16519

      Description

      We upgraded from Moodle 1.9 to 2.0.2.

      Images in all questions are lost. There used to be a filed in mdl_question table called 'image'. Now that filed is removed.

      I am unsure if some alternative arrangements were made before removing that filed or not.

      1. 0001-MDL-26892-Fixed-quiz-image-for-system-context.patch
        1 kB
        Chris Edwards
      2. mdl-26892.patch
        0.6 kB
        Chris Edwards
      1. Question Preview in 1.9.18.jpg
        48 kB
      2. screenshot-2.jpg
        88 kB
      3. screenshot-3.jpg
        74 kB

        Issue Links

          Activity

          Hide
          jai gupta added a comment - - edited

          On some troubleshooting we found that code inside if statement never gets executed.

          File: moodle/lib/db/upgrade.php
          Line: 5045
          Code: if ($image = $fs->get_file($context->id, 'course', 'legacy', 0, $filepath, $filename)) {

          As an temporary workaround we were able to add HTML image tags after commenting out if and related statements.

          $question->questiontext .= ' <img src= . $CFG->wwwroot . '/file.php/' . $courseid . $filepath . $filename . '" />';

          Show
          jai gupta added a comment - - edited On some troubleshooting we found that code inside if statement never gets executed. File: moodle/lib/db/upgrade.php Line: 5045 Code: if ($image = $fs->get_file($context->id, 'course', 'legacy', 0, $filepath, $filename)) { As an temporary workaround we were able to add HTML image tags after commenting out if and related statements. $question->questiontext .= ' <img src= . $CFG->wwwroot . '/file.php/' . $courseid . $filepath . $filename . '" />';
          Hide
          Tim Hunt added a comment -

          What the code is trying to do is only keep the link if the image being linked to really exists. That seems a sensible idea to me, even if something is going wrong at the moment.

          And the system basically works, at least it did when we tested it, so there must be something about your Moodle 1.9 site that triggers the problem. We need to try to work out what that is.

          Your proposed fix is definitely wrong. file.php should not be used in Moodle 2+.

          I'm adding Dongsheng as a watcher since he originally implemented this.

          Show
          Tim Hunt added a comment - What the code is trying to do is only keep the link if the image being linked to really exists. That seems a sensible idea to me, even if something is going wrong at the moment. And the system basically works, at least it did when we tested it, so there must be something about your Moodle 1.9 site that triggers the problem. We need to try to work out what that is. Your proposed fix is definitely wrong. file.php should not be used in Moodle 2+. I'm adding Dongsheng as a watcher since he originally implemented this.
          Hide
          jai gupta added a comment -

          My intention was not to propose a fix, I was only describing what I did as an temporary workaround to view question images.

          I can confirm that images do exist, I can view them using legacy course files.

          I have tried upgrading multiple times. I have also tried to first upgrade Moodle 1.9.5 to latest 1.9 and then upgrading to 2.0. All results in missing images in questions.

          Please let me know if there is any information you require from me.

          Show
          jai gupta added a comment - My intention was not to propose a fix, I was only describing what I did as an temporary workaround to view question images. I can confirm that images do exist, I can view them using legacy course files. I have tried upgrading multiple times. I have also tried to first upgrade Moodle 1.9.5 to latest 1.9 and then upgrading to 2.0. All results in missing images in questions. Please let me know if there is any information you require from me.
          Hide
          Sebastian Berm added a comment -

          We've had major issues with this.
          For now, I've created a hack in /file.php allowing the usage of the old and the new system. As no one has been able to do something about this yet.
          (I backported some parts of the old Moodle filelib to Moodle 2 and did some path tricks to get it working, note, I am absolutely not following any kind of coding guidelines with this hack...)

          Would it be interesting for everyone if I were to create a patch file for this?
          Please note, we're still talking about a temporary hack, instead of an solution...

          Show
          Sebastian Berm added a comment - We've had major issues with this. For now, I've created a hack in /file.php allowing the usage of the old and the new system. As no one has been able to do something about this yet. (I backported some parts of the old Moodle filelib to Moodle 2 and did some path tricks to get it working, note, I am absolutely not following any kind of coding guidelines with this hack...) Would it be interesting for everyone if I were to create a patch file for this? Please note, we're still talking about a temporary hack, instead of an solution...
          Hide
          Pierre Pichet added a comment -

          Tim,
          Is this bug has not been resolved elsewhere i.e. when resolving other bugs?

          Show
          Pierre Pichet added a comment - Tim, Is this bug has not been resolved elsewhere i.e. when resolving other bugs?
          Hide
          Tim Hunt added a comment -

          I don't think so. I think this is still a mystery.

          What we need is a reproducible test case, something like:

          1. Install Moodle 1.9.
          2. Set up a question exactly like ... (this is where we need the details filled in).
          3. Upgrade to 2.0.x
          4. Observe that the images have gone.

          Given that, there is probably an easy fix. Without that, we can't do anything.

          Show
          Tim Hunt added a comment - I don't think so. I think this is still a mystery. What we need is a reproducible test case, something like: 1. Install Moodle 1.9. 2. Set up a question exactly like ... (this is where we need the details filled in). 3. Upgrade to 2.0.x 4. Observe that the images have gone. Given that, there is probably an easy fix. Without that, we can't do anything.
          Hide
          Sebastian Berm added a comment -

          I will ask if someone here can provide us with this information.
          However I am not entirely sure if I can provide the correct way, because the environment this happened on is rather big.

          What I did notice is this only seems to happen on bigger environments. But it doesn't seem to be a memory issue or other fatal error because the migration is being completed. Will ask a colleague for more information.

          Show
          Sebastian Berm added a comment - I will ask if someone here can provide us with this information. However I am not entirely sure if I can provide the correct way, because the environment this happened on is rather big. What I did notice is this only seems to happen on bigger environments. But it doesn't seem to be a memory issue or other fatal error because the migration is being completed. Will ask a colleague for more information.
          Hide
          Chris Edwards added a comment -

          I encountered this in upgrading a 1.9.18 stable branch install to 2.2.3 stable branch. In this case, the system in question is used to host quizzes that are used across courses through system quiz categories and the images were being held in the site files. During the upgrade, I found reference to the section of lib/db/upgrade.php in MDL-32583. What I had to do to get the code to append the image was find the right context that was used in the migration to legacy files. I'll attach my patch that successfully migrated our images.

          Show
          Chris Edwards added a comment - I encountered this in upgrading a 1.9.18 stable branch install to 2.2.3 stable branch. In this case, the system in question is used to host quizzes that are used across courses through system quiz categories and the images were being held in the site files. During the upgrade, I found reference to the section of lib/db/upgrade.php in MDL-32583 . What I had to do to get the code to append the image was find the right context that was used in the migration to legacy files. I'll attach my patch that successfully migrated our images.
          Hide
          Chris Edwards added a comment -

          Fix for System Quiz Category Images in migration from image field to inline in questiontext

          Show
          Chris Edwards added a comment - Fix for System Quiz Category Images in migration from image field to inline in questiontext
          Hide
          Tim Hunt added a comment -

          Great! Thanks. Now I just need to find time to review the patch.

          Show
          Tim Hunt added a comment - Great! Thanks. Now I just need to find time to review the patch.
          Hide
          Chris Edwards added a comment -

          Sorry for the update once again, but I'm uploading a git formatted patch against MOODLE_22_STABLE.

          Show
          Chris Edwards added a comment - Sorry for the update once again, but I'm uploading a git formatted patch against MOODLE_22_STABLE.
          Hide
          Chris Edwards added a comment -

          This is what an example question looks like in 1.9.18

          Show
          Chris Edwards added a comment - This is what an example question looks like in 1.9.18
          Hide
          Chris Edwards added a comment -

          A preview of the question after a direct upgrade to 2.2.3

          Show
          Chris Edwards added a comment - A preview of the question after a direct upgrade to 2.2.3
          Hide
          Chris Edwards added a comment -

          A preview of the question after an upgrade with my patch applied on MOODLE_22_STABLE

          Show
          Chris Edwards added a comment - A preview of the question after an upgrade with my patch applied on MOODLE_22_STABLE
          Hide
          Chris Edwards added a comment -

          Steps I took to reproduce:

          1. Clean Install of Moodle 1.9.18
          2. Log in as Administrator
          3. Create a Course
          4. Upload an Image to Site Files
          5. Add a question to the Default System Category
          6. Select the file uploaded
          7. Preview to see the question and image
          8. Backup Database and Moodle Data
          9. Complete Upgrade to Moodle 2.2.3
          10. Preview the question again and the image is gone.

          Proof the patch works:
          11. Restore Database and Moodle Data
          12. Apply patch and re-upgrade
          13. Preview the question, and image is there

          Show
          Chris Edwards added a comment - Steps I took to reproduce: 1. Clean Install of Moodle 1.9.18 2. Log in as Administrator 3. Create a Course 4. Upload an Image to Site Files 5. Add a question to the Default System Category 6. Select the file uploaded 7. Preview to see the question and image 8. Backup Database and Moodle Data 9. Complete Upgrade to Moodle 2.2.3 10. Preview the question again and the image is gone. Proof the patch works: 11. Restore Database and Moodle Data 12. Apply patch and re-upgrade 13. Preview the question, and image is there
          Hide
          Tim Hunt added a comment -

          Thanks, look like you are doing everything you can to make it easy for me, which is good. Now, if you could just arrange for there to be 26 hours in every day ...

          Show
          Tim Hunt added a comment - Thanks, look like you are doing everything you can to make it easy for me, which is good. Now, if you could just arrange for there to be 26 hours in every day ...
          Hide
          Chris Edwards added a comment -

          Tim, I would if I could! I also tested creating a question in a category and confirmed that it behaved the same as my testing for questions in the system context, so this patch corrects questions that are not upgraded properly that reside in the category context. Category questions in 1.9 also pulled their images from the site folder, so this makes sense. Additionally, the upgrade code backs that up by falling through to the system context in the switch case I patched. I took screenshots of that as well and will upload them if you wish.

          I suspect that the original issue was that the question resided in either the category or system context and the image was held in the site folder. For what it is worth, I tested a course uploaded image in a question as well and it was not affected by the patch – both patched and unpatched installs successfully upgrade course level questions.

          Show
          Chris Edwards added a comment - Tim, I would if I could! I also tested creating a question in a category and confirmed that it behaved the same as my testing for questions in the system context, so this patch corrects questions that are not upgraded properly that reside in the category context. Category questions in 1.9 also pulled their images from the site folder, so this makes sense. Additionally, the upgrade code backs that up by falling through to the system context in the switch case I patched. I took screenshots of that as well and will upload them if you wish. I suspect that the original issue was that the question resided in either the category or system context and the image was held in the site folder. For what it is worth, I tested a course uploaded image in a question as well and it was not affected by the patch – both patched and unpatched installs successfully upgrade course level questions.
          Hide
          Tim Hunt added a comment -

          I see. You have correctly diagnosed the problem - we should be looking for the files in the front-page context, not the system context.

          However, I think your proposed solution is wrong. The id of the front-page course is not the system context id. It is the front-page course id. (Co-incidentally, these ids may both be 1 on your server.)

          I will make an amended commit.

          Show
          Tim Hunt added a comment - I see. You have correctly diagnosed the problem - we should be looking for the files in the front-page context, not the system context. However, I think your proposed solution is wrong. The id of the front-page course is not the system context id. It is the front-page course id. (Co-incidentally, these ids may both be 1 on your server.) I will make an amended commit.
          Hide
          Tim Hunt added a comment -

          I think this patch: https://github.com/timhunt/moodle/compare/MOODLE_22_STABLE...MDL-26892_22 is right, but I don't have any real way to test it.

          Chris, can you confirm that on your server, get_site()>id is the same as $system_context>id and hence that your previous testing is still valid for my patch. Thanks.

          Show
          Tim Hunt added a comment - I think this patch: https://github.com/timhunt/moodle/compare/MOODLE_22_STABLE...MDL-26892_22 is right, but I don't have any real way to test it. Chris, can you confirm that on your server, get_site() >id is the same as $system_context >id and hence that your previous testing is still valid for my patch. Thanks.
          Hide
          Chris Edwards added a comment -

          That explains it then and your patch makes more sense. Yes, both ids are the same on both the server that I upgraded and on the test install that I did for testing and the patch worked the same for me. I guess it is just a coincidence that the two ids are the same. Probably my unfamiliarity with parts of the code. Thanks for double checking it!

          Show
          Chris Edwards added a comment - That explains it then and your patch makes more sense. Yes, both ids are the same on both the server that I upgraded and on the test install that I did for testing and the patch worked the same for me. I guess it is just a coincidence that the two ids are the same. Probably my unfamiliarity with parts of the code. Thanks for double checking it!
          Hide
          Tim Hunt added a comment -

          David, I think you understand 1.9 -> 2.x restore code. Do we need to fix the restore code in a similar way to this upgrade code fix?

          Show
          Tim Hunt added a comment - David, I think you understand 1.9 -> 2.x restore code. Do we need to fix the restore code in a similar way to this upgrade code fix?
          Hide
          David Mudrak added a comment -

          Hmm, seems that I was simply ignoring this case when writing moodle1_question_bank_handler class. Looking at https://github.com/moodle/moodle/blob/MOODLE_22_STABLE/backup/converter/moodle1/handlerlib.php#L1191 it seems that the moodle1 converter does not even try to look for the file in the site_files folder within the ZIP backup. So yes, moodle1 converter code needs to be fixed, too.

          Unfortunately I can't work on this personally right now. If you think the restore part can be solved in a separate issue, feel free to create one assigned to moodle.com. Our STABLE team could work on it after 2.3 release. Thanks.

          Show
          David Mudrak added a comment - Hmm, seems that I was simply ignoring this case when writing moodle1_question_bank_handler class. Looking at https://github.com/moodle/moodle/blob/MOODLE_22_STABLE/backup/converter/moodle1/handlerlib.php#L1191 it seems that the moodle1 converter does not even try to look for the file in the site_files folder within the ZIP backup. So yes, moodle1 converter code needs to be fixed, too. Unfortunately I can't work on this personally right now. If you think the restore part can be solved in a separate issue, feel free to create one assigned to moodle.com. Our STABLE team could work on it after 2.3 release. Thanks.
          Hide
          Tim Hunt added a comment -

          OK, so it looks like we should submit this fix for integration. I opened MDL-33817 for the restore issue.

          to INTEGRATORS, please cherry-pick the fix to 2.1. Thanks.

          Show
          Tim Hunt added a comment - OK, so it looks like we should submit this fix for integration. I opened MDL-33817 for the restore issue. to INTEGRATORS, please cherry-pick the fix to 2.1. Thanks.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Tim

          Show
          Dan Poltawski added a comment - Integrated, thanks Tim
          Hide
          Ankit Agarwal added a comment -

          Tested this on a 1.9.x-2.2.x upgrade.
          Working fine.
          passing the issue
          thanks

          Show
          Ankit Agarwal added a comment - Tested this on a 1.9.x-2.2.x upgrade. Working fine. passing the issue thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi!

          I wrote the solution that David Mudrak suggest on MDL-33424

          Thanks all for your great work!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi! I wrote the solution that David Mudrak suggest on MDL-33424 Thanks all for your great work!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: