Details
-
Type:
Bug
-
Status:
Open
-
Priority:
Minor
-
Resolution: Unresolved
-
Affects Version/s: 1.9
-
Fix Version/s: STABLE backlog
-
Component/s: Backup
-
Labels:None
-
Affected Branches:MOODLE_19_STABLE
Description
list of issues in backup/restore, both long standing and gradebook related
Attachments
Issue Links
Activity
- All
- Comments
- History
- Activity
- Source
- Test Sessions
Hi Petr,
just some quick comments (it has been impossible for me to examine the patch deeply).
1) Grades should be restored a bit below, as talked, after all the modules and instances have been properly set, i.e. after the restore_check_instances() block of code.
2) There are some "grrr..." debugs here and there. 
3) The double loops, get_records_select() + foreach should be easily transformed into get_recordset_XXX() loops, saving a bunch of memory, specially for user information records.
4) It seems you are using the gradebook API to build grades and so on (instead or raw DB access). Perfect!
5) Also you've differentiated old gradebooks and new gradebooks from the parser. Good idea!
6) I've seen you've hacked the backup_getid() function to avoid current notices about the "needed" thing. Hehe, I've it in my TODO list. And really think that hack shouldn't be there. It's relate with how that info is added to the backup_ids table (unserialised) and I wan't to fix it on insertion. I think it has one TODO defined somewhere in the file and I'll try to insert it properly serialised.
7) Good idea the conditional addslashes, in backup_todb()
8) Finally, one comment about big parts in the backup and restore. Both must include some output to the browser from time to time (to avoid browser timeouts). I guess all the critical parts of the gradebook will have such pieces of code added (haven't really checked that). For now I'd use the current strategy consisting in something like:
$counter++;
//Do some output
if ($counter % XXXXXX == 0) {
if (!defined('RESTORE_SILENTLY')) {
echo ".";
if ($counter % 20*XXXXX == 0) {
echo "<br />";
}
}
backup_flush(300);
}
it's only a matter of adjust XXXXX above to a number granting one dot to be sent to output each, say, 1-2 secs (aprox).
Edited: I missed. Of course those "output blocks" should be out for 2.0 with a central function replacing them, but I'd keep current behaviour for stable.
And that's all I've been able to review, hope it helps. Ciao 
3) The double loops, get_records_select() + foreach should be easily transformed into get_recordset_XXX() loops, saving a bunch of memory, specially for user information records.
4) It seems you are using the gradebook API to build grades and so on (instead or raw DB access). Perfect!
5) Also you've differentiated old gradebooks and new gradebooks from the parser. Good idea!
6) I've seen you've hacked the backup_getid() function to avoid current notices about the "needed" thing. Hehe, I've it in my TODO list. And really think that hack shouldn't be there. It's relate with how that info is added to the backup_ids table (unserialised) and I wan't to fix it on insertion. I think it has one TODO defined somewhere in the file and I'll try to insert it properly serialised.
7) Good idea the conditional addslashes, in backup_todb()
8) Finally, one comment about big parts in the backup and restore. Both must include some output to the browser from time to time (to avoid browser timeouts). I guess all the critical parts of the gradebook will have such pieces of code added (haven't really checked that). For now I'd use the current strategy consisting in something like:
$counter++;
//Do some output
if ($counter % XXXXXX == 0) {
if (!defined('RESTORE_SILENTLY')) {
echo ".";
if ($counter % 20*XXXXX == 0) {
echo "<br />";
}
}
backup_flush(300);
}
it's only a matter of adjust XXXXX above to a number granting one dot to be sent to output each, say, 1-2 secs (aprox).
Edited: I missed. Of course those "output blocks" should be out for 2.0 with a central function replacing them, but I'd keep current behaviour for stable.
And that's all I've been able to review, hope it helps. Ciao
1/ grades were already restored after restore_check_instances() in that patch
2/ grrr is gone 
3/ in next patch
6/ the "needed" flag is bloody hack - seems Yu invented most of these in role and user restore; goingto look at this together with roles restore later
8/ only individual grades restore does this - there is always limited number of grade_items and categories ( < 100) - it executes fast enough; anyway I think there should be a function for this in backup/lib.php next patch
going to test this a bit more today and commit, then continue with the rest of backup/restore issues
thanks!
3/ in next patch
6/ the "needed" flag is bloody hack - seems Yu invented most of these in role and user restore; goingto look at this together with roles restore later
8/ only individual grades restore does this - there is always limited number of grade_items and categories ( < 100) - it executes fast enough; anyway I think there should be a function for this in backup/lib.php next patch
going to test this a bit more today and commit, then continue with the rest of backup/restore issues
thanks! Hi Petr, about 6) yup, it wasn't invented for roles but it's a dependency from old (before roles) versions. In fact, somewhere in my TODO list are:
- TODO: Change this call inserting to a standard backup_putid() call
- TODO: Also analyse it the "needed" info is really needed for anything. Drop if not.
That I detected when cleaning up a bit users backup http://tracker.moodle.org/browse/MDL-10721
AFAIK, roles assignments and overrides are properly working, I remember I did some tests when fixing that bug and everything worked (with the TODO notes above).
Just FYI, ciao
- TODO: Change this call inserting to a standard backup_putid() call
- TODO: Also analyse it the "needed" info is really needed for anything. Drop if not.
sending preview of patch for gradebook restore//backup issues,
other stuff will be in second wave
please test/review