Moodle
  1. Moodle
  2. MDL-37967

Improve XML question import format handling of duplicated files

    Details

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

      Verify that without this patch trying to import the attached sample XML file produce the error message "Can not create file" followed by the name of a draftfile ending in moodle_logo_small.png and import fails.

      With this patch applied file is imported but a developper debugging message is displayed :
      Duplicate file in XML: moodle_logo_small.png

      line 177 of \question\format\xml\format.php: call to debugging()
      line 159 of \question\format\xml\format.php: call to qformat_xml->import_files_as_draft()
      line 214 of \question\format\xml\format.php: call to qformat_xml->import_text_with_files()
      line 513 of \question\format\xml\format.php: call to qformat_xml->import_headers()
      line 915 of \question\format\xml\format.php: call to qformat_xml->import_truefalse()
      line 303 of \question\format.php: call to qformat_xml->readquestions()
      line 119 of \question\import.php: call to qformat_default->importprocess()

      After the import verify that a working true/false question has been correctly imported with twice the same small Moodle logo included in question text.

      Show
      Verify that without this patch trying to import the attached sample XML file produce the error message "Can not create file" followed by the name of a draftfile ending in moodle_logo_small.png and import fails. With this patch applied file is imported but a developper debugging message is displayed : Duplicate file in XML: moodle_logo_small.png line 177 of \question\format\xml\format.php: call to debugging() line 159 of \question\format\xml\format.php: call to qformat_xml->import_files_as_draft() line 214 of \question\format\xml\format.php: call to qformat_xml->import_text_with_files() line 513 of \question\format\xml\format.php: call to qformat_xml->import_headers() line 915 of \question\format\xml\format.php: call to qformat_xml->import_truefalse() line 303 of \question\format.php: call to qformat_xml->readquestions() line 119 of \question\import.php: call to qformat_default->importprocess() After the import verify that a working true/false question has been correctly imported with twice the same small Moodle logo included in question text.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      47742

      Description

      During working on MDL-37934 it was discovered that if an XML contains the same file several times for the same field of the same question an error message not very explicit is displayed and the import abort for the whole file.
      This issue is about a better handling of this issue that can only happen with XML files generated by external system like MoodleXMLBuilder not with files generated by Moodle question XML export.

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          Of course we can use file_exists to test file existence before trying to create it but I am a little worried this will add a db query for each file.

          Show
          Jean-Michel Vedrine added a comment - Of course we can use file_exists to test file existence before trying to create it but I am a little worried this will add a db query for each file.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, please can you attach the example file here, so it is easy to find.

          I cannot see why using file_exists would add a DB query, but I must be missing something.

          Show
          Tim Hunt added a comment - Jean-Michel, please can you attach the example file here, so it is easy to find. I cannot see why using file_exists would add a DB query, but I must be missing something.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim, the problem is that file send by user is made of questions actually in use in her school, so she doesn't want it to be posted.
          I will try to reproduce the problem using MoodleXMLBuilder to create the XML file with the default so that we have a sample file.
          Maybe I am wrong in thinking that using file_exists would add a db query ?
          I was thinking that file_exists call file_exists_by_hash which in turn call $DB->record_exists so it would means one db query by file.
          But I must admit I am not familiar with filestorage lib so maybe this reasoning is wrong ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, the problem is that file send by user is made of questions actually in use in her school, so she doesn't want it to be posted. I will try to reproduce the problem using MoodleXMLBuilder to create the XML file with the default so that we have a sample file. Maybe I am wrong in thinking that using file_exists would add a db query ? I was thinking that file_exists call file_exists_by_hash which in turn call $DB->record_exists so it would means one db query by file. But I must admit I am not familiar with filestorage lib so maybe this reasoning is wrong ?
          Hide
          Tim Hunt added a comment -

          Re: sample file. I guess you will have to make one somehow. Sorry.

          Re file_exists: are we talking about the standard file_exists PHP function? that just talks to the file system. No DB queries. Or are we talking about something else?

          Show
          Tim Hunt added a comment - Re: sample file. I guess you will have to make one somehow. Sorry. Re file_exists: are we talking about the standard file_exists PHP function? that just talks to the file system. No DB queries. Or are we talking about something else?
          Hide
          Jean-Michel Vedrine added a comment -

          No I was talking of filestorage class file_exists method to see if the same file was already stored in Moodle file system.
          Maybe "That's not the way to do it" as would Mister Punch said ?

          Show
          Jean-Michel Vedrine added a comment - No I was talking of filestorage class file_exists method to see if the same file was already stored in Moodle file system. Maybe "That's not the way to do it" as would Mister Punch said ?
          Hide
          Tim Hunt added a comment -

          Ah. now I understand the question about file_exists.

          I don't understand why that is necessary. The import creates a new question, so we know exactly which files are in each file area - the ones we just put there. Therefore, it should be possible for the XML import code to track the list of files itself, just using an array.

          Unless I am missing something.

          Show
          Tim Hunt added a comment - Ah. now I understand the question about file_exists. I don't understand why that is necessary. The import creates a new question, so we know exactly which files are in each file area - the ones we just put there. Therefore, it should be possible for the XML import code to track the list of files itself, just using an array. Unless I am missing something.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          It's quite clear I was thinking wrong ! import_files_as_draft function has all the info it needs in $xml to track the created files without calling any filestorage function. This is exactly what I have done in MDL-37934. I don't know why I took a different way here. I will try to create some code for this issue.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, It's quite clear I was thinking wrong ! import_files_as_draft function has all the info it needs in $xml to track the created files without calling any filestorage function. This is exactly what I have done in MDL-37934 . I don't know why I took a different way here. I will try to create some code for this issue.
          Hide
          Jean-Michel Vedrine added a comment -

          Branch for master created.
          I will wait your comments before doing other branchs.
          Most likely I will not be able to add sample file and testing instructions before next week-end. But I will try.

          Show
          Jean-Michel Vedrine added a comment - Branch for master created. I will wait your comments before doing other branchs. Most likely I will not be able to add sample file and testing instructions before next week-end. But I will try.
          Hide
          Jean-Michel Vedrine added a comment -

          Starting peer review.

          Show
          Jean-Michel Vedrine added a comment - Starting peer review.
          Hide
          Tim Hunt added a comment -

          That code is fine.

          I would not do it quite like that, and I feel compelled to explain how I would have written it. (Please feel free to ignore this.)

                  foreach ($xml as $file) {
                      $filename = $file['@']['name'];
                      if (in_array($filename, $filenames) {
                          debugging('Duplicate file in XML: ' . $filename, DEBUG_DEVELOPER);
                          continue;
                      }
                      $filerecord = array(
                          'contextid' => context_user::instance($USER->id)->id,
                          'component' => 'user',
                          'filearea'  => 'draft',
                          'itemid'    => $itemid,
                          'filepath'  => '/',
                          'filename'  => $filename,
                      );
                      $fs->create_file_from_string($filerecord, base64_decode($file['#']));
                      $filenames[] = $filename;
                  }
          

          Basically, it makes the if statement smaller, and I like that better. I expect other people would dislike the continue. Note, I have also added the filename to the debugging message. I think that is useful.

          In order to make a test file, would it work to export a question with one small image from Moodle, then edit the XML using copy and paste?

          Anyway, no hurry for this, but it will be nice to have it when it is done.

          Show
          Tim Hunt added a comment - That code is fine. I would not do it quite like that, and I feel compelled to explain how I would have written it. (Please feel free to ignore this.) foreach ($xml as $file) { $filename = $file['@']['name']; if (in_array($filename, $filenames) { debugging('Duplicate file in XML: ' . $filename, DEBUG_DEVELOPER); continue ; } $filerecord = array( 'contextid' => context_user::instance($USER->id)->id, 'component' => 'user', 'filearea' => 'draft', 'itemid' => $itemid, 'filepath' => '/', 'filename' => $filename, ); $fs->create_file_from_string($filerecord, base64_decode($file['#'])); $filenames[] = $filename; } Basically, it makes the if statement smaller, and I like that better. I expect other people would dislike the continue. Note, I have also added the filename to the debugging message. I think that is useful. In order to make a test file, would it work to export a question with one small image from Moodle, then edit the XML using copy and paste? Anyway, no hurry for this, but it will be nice to have it when it is done.
          Hide
          Jean-Michel Vedrine added a comment -

          Well,
          I think I will use your code. Reasons are:

          1. usually in the past following your advices has proven to be good
          2. even if I don't quite understand your motives here, having the debugging stuff encapsulated in that little if block look pretty

          Yes I think adding the $filename to the debugging message can prove very useful.
          Yes manually editing the xml file should do the trick to produce a sample file.

          Show
          Jean-Michel Vedrine added a comment - Well, I think I will use your code. Reasons are: usually in the past following your advices has proven to be good even if I don't quite understand your motives here, having the debugging stuff encapsulated in that little if block look pretty Yes I think adding the $filename to the debugging message can prove very useful. Yes manually editing the xml file should do the trick to produce a sample file.
          Hide
          Jean-Michel Vedrine added a comment -

          I forgot to add there was a missing parenthesis in my code, but fortunately running the phpunit tests catched that mistake.

          Show
          Jean-Michel Vedrine added a comment - I forgot to add there was a missing parenthesis in my code, but fortunately running the phpunit tests catched that mistake.
          Hide
          Jean-Michel Vedrine added a comment -

          Adding a sample XML file with a duplicated file tag.
          As described in issue's comments this is not a real file as I can't share the file transmitted by original Moodle user who reported the bug in the forum. This is a manually edited file to reproduce the problem.

          Show
          Jean-Michel Vedrine added a comment - Adding a sample XML file with a duplicated file tag. As described in issue's comments this is not a real file as I can't share the file transmitted by original Moodle user who reported the bug in the forum. This is a manually edited file to reproduce the problem.
          Hide
          Jean-Michel Vedrine added a comment -

          Grr ! Manually editing my first sample file I broke it and forgot to test before uploading it here. So here is (I hope) a good one.

          Show
          Jean-Michel Vedrine added a comment - Grr ! Manually editing my first sample file I broke it and forgot to test before uploading it here. So here is (I hope) a good one.
          Hide
          Tim Hunt added a comment -

          That all looks good now. I think all that remains is for you to squash it down to one commit, and cherry-pick to the stable branches.

          (P.S. Hello from Dublin.)

          Show
          Tim Hunt added a comment - That all looks good now. I think all that remains is for you to squash it down to one commit, and cherry-pick to the stable branches. (P.S. Hello from Dublin.)
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Have a nice MoodleMoot in Dublin. Here are the squashed comits and stable branches.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Have a nice MoodleMoot in Dublin. Here are the squashed comits and stable branches.
          Hide
          Tim Hunt added a comment -

          Thanks Jean-Michel. Submitting for integration.

          Show
          Tim Hunt added a comment - Thanks Jean-Michel. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24, master), thanks!

          PS: Just a random, thought... couldn't we cover this with some unittest with the expected behavior. Not critical, anyway.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, master), thanks! PS: Just a random, thought... couldn't we cover this with some unittest with the expected behavior. Not critical, anyway.
          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:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: