Moodle
  1. Moodle
  2. MDL-37934

Blackboard_six question import fails if the same image is used several times in the same question field

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.7, 2.3.4, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2, 2.5
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      Reproduce the bug in any MOODLE_23_STABLE MOODLE_24_STABLE, master moodle install where this fix is not applied, trying to import the file attached to this issue MDL-37934_testfile.zip using the Blackboard V6+ question import format should produce an error message similar to:
      Can not create file "5/user/draft/994659423///ppg_ppg0211132247_f1q1g1.jpg"
      Now trying to import the same file with the fix applied, one question should be imported without any error message displayed. The resulting question should have the same image once in the question text and twice in the same answer. The image is a small circle (degree sign).
      As the fix is very short and very simple I don't think additional tests are required.

      Show
      Reproduce the bug in any MOODLE_23_STABLE MOODLE_24_STABLE, master moodle install where this fix is not applied, trying to import the file attached to this issue MDL-37934 _testfile.zip using the Blackboard V6+ question import format should produce an error message similar to: Can not create file "5/user/draft/994659423///ppg_ ppg0211132247 _f1q1g1.jpg" Now trying to import the same file with the fix applied, one question should be imported without any error message displayed. The resulting question should have the same image once in the question text and twice in the same answer. The image is a small circle (degree sign). As the fix is very short and very simple I don't think additional tests are required.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      See https://moodle.org/mod/forum/discuss.php?d=191506&parent=964908
      the code is failing if the same image is used several times in the same field of the same question because in store_file_for_text_field function create_file_from_pathname is called several times with the exact same parameters.
      The solution is to check if file exists before trying to create it.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jean-Michel Vedrine added a comment - - edited

            My current solution is to add a check :

             if (!$fs->file_exists(context_user::instance($USER->id)->id, 'user', 'draft', $text['itemid'], '/', $newfilename)) {
                        $fs->create_file_from_pathname($filerecord, $tempdir . '/' . $filepathinsidetempdir . '/' . $filename);
                    }
            

            But the same user that send me the zip archive that failed to import using blackboard_six has also send me the .xml file generated by Gary Blackburn's Moodle XMLbuilder and I was quite surprised to see this file too fails to import using Moodle XML import format. This is because it contains
            <text><![CDATA[some text...<img align="middle" height="20" width="9" src="@@PLUGINFILE@@/u8728.jpg" />some text...<img align="middle" height="20" width="9" src="@@PLUGINFILE@@/u8728.jpg" />some text ...]]></text>
            <file name="u8728.jpg" encoding="base64"> ...
            </file>
            <file name="u8728.jpg" encoding="base64"> ...
            </file>
            Should we consider this as a malformed Moodle XML file and not do anything about it (Moodle would certainly not generate such a file I suppose) or should we also modify Moodle XML import format to correctly import such a file ?

            Show
            Jean-Michel Vedrine added a comment - - edited My current solution is to add a check : if (!$fs->file_exists(context_user::instance($USER->id)->id, 'user', 'draft', $text['itemid'], '/', $newfilename)) { $fs->create_file_from_pathname($filerecord, $tempdir . '/' . $filepathinsidetempdir . '/' . $filename); } But the same user that send me the zip archive that failed to import using blackboard_six has also send me the .xml file generated by Gary Blackburn's Moodle XMLbuilder and I was quite surprised to see this file too fails to import using Moodle XML import format. This is because it contains <text><![CDATA [some text...<img align="middle" height="20" width="9" src="@@PLUGINFILE@@/u8728.jpg" />some text...<img align="middle" height="20" width="9" src="@@PLUGINFILE@@/u8728.jpg" />some text ...] ]></text> <file name="u8728.jpg" encoding="base64"> ... </file> <file name="u8728.jpg" encoding="base64"> ... </file> Should we consider this as a malformed Moodle XML file and not do anything about it (Moodle would certainly not generate such a file I suppose) or should we also modify Moodle XML import format to correctly import such a file ?
            Hide
            Tim Hunt added a comment -

            I think that we should consider that to be a mal-formed XML file.

            However, the question is then, what to do about it?

            1. give an error. This is what happens at the moment. Does the error message make it clear what the problem is? If so, we could improve the error message.

            2. make the code handle it anyway, but output a debugging(..., DEBUG_DEVELOPER) warning to point out the problem.

            If you don't mind writing the code, I think 2. is better.

            Show
            Tim Hunt added a comment - I think that we should consider that to be a mal-formed XML file. However, the question is then, what to do about it? 1. give an error. This is what happens at the moment. Does the error message make it clear what the problem is? If so, we could improve the error message. 2. make the code handle it anyway, but output a debugging(..., DEBUG_DEVELOPER) warning to point out the problem. If you don't mind writing the code, I think 2. is better.
            Hide
            Jean-Michel Vedrine added a comment -
            1. Currently the error message is Can not create file "xxxxx.jpg" so maybe not very clear about the problem. If developer debugging is activated we get additional details Debug info: Duplicate entry '491806aa3afcbefc8a05023aa26c25f5c5882899' for key 'mdl_file_pat_uix' and the stack trace
              line 1181 of \lib\filestorage\file_storage.php: stored_file_creation_exception thrown
              line 182 of \question\format\xml\format.php: call to file_storage->create_file_from_string()
              line 159 of \question\format\xml\format.php: call to qformat_xml->import_files_as_draft()
              line 279 of \question\format\xml\format.php: call to qformat_xml->import_text_with_files()
              line 425 of \question\format\xml\format.php: call to qformat_xml->import_answer()
              line 906 of \question\format\xml\format.php: call to qformat_xml->import_multichoice()
              line 303 of \question\format.php: call to qformat_xml->readquestions()
              line 119 of \question\import.php: call to qformat_default->importprocess()
              Which makes the problem more obvious to diagnoze.
              If we test with a old version of Moodle where using draft files in XML import is not implemented like Moodle 2.2 the error displayed is exactly the same, only the stack trace is different.
            2. Yes I think you are right. I will create a patch. Maybe it would be better to just fix blackboard_six in this issue and create a separate issue for XML because even if the same forum thread makes us aware of both problems this is not quite the same issue: in blackboard_six the code is parsing the text to find images references so not looking at duplicates images is an authentic bug, but in XML this is not really a bug, just better handling of some incorrect files.
            Show
            Jean-Michel Vedrine added a comment - Currently the error message is Can not create file "xxxxx.jpg" so maybe not very clear about the problem. If developer debugging is activated we get additional details Debug info: Duplicate entry '491806aa3afcbefc8a05023aa26c25f5c5882899' for key 'mdl_file_pat_uix' and the stack trace line 1181 of \lib\filestorage\file_storage.php: stored_file_creation_exception thrown line 182 of \question\format\xml\format.php: call to file_storage->create_file_from_string() line 159 of \question\format\xml\format.php: call to qformat_xml->import_files_as_draft() line 279 of \question\format\xml\format.php: call to qformat_xml->import_text_with_files() line 425 of \question\format\xml\format.php: call to qformat_xml->import_answer() line 906 of \question\format\xml\format.php: call to qformat_xml->import_multichoice() line 303 of \question\format.php: call to qformat_xml->readquestions() line 119 of \question\import.php: call to qformat_default->importprocess() Which makes the problem more obvious to diagnoze. If we test with a old version of Moodle where using draft files in XML import is not implemented like Moodle 2.2 the error displayed is exactly the same, only the stack trace is different. Yes I think you are right. I will create a patch. Maybe it would be better to just fix blackboard_six in this issue and create a separate issue for XML because even if the same forum thread makes us aware of both problems this is not quite the same issue: in blackboard_six the code is parsing the text to find images references so not looking at duplicates images is an authentic bug, but in XML this is not really a bug, just better handling of some incorrect files.
            Hide
            Tim Hunt added a comment -

            You are right. Separate tracker issues would be better. Thank you.

            Show
            Tim Hunt added a comment - You are right. Separate tracker issues would be better. Thank you.
            Hide
            Jean-Michel Vedrine added a comment -

            Rather than using file_exists I used an array to remember all images already imported in the same field. Tghis should be faster and not add any db query.

            Show
            Jean-Michel Vedrine added a comment - Rather than using file_exists I used an array to remember all images already imported in the same field. Tghis should be faster and not add any db query.
            Hide
            Jean-Michel Vedrine added a comment -

            Putting in peer review. I will create the issue for XML format.

            Show
            Jean-Michel Vedrine added a comment - Putting in peer review. I will create the issue for XML format.
            Hide
            Jean-Michel Vedrine added a comment -

            Linking Blackboard_six and XML issues

            Show
            Jean-Michel Vedrine added a comment - Linking Blackboard_six and XML issues
            Hide
            Tim Hunt added a comment -

            That looks good to me. What branches should this apply to? I assume all stable.

            Also, we need testing instructions, then this can be submitted for integration.

            Show
            Tim Hunt added a comment - That looks good to me. What branches should this apply to? I assume all stable. Also, we need testing instructions, then this can be submitted for integration.
            Hide
            Jean-Michel Vedrine added a comment - - edited

            Thanks Tim, will do that.
            Yes I think this should be applied to all supported branchs.

            Show
            Jean-Michel Vedrine added a comment - - edited Thanks Tim, will do that. Yes I think this should be applied to all supported branchs.
            Hide
            Jean-Michel Vedrine added a comment -

            Creating branchs for 238STABLE and 24_STABLE branchs

            Show
            Jean-Michel Vedrine added a comment - Creating branchs for 238STABLE and 24_STABLE branchs
            Hide
            Jean-Michel Vedrine added a comment -

            Small test file with a single multichoice question.

            Show
            Jean-Michel Vedrine added a comment - Small test file with a single multichoice question.
            Hide
            Jean-Michel Vedrine added a comment -

            Testing instructions added.
            One thing caught my eye: the commit message is too long! Unfortunately I spotted this after doing the 3 branchs. Grr! too late to do that now will do it tomorrow.

            Show
            Jean-Michel Vedrine added a comment - Testing instructions added. One thing caught my eye: the commit message is too long! Unfortunately I spotted this after doing the 3 branchs. Grr! too late to do that now will do it tomorrow.
            Hide
            Tim Hunt added a comment -

            Thanks Jean-Michel. I am going to submit for integration, and trust you to fix the commit comment before next week.

            Show
            Tim Hunt added a comment - Thanks Jean-Michel. I am going to submit for integration, and trust you to fix the commit comment before next week.
            Hide
            Jean-Michel Vedrine added a comment -

            Hello Tim, just to let you know I shortened the commit comment on all branchs.

            Show
            Jean-Michel Vedrine added a comment - Hello Tim, just to let you know I shortened the commit comment on all branchs.
            Hide
            Damyon Wiese added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            Thanks!

            Show
            Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
            Hide
            Dan Poltawski added a comment -

            Integrated to master, 24 and 23. Thanks a lot Jean-Michel

            Show
            Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks a lot Jean-Michel
            Hide
            Jean-Michel Vedrine added a comment -

            Thanks Dan, sorry for the typo in the branch names that I copied/pasted in all of them

            Show
            Jean-Michel Vedrine added a comment - Thanks Dan, sorry for the typo in the branch names that I copied/pasted in all of them
            Hide
            Frédéric Massart added a comment -

            Passing, thanks!

            Show
            Frédéric Massart added a comment - Passing, thanks!
            Hide
            Damyon Wiese added a comment -

            Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

            Regards, Damyon

            Show
            Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: