an example is function backup_module (line 2278 in backuplib.php in current MOODLE_19).

foreach ($preferences->mods[$module]->instances as $instance => $object) {
if (!empty($object->backup)) { $status = $onemodbackup($bf,$preferences,$instance); }
}

As you can see, this will set the final 'status' value to whatever the last module returned. So if modules 1, 2, 3, and 4 fail, but module 5 passes, this function returns success. This is clearly wrong. In addition, this function also does not output any status information - which should be visible always-displayed status information, not just
debugging() - indicating which module instance has actually failed.

The way this was written (status value getting overwritten, not displaying information about which module caused a failure) appeared to me to be endemic across backup/restore code - that's what I wanted sorting out.

• should change to something like this (untested!)
  foreach ($preferences->mods[$module]->instances as $instance => $object) {
  if (!empty($object->backup))
  Unknown macro: { $statusm = $onemodbackup($bf,$preferences,$instance); if (!$statusm) { notify('backup of:'.$module.'-'.$instance.' failed.'); $status = false; } }
  
  }

--should also check to see if backup is silent and only notify when not silent, and should log an entry to backup_log

if (!empty($object->backup)) {
$statusm = $onemodbackup($bf,$preferences,$instance);
if (!$statusm) {
if (!defined('BACKUP_SILENTLY')) { notify('backup of '.$module.'-'.$object->name.' failed.'); }
$status = false;
}
}

the $status=false for silent backup imo isn't enough either... - as part of MDL-14736 I plan to add a new field to the backup_log table in HEAD which allows us to push errors like this into it.

Dan

Dan

Dan

I think backup_log_table.patch (7 kB) looks ok. The older backup_log_patch.txt (23 kB) won't work anymore (backup tables were transferred to core lib/db some weeks ago).

As far as current scheduled logs are written with 'scheduledbackup' and retrieved in that way, it seems ok... although what happens with "old" records? It seems that the new field isn't filled ever for those old records...

1. - new field in backup_log table to allow manual backups to report errors there
2. - improved backuplib.php backup_execute() to write errors to the backup_log table. (especially useful for silent backups)

There are probably further areas for improvement, but closing this issue as fixed for now - We can use other Tracker issues for further improvements! - if anyone disagrees, feel free to re-open!

Dan

My only comment is if, instead of executing that:

$DB->execute("UPDATE {backup_log} SET backuptype='scheduledbackup'");

we should use one simpler $DB->set_field() or, perhaps, if using 'scheduledbackup' is better for some databases (fill on creation instead of update).

But I guess it isn't really important.

Good work. Closing... ciao