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:

      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.

        Gliffy Diagrams

        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: