Database and file backup script

I created a Backup class because I couldn't find one that worked well on Windows and remotely, to do this I have combined a lot of questions on stackoverflow with a lot of trial and error to get it to work on these two platforms. I'm fairly new to OOP, and I really just want to get some tips on handling passing error messages, and I feel like I'm returning a lot (because some functions shouldn't continue if previous ones have failed), and general thoughts on the class. I have included setup stuff at the top, and some functions the class uses on the bottom (obviously I couldn't include them all). As you may notice the script backs up both user specified tables in a database (and I know I should be using PDO, but ignore that for now) as well as user specified folders and whats cool is the zippy function.

```
dir_backup = CMS_DIR . 'backup' . DIRECTORY_SEPARATOR;
$backup_core->dir_sql = $backup_core->dir_backup . 'sql' . DIRECTORY_SEPARATOR;
$backup_core->dir_files = $backup_core->dir_backup . 'files' . DIRECTORY_SEPARATOR;
$backup_core->dir_unpack = ROOT;

$backup_core->folders_to_backup = array(
FRONTEND_IMAGE_UPLOAD_PATH,
BACKEND_IMAGE_UPLOAD_PATH . 'users' . DIRECTORY_SEPARATOR
);

class Backup
{
/ BACKUP CONFIGURATION */
// folder locations
public $dir_backup;
public $dir_files;
public $dir_sql;
// unpack directory, should be at the first writable directory
public $dir_unpack;
// array of folders
public $folders_to_backup;
public $backup_amt;
// tables
public $tables_to_backup = NULL;
// don't change
private $sqlDirTemp;
private $msg;
private $totalSiteSize;
/ ZIP CONFIGURATION ****/
// settings
public $sqlDirInZip = 'sql_temp'; // sql dir inside zip
private $overwrite = true; // overwrite file if exists (just incase unlink doesnt work)
// really imp!
private $offsetLocal = -1; // local has fagard_designs afte

This is a lot of code, so this is not a complete review, just a couple of points:

Overall Program Structure

Class

Your Backup class is a bit over 700 lines long. Without even looking at the code, it's a good guess that it does too much.

What your class does:

• execute sql queries

• write to a file

• read from a file

• zip a file

• unzip a file

• create directories

• something called maintenance

• count slashes

• check size of a directory

Most of those things would deserve their own class. At least three new classes would already be helpful: for reading/writing, zipping/unzipping, and handling of directories.

Methods

Your methods do too many things as well (a method should really only do one thing). Take your backupPkg method as example: It's 80 lines long (as a rule of thumb: if it doesn't fit on your screen, it's probably too long), and it does multiple things. Even your comment states this: It creates the SQL, it writes it to a file, and it zips the file.

But even if you extract those functionalities, your method to create the SQL would still be too long. I would create a function which is responsible for creating the SQL for inserting data into one table. That way, at least your for - while - for - if - else - if code is separated from the rest of the method.

Details

Early Return

I'll take your doRestore function as example. You ask if (empty($err)) a lot here, and this leads to very deep nesting. It's very confusing to read, and it will lead to bugs quickly. Instead of using your approach, it's better to just return as soon as you get an error (if there was an error, you are not doing anything else anyways).

So your code would look something like this:

public function doRestore($id, $fromFile = false) {
    $err = '';
    $errPrepend = "Backup restore failed:{$err}";

    // remove old folders
    foreach ($this->folders_to_backup as $folder) {
        rmdirRecursive($folder);
    }

    if (!$this->zipExtract($this->dir_files . $id . '.zip', $this->dir_unpack)) {
        return $errPrepend . "An error occurred while extracting backup files from zip for backup {$id}!";
    }

    if ($fromFile && !@copy($this->sqlDirTemp . $id . '.sql', $this->dir_sql . $id . '.sql')) {
        return $errPrepend . "Failed to copy SQL file to: {$this->dir_sql}";
    }

    $open = $this->dir_sql . $id . '.sql';
    $handle = @fopen($open, "r+");
    if (!$handle) {
        return $errPrepend . "Could not open: {$open}";
    }
    $sqlFile = @fread($handle, @filesize($open));
    if (!$sqlFile) {
        return $errPrepend . "Could not read: {$open}";
    }

    $sqlArray = explode(';', $sqlFile);
    // process the sql file by statements
    foreach ($sqlArray as $stmt) {
        if (strlen($stmt) > 3) {
            $result = Nemesis::query($stmt);
        }
    }

    @fclose($handle);
    $fromFile ? rmdirRecursive($this->sqlDirTemp) : '';

    return "Backup {$id} successfully restored.";
}

This is still not perfect, but we did get rid of the deep nested ifs.

Nested if in rmdirRecursive

This pattern is not optimal:

if ($empty == false) {
            if (!rmdir($dir)) {
                return false;
            }
        }
        return true;

It takes a while to figure out that true will be returned if $empty is false and rmdir is false as well. This is clearer:

better:

if (!$empty && !rmdir($dir)) {
            return false;
        } else {
            return true;
        }

And then you can just do this:

return !(!$empty && !rmdir($dir))

Or

return !$empty || rmdir($dir)

Naming

• backupPkg: what is Pkg? The name doesn't really tell me this, and neither do the comments.

• verifySQL: this method doesn't verify SQL (which is what I would have expected), but checks if a directory exists.

Comments

/**
 *
 * Backup
 * 
 * Creates SQL, and zip backups
 *
 * @param   string  $filename
 * @param   array   $tables     tables to backup
 * @return          returns     true on success and errors on failure
 *
 */
private function backupPkg($filename) {

This comment doesn't tell me all that much.

And what it tells me is at least partially wrong (there is no $tables argument). Comments should always be up-to-date and they should tell me what the method does (which yours does partly; it doesn't tell me that it writes the SQL to a file), what side-effects it has (like your doRestore method, which deletes directories), what parameters it expects and what values they can have (does the file name has to end in .sql? can or should I prepend a path to the filename? where is the file stored?; are the table names case sensitive?; etc) and what is returned.

StackExchange Code Review Q#61675, answer score: 3