Moodle
  1. Moodle
  2. MDL-27174

Automated Backups Fail When Assignment Course Module Has Instance of Zero (0)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.10
    • Fix Version/s: 1.9.14
    • Component/s: Backup
    • Labels:
    • Rank:
      16833

      Description

      We have multiple client sites with automated backups that do not complete. Error messages point to the assignment module trying to require a file that doesn't exist.

      PHP Warning: require_once(<moodle_root>/mod/assignment/type//assignment.class.php): failed to open stream: No such file or directory in <moodle_root>/mod/assignment/backuplib.php on line 79
      PHP Fatal error: require_once(): Failed opening required '<moodle_root>/mod/assignment/type//assignment.class.php' (include_path='<moodle_root>/lib/pear:.:/usr/share/php:/usr/share/pear') in <moodle_root>/mod/assignment/backuplib.php on line 79

      The middle of the require_once where it has a double slash should have been populated with the assignment type. It's not populated because the course_module record for the assignment has an instance of 0 so Moodle doesn't know what kind of assignment it is. The instance=0 records show up after a course is restored from backup, although not every restore has this issue and it is not limited to backups from different servers.

      We created a patch that allows the assignment backup to skip assignments that have an instance of 0 and removes such records when restoring.

      1. assignmentbackup_part2.patch
        0.5 kB
        Chris Follin
      2. assignmentbackup.patch
        2 kB
        Chris Follin

        Issue Links

          Activity

          Hide
          Chris Follin added a comment -

          We still had some issues with this after the original patch, although the original did help. This second patch is in addition to, not in place of, the first patch.

          Show
          Chris Follin added a comment - We still had some issues with this after the original patch, although the original did help. This second patch is in addition to, not in place of, the first patch.
          Hide
          Michael Blake added a comment -

          This issue has been reported as causing an issue for a MP. Please give it priority.

          Show
          Michael Blake added a comment - This issue has been reported as causing an issue for a MP. Please give it priority.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Chris,
          is it possible to provide a sample backup that is causing this?

          i've been wondering if the assignment sub-type was installed prior to restoring the course. Could that have caused the instance=0 record?

          if so, we need to improve the zero instances cleanup (ah your patch does do that) or imo preferably not create these zero instance records.

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - - edited Hi Chris, is it possible to provide a sample backup that is causing this? i've been wondering if the assignment sub-type was installed prior to restoring the course. Could that have caused the instance=0 record? if so, we need to improve the zero instances cleanup (ah your patch does do that) or imo preferably not create these zero instance records. cheers, Aparup
          Hide
          Aparup Banerjee added a comment -

          linking bug about cleaning up zero instances after restore.

          Show
          Aparup Banerjee added a comment - linking bug about cleaning up zero instances after restore.
          Hide
          Aparup Banerjee added a comment -

          This will need proper review and testing. The suggested patch seems to make sense , just not sure about the improved zero instance cleaning up.

          Surely there will be some regressions or else why was the original solution to zero instances cleanup more strict? but i'm not sure - hopefully peer reviewing this will shed some light.

          Show
          Aparup Banerjee added a comment - This will need proper review and testing. The suggested patch seems to make sense , just not sure about the improved zero instance cleaning up. Surely there will be some regressions or else why was the original solution to zero instances cleanup more strict? but i'm not sure - hopefully peer reviewing this will shed some light.
          Hide
          David Mudrak added a comment -

          Well I believe there is a good reason why the original cleaner selected only the course modules it was interested in: we can't delete all course modules with instance=0 in the given course as there can be some script executed in parallel that just created them and is going to update the field soon...

          Show
          David Mudrak added a comment - Well I believe there is a good reason why the original cleaner selected only the course modules it was interested in: we can't delete all course modules with instance=0 in the given course as there can be some script executed in parallel that just created them and is going to update the field soon...
          Hide
          Aparup Banerjee added a comment -

          Thanks David. I believe you're right. also just realised that field is zero by default.

          Perhaps we should not have to restore such modules in the first place but 1.9 backup/restore is so fantastic

          in any case, that first patch does make sense - i've updated my repo. David, if you can have a look to peer review it again that would be great.

          perhaps cleanup can be improved as an improvement issue?

          Show
          Aparup Banerjee added a comment - Thanks David. I believe you're right. also just realised that field is zero by default. Perhaps we should not have to restore such modules in the first place but 1.9 backup/restore is so fantastic in any case, that first patch does make sense - i've updated my repo. David, if you can have a look to peer review it again that would be great. perhaps cleanup can be improved as an improvement issue?
          Hide
          Chris Follin added a comment -

          Hi Aparup,

          Sorry it has taken me a few days to get back to you. I don't have a sample backup handy at the moment but I might be able to find one if you still need it. It would not seem to be an issue of the assignment type not existing because we had this problem when restoring a course on the exact same site on which the backup was made. The assignment type existed when the backup was made and still existed when the backup was attempted to be restored. That said, I unfortunately still do not have a better idea, or any idea for that matter, of how these 0 instances came to be.

          Show
          Chris Follin added a comment - Hi Aparup, Sorry it has taken me a few days to get back to you. I don't have a sample backup handy at the moment but I might be able to find one if you still need it. It would not seem to be an issue of the assignment type not existing because we had this problem when restoring a course on the exact same site on which the backup was made. The assignment type existed when the backup was made and still existed when the backup was attempted to be restored. That said, I unfortunately still do not have a better idea, or any idea for that matter, of how these 0 instances came to be.
          Hide
          David Mudrak added a comment -

          Looking good and safe for me now.

          Show
          David Mudrak added a comment - Looking good and safe for me now.
          Hide
          Aparup Banerjee added a comment -

          Thanks David,
          Sending this up for integration as it fixes the fatal error.

          Chris,
          Thanks for your fix!

          If the clean up improvements do still need to be done, please open up a separate issue linked to this

          Show
          Aparup Banerjee added a comment - Thanks David, Sending this up for integration as it fixes the fatal error. Chris, Thanks for your fix! If the clean up improvements do still need to be done, please open up a separate issue linked to this
          Hide
          Sam Hemelryk added a comment -

          Thanks Apu, this has been integrated now.
          Please add some testing instructions.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Apu, this has been integrated now. Please add some testing instructions. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Hi Aparup. Could you please expand those testing instructions a bit. Do you mean an assignment that has several subtypes? Can you provide a link to such a thing? And just generally clean up the English

          Show
          Andrew Davis added a comment - Hi Aparup. Could you please expand those testing instructions a bit. Do you mean an assignment that has several subtypes? Can you provide a link to such a thing? And just generally clean up the English
          Hide
          Aparup Banerjee added a comment -

          Hi Andrew,
          I've clarified it a bit more. Links added too

          Show
          Aparup Banerjee added a comment - Hi Andrew, I've clarified it a bit more. Links added too
          Hide
          Rajesh Taneja added a comment -

          Works as expected...
          Thanks Chris and Aparup for fixing this issue

          Show
          Rajesh Taneja added a comment - Works as expected... Thanks Chris and Aparup for fixing this issue
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream and closing, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream and closing, many thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: