Moodle
  1. Moodle
  2. MDL-20596

import backup file silently / backup course silently bugfixes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.7, 2.0
    • Fix Version/s: 1.9.7, 2.0
    • Component/s: Backup
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      31907

      Description

      Patch for review.

      Tested with the two example scripts to be placed in moodle's root directory. Obviously the course to be backed up and the file to restore will need to be changed.

      1. 0001-Reworked-handling-of-new-courses-slightly-based-on-E.patch
        3 kB
        Penny Leach
      2. backuptest.php
        0.4 kB
        Penny Leach
      3. restoretest.php
        0.6 kB
        Penny Leach
      4. silentbackuprestore.patch
        13 kB
        Penny Leach
      5. silentbackuprestore2.patch
        13 kB
        Penny Leach

        Activity

        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Penny,

        I'v been reviewing it a bit and seems to:

        • Perform roles mapping on restore. I guess that means that new roles were being created on each silent restore? Wow. +1
        • Clean partially some $SESSION/$restore mess along different functions. +1
        • Double check restore of logs only happens if enabled and backup contained them. +1

        So, that looks ok, all good improvements. Anyway, I've some comments about some tiny changes that 1) I don't understand or 2) could affect normal (manual) process. Here they are:

        • Code below seems to relax the destination course detection, so no check is performed if $destinationcourse is false/zero... What happens in that case? Is the destination calculated later of is a bug? Previous check looked better for me:
          @@ -665,7 +664,8 @@
                   $cleanupafter = false;
                   $errorstr = ''; // passed by reference to restore_precheck to get errors from.
           
          -        if (!$course = get_record('course','id',$destinationcourse)) {
          +        $course = null;
          +        if ($destinationcourse && !$course = get_record('course','id',$destinationcourse)) {
                       mtrace($debuginfo.'Course with id $destinationcourse was not a valid course!');
                       return false;
                   }
          
        • In restore_create_new_course() you have change $restore to be passed by reference, so it gets changed by the function. Do we really need that and, if so, for what? It used to work ok. Not critical if you need it for silent operations, just hoping it won't affect manual ones.
        • In the same function, restore_create_new_course() you've changed the behaviour so now it returns the created course id. In theory that isn't necessary as far as $course_header->course_id already has it and, some lines below in restore_execute() $restore->course_id is set based in than value. Not critical, has sense to return the created course id.
        • But the point I don't like (related with the previous 2) is that you set a new variable $newcourseid with the value of the created course (when it's already both in $course_header->course_id and $restore->course_id, and worse, you alter the value returned by the restore_execute() function by returning always that $newcourseid value. IMO you only can return that value if the $status has ended ok, else we can end with courses that have failed to restore for some reason ($status = false), but with the restore_execute() funciton returning true (one valid courseid). And I think that's nono (in a logical way - note that I haven't checked the whole execution to see if that can happen in practice). So I think it's better to return the courseid (once more, only if you need it - like in previous points) only if the $status is true.

        So, apart from these little things, all them related to the course_id handling, I think your changes are ok. Great!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Penny, I'v been reviewing it a bit and seems to: Perform roles mapping on restore. I guess that means that new roles were being created on each silent restore? Wow. +1 Clean partially some $SESSION/$restore mess along different functions. +1 Double check restore of logs only happens if enabled and backup contained them. +1 So, that looks ok, all good improvements. Anyway, I've some comments about some tiny changes that 1) I don't understand or 2) could affect normal (manual) process. Here they are: Code below seems to relax the destination course detection, so no check is performed if $destinationcourse is false/zero... What happens in that case? Is the destination calculated later of is a bug? Previous check looked better for me: @@ -665,7 +664,8 @@ $cleanupafter = false ; $errorstr = ''; // passed by reference to restore_precheck to get errors from. - if (!$course = get_record('course','id',$destinationcourse)) { + $course = null ; + if ($destinationcourse && !$course = get_record('course','id',$destinationcourse)) { mtrace($debuginfo.'Course with id $destinationcourse was not a valid course!'); return false ; } In restore_create_new_course() you have change $restore to be passed by reference, so it gets changed by the function. Do we really need that and, if so, for what? It used to work ok. Not critical if you need it for silent operations, just hoping it won't affect manual ones. In the same function, restore_create_new_course() you've changed the behaviour so now it returns the created course id. In theory that isn't necessary as far as $course_header->course_id already has it and, some lines below in restore_execute() $restore->course_id is set based in than value. Not critical, has sense to return the created course id. But the point I don't like (related with the previous 2) is that you set a new variable $newcourseid with the value of the created course (when it's already both in $course_header->course_id and $restore->course_id, and worse , you alter the value returned by the restore_execute() function by returning always that $newcourseid value. IMO you only can return that value if the $status has ended ok, else we can end with courses that have failed to restore for some reason ($status = false), but with the restore_execute() funciton returning true (one valid courseid). And I think that's nono (in a logical way - note that I haven't checked the whole execution to see if that can happen in practice). So I think it's better to return the courseid (once more, only if you need it - like in previous points) only if the $status is true. So, apart from these little things, all them related to the course_id handling, I think your changes are ok. Great! Ciao
        Hide
        Penny Leach added a comment -

        Hi Eloy!

        1. Yep
        2. Yep
        3. Yep
        4. Previously there was no way to tell import_backup_file_silently to create a new course - it required an existing course to restore into. So yup, it is relaxed, and if it's not there, a new course is created.
        5. Hmm. I wonder about this actually, surely in php5 objects are always passed by reference anyway, although I guess in Moodle 1.9 we're not requiring it yet. I guess the reason is that I'm writing to $restore, but as you say, I could rework that to use SESSION instead.
        6. I think it's logical to return the new courseid, but maybe that's too much for 1.9. As you say, it's in SESSION, so I'll use that instead.
        7. Yep, Good catch, definitely. I think for 1.9 we just keep returning $status, and maybe for 2.0 should return the new courseid (if $status is true, or no exceptions are thrown, or however you're handling errors in 2.0)

        If you're happy with 4, I'll rework the patch to change 5, 6 and 7.

        Show
        Penny Leach added a comment - Hi Eloy! 1. Yep 2. Yep 3. Yep 4. Previously there was no way to tell import_backup_file_silently to create a new course - it required an existing course to restore into. So yup, it is relaxed, and if it's not there, a new course is created. 5. Hmm. I wonder about this actually, surely in php5 objects are always passed by reference anyway, although I guess in Moodle 1.9 we're not requiring it yet. I guess the reason is that I'm writing to $restore, but as you say, I could rework that to use SESSION instead. 6. I think it's logical to return the new courseid, but maybe that's too much for 1.9. As you say, it's in SESSION, so I'll use that instead. 7. Yep, Good catch, definitely. I think for 1.9 we just keep returning $status, and maybe for 2.0 should return the new courseid (if $status is true, or no exceptions are thrown, or however you're handling errors in 2.0) If you're happy with 4, I'll rework the patch to change 5, 6 and 7.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yup, np with 4 as far as it only affects import_silenty behaviour (i.e. doesn't introduce any change in "restore core"). Go, go, go!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yup, np with 4 as far as it only affects import_silenty behaviour (i.e. doesn't introduce any change in "restore core"). Go, go, go! Ciao
        Hide
        Penny Leach added a comment -

        Attached are two new patches, one containing the rework and one containing the full new patch.

        It occurs to me that Piers would be a good reviewer for this because of his moodlectl work. I'll see if we can get him to test it too, before we commit.

        Show
        Penny Leach added a comment - Attached are two new patches, one containing the rework and one containing the full new patch. It occurs to me that Piers would be a good reviewer for this because of his moodlectl work. I'll see if we can get him to test it too, before we commit.
        Hide
        Penny Leach added a comment -

        watcher-ping

        Piers, could you possibly have a play with this with your moodlectl stuff ?

        Show
        Penny Leach added a comment - watcher-ping Piers, could you possibly have a play with this with your moodlectl stuff ?
        Hide
        Piers Harding added a comment -

        Hi Penny and Eloy - I've run my unit test suite (for moodlectl) against this patch and course backup/restore works perfectly.
        Cheers,
        Piers Harding.

        Show
        Piers Harding added a comment - Hi Penny and Eloy - I've run my unit test suite (for moodlectl) against this patch and course backup/restore works perfectly. Cheers, Piers Harding.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        +1 I think now everything is ok. Please send it both to 19_STABLE and HEAD (just to have it as a reference there, although, for sure, that's is going to be 100% refactored to only one backup()/restore() functions.

        (great the "incremental" patch. It really helps to read the interesting bits)

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - +1 I think now everything is ok. Please send it both to 19_STABLE and HEAD (just to have it as a reference there, although, for sure, that's is going to be 100% refactored to only one backup()/restore() functions. (great the "incremental" patch. It really helps to read the interesting bits) Ciao
        Hide
        Penny Leach added a comment -

        committed to head & stable. thanks eloy & piers

        Show
        Penny Leach added a comment - committed to head & stable. thanks eloy & piers

          People

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

            Dates

            • Created:
              Updated:
              Resolved: