Details
Bug
Closed
Minor
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.
Attachments
Activity
- All
- Comments
- History
- Activity
- Source
- Test Sessions
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
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
Hi Penny,
I'v been reviewing it a bit and seems to:
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:
@@ -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; }So, apart from these little things, all them related to the course_id handling, I think your changes are ok. Great!
Ciao
- 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:
- 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@@ -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; }