Issue Details (XML | Word | Printable)

Key: MDL-14736
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Dan Marsden
Reporter: Dan Marsden
Votes: 0
Watchers: 2
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

issue deleting temp dirs in backup

Created: 08/May/08 06:34 AM   Updated: 04/Mar/09 08:35 AM
Return to search
Component/s: Backup
Affects Version/s: 1.8.3, 1.9
Fix Version/s: 1.9.5, 2.0

File Attachments: 1. Text File backup_scheduled.txt (1 kB)
2. Text File restorelib.txt (2 kB)


Participants: Dan Marsden and Eloy Lafuente (stronk7)
Security Level: None
QA Assignee: Eloy Lafuente (stronk7)
Resolved date: 03/Mar/09
Affected Branches: MOODLE_18_STABLE, MOODLE_19_STABLE
Fixed Branches: MOODLE_19_STABLE, MOODLE_20_STABLE


 Description  « Hide
Hi Eloy,

this is related to the user post here:
http://moodle.org/mod/forum/discuss.php?d=95768#p426822

it seems to happen in backup_execute() in backuplib.php

obviously for some reason, backup_delete_old_data() is failing and not deleting the directories inside directories completely - I haven't looked closely - but is this a bug in backup_delete_old_dirs() ? - the user is using an older version of moodle so it's potentially fixed.....

I suggest we change the call in backup_execute:
            if (!defined('BACKUP_SILENTLY')) {
                error ("An error occurred deleting old backup data");
            }

... to use a debugging call instead of the error() function.. - the fact that it hasn't deleted all the old dirs in temp doesn't actually affect the backup itself, which should succeed fine, and simply displaying a warning should suffice IMO

thanks,

:-)


Dan

 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Eloy Lafuente (stronk7) added a comment - 08/May/08 07:30 AM
Well spotted, 100% agree, Dan...

that shouldn't be an stop-error. debugging sounds ok here... although perhaps I also will notify() the user about that. Note that the BACKUP_SILENTLY part (some lines below...) is also returning false and perhaps should allow to continue. And same could be happening in scheduled_backups and restore (both use that function).

Can you handle this? TIA!


Dan Marsden added a comment - 08/May/08 07:34 AM
will do

thanks,

Dan


Dan Marsden added a comment - 09/May/08 07:29 AM
now in 1.9stable and HEAD - not sure about 1.8stable though.... that function is new in 1.9 and I can't seem to find the string mentioned in the forum posting anywhere in 1.8 .... is this an issue in 1.8stable?

Dan


Eloy Lafuente (stronk7) added a comment - 09/May/08 07:57 AM
Hi Dan,

if I'm not wrong... under 1.8 that is executed by backup_execute.html (in 1.9 it was repackaged to the backup_execute() function you've changed).

Also... backup_scheduled.php and restorelib.php seem to be using that function with similar behaviour: stop the process.

Ciao


Dan Marsden added a comment - 09/May/08 11:00 AM
thanks for the details about 1.8 - will look at patching that too sometime soon (I thought those other files did something similar too, but was playing with cvs a bit and wanted to test some small commits!)

...looking at the way those errors are presented - the silent backup errors don't go anywhere! - I'm keen to insert them into the backup_logs table, and can't see much in scheduled backups that relys on the data in backup_logs except for this call to see if it hasn't completed - which we might be able to alter slightly....

$numofrec = count_records_select ("backup_log","time > $timeafter")

In fact I'm thinking that we could add a new field to backup_log that defines what process/function wrote to the log eg (manual backup, Scheduled backup, Incremental backup, Silient backup - or individual module backup scripts?)

does that sound ok?

Dan


Eloy Lafuente (stronk7) added a comment - 13/May/08 06:07 AM - edited
Ah, yes! I read this yesterday and forgot to comment.

Sounds ok for me (HEAD only) + potential mini add_to_backup_log() function to control all them together.

Ciao


Dan Marsden added a comment - 13/May/08 06:09 AM
yes - I was thinking Head only - (can't make db changes to Stable branches)

Dan


Dan Marsden added a comment - 14/May/08 08:24 AM
attaching patches for restorelib and backup_scheduled.php - we really need somewhere to store all those $errorstr when calling restore_silently, backup_silently!

Dan


Dan Marsden added a comment - 03/Mar/09 10:19 AM
fix now in HEAD

Eloy Lafuente (stronk7) added a comment - 04/Mar/09 08:35 AM
Adding 2.0 as fixfor version (partially) and closing... thanks Dan!