Moodle
  1. Moodle
  2. MDL-34872

Backup fails with deleted users in course

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • A student enrolled in a course
      • A forum activity where the student can post

      Test steps

      1. As the student, post to the forum
      2. As the admin, delete the student from the website
      3. Backup the course
      4. Make sure the backup is correct and no exceptions are raised during the process
      Show
      Test pre-requisites A student enrolled in a course A forum activity where the student can post Test steps As the student, post to the forum As the admin, delete the student from the website Backup the course Make sure the backup is correct and no exceptions are raised during the process
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34872-master
    • Rank:
      43395

      Description

      When running the CLI to create backups, an exception was raised 'invaliduser'.

      After digging a bit, I think this is a regression caused by MDL-33061.

      This seem to fix the issue:

      diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php
      index 24d7da9..78c0a45 100644
      --- a/backup/moodle2/backup_stepslib.php
      +++ b/backup/moodle2/backup_stepslib.php
      @@ -1835,7 +1835,7 @@ class backup_annotate_all_user_files extends backup_execution_step {
                   'backupid' => $this->get_backupid(), 'itemname' => 'userfinal'));
               foreach ($rs as $record) {
                   $userid = $record->itemid;
      -            $userctx = context_user::instance($userid);
      +            $userctx = context_user::instance($userid, IGNORE_MISSING);
                   if (!$userctx) {
                       continue; // User has not context, sure it's a deleted user, so cannot have files
                   }
      

      I did not test those replication steps, but they should work:

      1. Create a course and enrol users in them
      2. Delete one of those users
      3. Run `php admin/cli/automated_backups.php`

      Expected:

      • The course is backed up

      Actual:

      • An exception is raised, the course and the following are not backed up

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          (Master only integration)

          Show
          Frédéric Massart added a comment - (Master only integration)
          Hide
          Aparup Banerjee added a comment -

          Hi Fred,
          this looks ok to me .

          I also looked up other areas in backup using contest_user::instance().
          something seems amiss in backup/restorefile.php liens 119 and 160 as the $user_context vars aren't used there for some reason. (no todo either)
          my wild untested guess is : https://github.com/nebgor/moodle/compare/int_master...int_MDL-34872

          Show
          Aparup Banerjee added a comment - Hi Fred, this looks ok to me . I also looked up other areas in backup using contest_user::instance(). something seems amiss in backup/restorefile.php liens 119 and 160 as the $user_context vars aren't used there for some reason. (no todo either) my wild untested guess is : https://github.com/nebgor/moodle/compare/int_master...int_MDL-34872
          Hide
          Frédéric Massart added a comment -

          Hi Apu, thanks for reviewing this. About the restorefile.php, that is not related to this issue, but I wonder if it is a duplicated variable, or if it was meant to ne used in 'filecontext' but wasn't...

          Pushing for integration.

          Show
          Frédéric Massart added a comment - Hi Apu, thanks for reviewing this. About the restorefile.php , that is not related to this issue, but I wonder if it is a duplicated variable, or if it was meant to ne used in 'filecontext' but wasn't... Pushing for integration.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks.

          Show
          Dan Poltawski added a comment - Integrated, thanks.
          Hide
          Jason Fowler added a comment -

          Works fine

          Show
          Jason Fowler added a comment - Works fine
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: