4
\$\begingroup\$

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.

<?php
if (!defined('script_access')) {
    exit('No direct script access allowed');
}



define('APP_PATH', realpath(dirname(__FILE__)) . DIRECTORY_SEPARATOR);                                                      
define('ROOT', realpath(dirname(dirname(dirname(__FILE__)))));
define('CMS_DIR_NAME', 'neou_cms'); 
define('CMS_DIR', ROOT . DIRECTORY_SEPARATOR . CMS_DIR_NAME . DIRECTORY_SEPARATOR);                                                     
define('FRONTEND_IMAGE_UPLOAD_PATH', ROOT . DIRECTORY_SEPARATOR . 'images' . DIRECTORY_SEPARATOR . 'uploads' . DIRECTORY_SEPARATOR);        
define('BACKEND_IMAGE_UPLOAD_PATH', CMS_DIR . 'images' . DIRECTORY_SEPARATOR);              

$backup_core = new Backup();

$backup_core->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 after doc root remove one directory
    private $offsetRemote = 0; // remote files start in doc root (no offset)
    // files and folders to ignore
    public $ignored_files = array('.DS_STORE');
    public $ignored_folders = array('_notes');
    public $ignored_ext = array('');
    // don't change
    private $zip;
    private $SQLinit = false;
    private $offset;

    public function __construct()
    {
        ini_set('upload_max_filesize', '200M');

        $this->msg = Messages::getInstance();

        $this->totalSiteSize = !Localization::isLocal() ? folderSize(ROOT) : '';

        // Note: zip will extract in ROOT and put sql there as well
        $this->sqlDirTemp = ROOT . DIRECTORY_SEPARATOR . $this->sqlDirInZip . DIRECTORY_SEPARATOR;

        extension_loaded('zip') ? $this->zip = new ZipArchive() : '';

        $this->offset = Localization::isLocal() ? $this->offsetLocal : $this->offsetRemote;
    }
    /*------------------------------------------- BACKUP FN'S ------------------------------------------- */
    /**
     *
     * 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)
    {

        $err = '';
        $return = '';

        // get all of the tables
        if (!isset($this->tables_to_backup)) {
            $tables = array();
            $result = Nemesis::query('SHOW TABLES');
            if (!$result) {
                return 'Could not gather table names';
            }
            while ($row = $result->fetch_row()) {
                $tables[] = $row[0];
            }
            // or if the user defines the tables
        } elseif (is_array($this->tables_to_backup)) {
            $tables = explode(',', $this->tables_to_backup);
        } else {
            return 'If you are specifying tables to be backed up, you need to provide function ' . __FUNCTION__ . ' with an array.';
        }

        // cycle through each provided table
        foreach ($tables as $table) {
            $result = Nemesis::select("*", $table);
            $result_q = Nemesis::query("SHOW CREATE TABLE {$table}");
            if (!$result && !$result_q) {
                return 'SQL dump was unsuccessful. Could not run queries.';
            }
            $num_fields = $result->field_count;
            // first part of the output - remove the table
            $return .= 'DROP TABLE ' . $table . ';<|||||||>';
            // second part of the output - create table
            $result_q_row = $result_q->fetch_row();
            $return .= "\n\n" . $result_q_row[1] . ";<|||||||>\n\n";
            // third part of the output - insert values into new table
            for ($i = 0; $i < $num_fields; $i++) {
                while ($row = $result->fetch_row()) {
                    $return .= 'INSERT INTO ' . $table . ' VALUES(';
                    for ($j = 0; $j < $num_fields; $j++) {
                        $row[$j] = addslashes($row[$j]);
                        $row[$j] = preg_replace("#\n#", "\\n", $row[$j]);
                        if (isset($row[$j])) {
                            $return .= '"' . $row[$j] . '"';
                        } else {
                            $return .= '""';
                        }
                        if ($j < ($num_fields - 1)) {
                            $return .= ',';
                        }
                    }
                    // add an identifier for explode later
                    $return .= ");<|||||||>\n";
                }
            }
            $return .= "\n\n\n";
        }

        if (!is_bool($mkdir = $this->createSaveDir())) {
            return $mkdir;
        }
        $sqlFile = $this->dir_sql . $filename . '.sql';
        $handle = @fopen($sqlFile, 'w+');
        if (!$handle) {
            $err = "Could not open: {$sqlFile}";
        }
        if (empty($err) && @fwrite($handle, $return) === FALSE) {
            $err = "Could write: {$sqlFile}";
        }
        if (empty($err) && $this->zippyConstructor($this->dir_files . $filename . '.zip', $this->folders_to_backup, $sqlFile) == FALSE) {
            $err = "Zip could not complete successfully.";
        }
        @fclose($handle); // we have to close the sql file so we have permission to delete (if required)
        if (!empty($err)) {
            @unlink($this->dir_sql . $filename . '.sql');
            @unlink($this->dir_files . $filename . '.zip');
            return $err;
        } else {
            return true;
        }
    }
    /**
     *
     * Creates necessary dirs for save
     *
     * @returns true on success and errors on failure
     *
     */
    private function createSaveDir()
    {

        $dirs = array(
            $this->dir_backup,
            $this->dir_files,
            $this->dir_sql
        );

        if (!is_bool($mkdir = mkdirIterator($dirs))) {
            return $mkdir; // errors
        } else {
            return true;
        }
    }
    /**
     *
     * Do Backup
     * 
     * Calls backup
     *
     * @return adds messages
     *
     */
    public function doBackup()
    {
        $filenameFormat = 'dbbackup_' . date(str_replace('-', '.', DATE_FORMAT) . '_H_i_s');

        if (!is_bool($output = $this->backupPkg($filenameFormat))) {
            $this->msg->add('e', "Backup failed:<br>{$output}");
        } else {
            $this->msg->add('s', "Backup {$filenameFormat} successfully created.");
        }
    }
    /**
     *
     * Do Restore
     *
     * @param   string  $id         restore id
     * @param   boolean $fromFile   if we are restoring from file this should be true
     *  
     */
    public function doRestore($id, $fromFile = false)
    {
        $err = '';

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

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

        if (empty($err)) {
            if ($fromFile) {
                if (!@copy($this->sqlDirTemp . $id . '.sql', $this->dir_sql . $id . '.sql')) {
                    $err = "Failed to copy SQL file to: {$this->dir_sql}";
                }
            }
            if (empty($err)) {
                $open = $this->dir_sql . $id . '.sql';
                $handle = @fopen($open, "r+");
                if (!$handle) {
                    $err = "Could not open: {$open}";
                }
                $sqlFile = @fread($handle, @filesize($open));
                if (empty($err) && !$sqlFile) {
                    $err = "Could not read: {$open}";
                }
                if (empty($err)) {
                    $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) : '';
            }
        }

        if (empty($err)) {
            return "Backup {$id} successfully restored.";
        } else {
            return "Backup restore failed:<br>{$err}";
        }
    }
    /**
     *
     * Restores from file upload
     *  Checks file is zip
     *  Moves file to $dir_files directory
     *  Checks zip by extracting to tmp directory
     *  On success, deletes tmp dir, extracts to root
     *
     */
    public function doRestoreFromFile()
    {

        $isCorrect = false;

        $filename = $_FILES["zip_file"]["name"];
        $source = $_FILES["zip_file"]["tmp_name"];
        $type = $_FILES["zip_file"]["type"];
        $accepted_types = array(
            'application/zip',
            'application/x-zip-compressed',
            'multipart/x-zip',
            'application/x-compressed'
        );

        foreach ($accepted_types as $mime_type) {
            if ($mime_type == $type) {
                $isCorrect = true;
                break;
            }
        }

        $name = explode(".", $filename);
        $continue = strtolower($name[1]) == 'zip' ? true : false;
        if (!$isCorrect && !$continue && $_FILES['zip_file']['error'] != 1) {
            return 'The file you are trying to upload is not a .zip file. Please try again.';
        }

        if (!is_bool($mkdir = $this->createSaveDir())) {
            return $mkdir;
        }

        $files = $this->dir_files . $filename;
        // move and begin validating contents for sql file
        if (@move_uploaded_file($source, $files)) {

            $filenameNoExt = self::removeExt($filename);

            // make a temp dir to extract and test for sql file
            $tmpDir = $this->dir_files . 'temp_' . md5(time()) . DIRECTORY_SEPARATOR;
            @mkdir($tmpDir);

            // could not extract
            if (!$this->zipExtract($files, $tmpDir)) {
                return 'An error occurred while extracting files.';
            }

            // check for sql file
            $search_sql = $tmpDir . $this->sqlDirInZip . DIRECTORY_SEPARATOR . $filenameNoExt . '.sql';
            if (!@is_file($search_sql) && !@is_readable($search_sql)) {
                // remove bad upload
                @unlink($files);
                // remove temp dir
                rmdirRecursive($tmpDir);
                rmdirRecursive($files);
                return 'There is an error in your file.';
            }

            // now the zip file must be valid
            rmdirRecursive($tmpDir);
            if (strpos($output = $this->doRestore($filenameNoExt, true), 'successfully') !== FALSE) {
                return rtrim($output, '.') . ' and uploaded!';
            } else {
                return $output;
            }

        } else {
            return 'There was a problem with the upload. Please try again.';
        }
    }
    /**
     *
     * Removes extension from file
     *
     * @param   string  $filename
     * @return  string  $filename without extension
     *
     */
    public static function removeExt($filename)
    {
        return preg_replace('/\\.[^.\\s]{3,4}$/', '', $filename);
    }
    /**
     *
     * Gets extension from file
     *
     * @param   string  $filename
     * @return  string  $filename extension
     *
     */
    public static function getExt($filename)
    {
        return substr(strrchr($filename, '.'), 1);
    }
    /**
     *
     * Maintenance
     *
     * Handles creation (if frontpage = true) in the event that
     * the last backup was over a day old since login and deletion
     * of old backups, as defined by $backup_amt
     *
     * @param   bool    $frontpage  if true then this script can be run on the homepage
     *  
     */
    public function maintenance($frontpage = false)
    {
        if ($frontpage && isAdmin()) {
            // make sure the dirs exists
            if (!is_bool($mkdir = $this->createSaveDir())) {
                return $mkdir;
            }
            // if the dir is empty create a backup
            if (isDirEmpty($this->dir_sql)) {
                $this->doBackup();
                // otherwise, do the normal routine
            } else {
                $path = $this->dir_sql;
                $latest_ctime = 0;
                $latest_filename = '';
                $entry = '';
                $d = dir($path);
                while (false !== ($entry = $d->read())) {
                    $filepath = $path . DIRECTORY_SEPARATOR . $entry;
                    // could do also other checks than just checking whether the entry is a file
                    if (is_file($filepath) && filectime($filepath) > $latest_ctime) {
                        $latest_ctime = filectime($filepath);
                        $latest_filename = $path . $entry;
                    }
                }
                /* check if latest backup is over a day old
                if it is create a new backup and run the regular
                maintenance scripts below */
                if (is_file($latest_filename) && date('d', filectime($latest_filename)) < date('d')) {
                    $this->doBackup();
                }
            }
        }
        $list = listDirDate($this->dir_sql);
        $handle = opendir($this->dir_sql);
        $file = readdir($handle);
        $ct = count($list);
        if (is_numeric($this->backup_amt) && $ct > $this->backup_amt) {
            /* we delete the user specified backup by the backup amt.
            say we have 5, user specified only 2 backups to be kept
            we delete the last 3 backups */
            $list = array_slice($list, 0, $ct - $this->backup_amt);
        foreach ($list as $file => $timestamp) {
            $filenameNoExt = self::removeExt($file);
            @unlink($this->dir_sql . $filenameNoExt . '.sql');
            @unlink($this->dir_files . $filenameNoExt . '.zip');
            $cleanup[] = $filenameNoExt;
        }
        }
        closedir($handle);
        if (!$frontpage && !empty($cleanup)) {
            $passed = implode(', ', $cleanup);
            $message = "Deleted: {$passed}";
            $this->msg->add('w', $message);
        }
    }
    /*------------------------------------------- ZIP ------------------------------------------- */
    /**
     *
     * zippy constructor
     *
     * Called to create zip files
     *
     * @param   string  $destination    where the file goes
     * @param   array   $sources        array
     * @param   string  $SQLfile        if set will include an sql file in the default zip location inside zip
     * @return  boolean                 true or false on success or failure, respectively
     *
     * Note: it is possible to provide sources as $sources = array('ROOT') where ROOT is the root
     * directory of the site
     *
     */
    public function zippyConstructor($destination, $sources = array(), $SQLfile = NULL)
    {
        if (!extension_loaded('zip')) {
            return false;
        }
        if (file_exists($destination)) {
            @unlink($destination);
        }
        if ($this->zip->open($destination, $this->overwrite == true ? ZIPARCHIVE::OVERWRITE : ZIPARCHIVE::CREATE) !== TRUE) {
            return false;
        }
        if (isset($SQLfile) && is_file($SQLfile)) {
            // zip sql file in sql directory
            $this->SQLinit = true;
            $SQLfile = str_replace('\\', DIRECTORY_SEPARATOR, realpath($SQLfile));
            $this->zip->addEmptyDir($this->sqlDirInZip);
            $this->zip->addFromString($this->sqlDirInZip . '/' . basename($SQLfile), file_get_contents($SQLfile));
        }
        foreach ($sources as $source) {
            if (!$this->zippy($source)) {
                $this->zip->close();
                @unlink($destination);
                return false;
            }
        }
        if ($this->SQLinit) {
            if (!$this->verifySQL($destination)) {
                // couldn't verify that sql made it we close dir and remove it, return a false
                $this->zip->close();
                @unlink($destination);
                return false;
            }
        }
        unset($this->SQLinit);
        $this->zip->close();
        return true;
    }
    /**
     *
     * zippy
     *
     * Creates a zip file from a $source directory
     * $include_dir = true means we get the parent directories up until the root directory
     *  otherwise, we just put in the $source dir and its children.
     *
     * @param   string      $source
     * @param   boolean     $include_subs   includes sub directories, limited by offset to root
     * @param   int         $cOffset        custom offset, $include_subs must be false to work
     *                                          default value of -2
     *
     */
    public function zippy($dir, $include_subs = true, $c0ffset = -2)
    {
        if (!extension_loaded('zip')) {
            return false;
        }
        $docRootArr = explode('/', rtrim($_SERVER['DOCUMENT_ROOT'], '/') . '/');
        $docRootCount = count($docRootArr);
        $sOffset = $docRootCount - (1 + $this->offset);
        if ($sOffset < 0) {
            return false;
        }
        $dir = realpath($dir);
        $files = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir), RecursiveIteratorIterator::SELF_FIRST);
        foreach ($files as $file) {
            if (is_dir($file)) {
                continue;
            } elseif (is_file($file)) {
                // ignore is in the order of most taxing on the system for efficency
                $filename = basename($file);
                // ignored files
                if (in_array($filename, $this->ignored_files)) {
                    continue;
                    // ignored extensions
                } elseif (in_array(self::getExt($filename), $this->ignored_ext)) {
                    continue;
                    // ignored folders (str_replace removes filename which we filtered first)
                } elseif (self::strposArray(str_replace($filename, '', $file), $this->ignored_folders)) {
                    continue;
                }
                $s1 = str_replace('\\', '/', $file); // normalize slashes
                $s2 = explode('/', $s1); // explode the dir by slashes
                if ($include_subs) {
                    // include directories until root (modified by offset)
                    $s3 = array_slice($s2, $sOffset);
                } elseif (isset($c0ffset)) {
                    // @ -2 just include the file and the directory (go from back of array 2 in (file + dir))
                    $s3 = array_slice($s2, $c0ffset);
                } else {
                    return false;
                }
                $s4 = str_replace('//', '/', implode('/', $s3)); // implode and remove double slashes
                $this->zip->addFromString($s4, file_get_contents($file)); // add file
            }
        }
        return true;
    }
    /**
     *
     * Count Slashes
     *
     * Given a $str will return the amount of slashes in the string
     * We can use this to find the diff (new func needed) between 
     *  /HTML/ and where we are now, and then send that number to minus
     *
     * @param   string  $str
     * @return  int     number of slashes in $str
     *
     */
    public static function countSlashes($str)
    {
        return substr_count($str, '/');
    }
    /**
     *
     * strposArray
     *
     * Array usage of strpos
     * Returns false on first true matching result from the given array
     *
     * @param   string              $haystack
     * @param   string || array     $needle
     * @param   int                 $offset
     * @return  boolean
     *
     */
    public static function strposArray($haystack, $needle, $offset = 0)
    {
        if (!is_array($needle)) {
            $needle = array(
                $needle
            );
        }
        foreach ($needle as $query) {
            if (strpos($haystack, $query, $offset) !== false) {
                return true; // stop on first true result
            }
        }
        return false;
    }
    /**
     *
     * verifySQL
     *
     * Called to check that the $sqlDirInZip exists
     * Returns false on failure to affirm the above
     *
     * @return  boolean
     *
     */
    private function verifySQL()
    {
        // the '/sql' dir exists
        // we want to use '/' instead of directory sep bc windows looks up in zips / instead of \
        if ($this->zip->statName($this->sqlDirInZip . '/') !== false) {
            return true;
        } else {
            return false;
        }
    }
    /**
     *
     * Zip Extract
     *
     * Extracts a ZIP archive to the specified extract path
     *
     * @param   string  $file           The ZIP archive to extract (including the path)    
     * @param   string  $extractPath    The path to extract the ZIP archive to
     * @return  boolean                 True if the ZIP archive is successfully extracted
     *  
     */
    public function zipExtract($file, $extractPath)
    {
        if (!extension_loaded('zip')) {
            return false;
        }
        if ($this->zip->open($file) === TRUE) {
            $this->zip->extractTo($extractPath);
            $this->zip->close();
            return true;
        } else {
            return false;
        }
    }
    /*------------------------------------------- STATS ------------------------------------------- */
    /**
     *
     * Percentage Overflow
     *
     * Child function of findBackupPercentage
     * Determines if the backup dir is too large
     *
     * @return bool true if the backup dir is greater than 25% of the entire site size
     *
     */
    public function percentageOverflow()
    {
        if (!Localization::isLocal()) {
            $stmt = str_replace('%', '', $this->findBackupPercentage());
            // return true if backup folder percentage is higher than 25% (1/4) of the total site
            if ($stmt > 25) {
                return true;
            } else {
                return false;
            }
        } else {
            return 'NaN';
        }
    }
    /**
     *
     * Find Backup Percentage
     *
     * Determines the percentage of the backup dir
     * relative to the entire site size
     *
     * @param   int     $precision (default 2 decimal points)
     * @return          percentage or NaN
     *
     */
    public function findBackupPercentage($precision = 2)
    {
        if (!Localization::isLocal()) {
            $total_backup_folder = folderSize($this->dir_backup);
            $percentage = ($total_backup_folder / $this->total_site_size) * 100;
            return round($percentage, $precision) . '%';
        } else {
            return 'NaN';
        }
    }
    /**
     * Find Backup File Percentage
     *
     * Determines the percentage of a backup file
     * relative to the entire site size
     *
     * @param   int     $precision (default 2 decimal points)
     * @return          percentage or NaN
     *
     */
    public function findBackupFilePercentage($filename, $precision = 2)
    {
        if (!Localization::isLocal()) {
            $size_files = filesize($this->dir_files . $filename . '.zip');
            $size_sql = filesize($this->dir_sql . $filename . '.sql');
            $total_file = $size_files + $size_sql;
            $percentage = ($total_file / $this->total_site_size) * 100;
            return round($percentage, $precision) . '%';
        } else {
            return 'NaN';
        }
    }
}
?>

