File-Backup should not backup itself && watchdog-calls wrong in files_backup

tirsales - March 12, 2009 - 10:14
Project:DB Maintenance
Version:6.x-2.0-beta1
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed
Description

When the backup target-location is part of the Drupal-directory (e.g. drupal/sites/default/backup), the backup-script backups all files - including the backups. This leads to fast growing backupfiles (e.g. 1GB, 2GB, 4GB, 8GB, etc).

For several reasons (e.g. Access to backups through normal Drupal-modules like filebrowser) backuping to a drupal-path seems logical to me. Nevertheless "backuping backups" is strange. The given patch excludes all files matching $dbname_files.tar.gz and $dbname_db.sql.

A warning: Setting /tmp as the default is very, very unsecure. In most cases we want a backup after e.g. a server failure occured - and in most cases, /tmp will be deleted on a regular schedule and/or with every restart.

While patching the tar-command, I found two additional errors:

1) exec($command,$output) creates $output as an array. Thus implode('
',$output) should be called to get the "line-wise" output of $command

2) Some calls to watchdog(..) are wrong.

The applied patch fixes all the above. To avoid the "exclude backup-files"-patch, simply remove the patch to $command. It would be better to add an option that allows to exclude backups.
It would be nice to integrate db_maintenance with backup-migrate (though both modules exclude each others backupfiles).

AttachmentSize
db_maintenance.patch2.79 KB

#1

tirsales - March 12, 2009 - 10:17
Priority:normal» critical

I apologize, priority should be critical as logging seems like an integral part of this module to me.
BTW: Writing $output=implode('
',$output); like I have done in the patch is somehow ugly and was only for testing purposes. Seems like I have diffed against the wrong file..

#2

tirsales - April 17, 2009 - 14:47

This is still open - e.g. db_maintenance.module, line 475 - why is there a (wrong) watchdog-statement BEHIND a return-statement? The watchdog is never reached (so probably sensible to have) - either delete it or correct it and swap statements.
For the rest of the (unfixed and uncommented) problems - see above issue.

I am more then willing to provide a patch against whatever CVS-version needed - but I want to know whether it will be used beforehand.

I am also more then willing to simply patch CVS myself if you want me too.

#3

deekayen - April 17, 2009 - 15:21
Status:needs review» fixed

committed

#4

System Message - May 1, 2009 - 15:30
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

 
 

Drupal is a registered trademark of Dries Buytaert.