functions from other classes / functions:

    public static function isLocal()
    {
        $whitelist = array(
            'localhost',
            '127.0.0.1'
        );
        if (in_array($_SERVER['HTTP_HOST'], $whitelist)) {
            return TRUE;
        } else {
            return FALSE;
        }
    }

    function rmdirRecursive($dir, $empty = false)
    {
        if (substr($dir, -1) == "/") {
            $dir = substr($dir, 0, -1);
        }
        if (!file_exists($dir) || !is_dir($dir)) {
            return false;
        } elseif (!is_readable($dir)) {
            return false;
        } else {
            $handle = opendir($dir);
            while ($contents = readdir($handle)) {
                if ($contents != '.' && $contents != '..') {
                    $path = $dir . "/" . $contents;
                    if (is_dir($path)) {
                        rmdirRecursive($path);
                    } else {
                        unlink($path);
                    }
                }
            }
            closedir($handle);
            if ($empty == false) {
                if (!rmdir($dir)) {
                    return false;
                }
            }
            return true;
        }
    }

    function folderSize($dir)
    {
        if (is_dir($dir)) {
            $total_size = 0;
            $files = scandir($dir);
            $cleanDir = rtrim($dir, '/') . '/';
            foreach ($files as $t) {
                if ($t <> "." && $t <> "..") {
                    $currentFile = $cleanDir . $t;
                    if (is_dir($currentFile)) {
                        $size = foldersize($currentFile);
                        $total_size += $size;
                    } else {
                        $size = filesize($currentFile);
                        $total_size += $size;
                    }
                }
            }
            return $total_size;
        } else {
            return false;
        }
    }

    function mkdirIterator($dirs)
    {
        $err = array();
        if (is_array($dirs)) {
            foreach ($dirs as $dir) {
                if (!is_dir($dir)) {
                    if (!mkdir($dir)) {
                        $err[] .= "Directory '{$dir}' could not be created.";
                    }
                }
            }
            // check to see if errors occured in the making of more than one directory
            if (count($err) > 1) {
                $err[] .= "More than one directory could not be made. Please check if you have permission to make folders on the server";
                return implode('<br>', $err);
            // we must have only one error then...
            } elseif (!empty($err)) {
                return $err[0];
            } else {
                return true;
            }
        } elseif (isset($dirs)) {
            if (!is_dir($dirs)) {
                if (!mkdir($dirs)) {
                    return "Directory {$dir} could not be created.";
                }
            }
        }
    }
\$\endgroup\$
3
\$\begingroup\$

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:<br>{$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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your input, it is much appreciated! For some reason I felt sort of dirty returning so much, and I do agree my functions do a lot of different things. I think by splitting them up I would remove alot of the nested ifs. \$\endgroup\$ – Alex Sep 2 '14 at 1:25
  • \$\begingroup\$ Do you feel like I should be handling errors diffrently? Like via try/catch? \$\endgroup\$ – Alex Sep 2 '14 at 1:30
  • \$\begingroup\$ @Alex yes, I think it would be an improvement. For example, the doRestore method returns string right now, and then you check if the string contains success or not. This is not good. If instead you throw exceptions in doRestore, then you don't need to do that. Also, you could try to handle the error by type (couldn't open file, couldn't write to file, etc). \$\endgroup\$ – tim Sep 2 '14 at 9:03
  • \$\begingroup\$ This app has certainty changed since '14 :) \$\endgroup\$ – Alex Mar 5 '18 at 6:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.