5
\$\begingroup\$

I'm still at a very beginner level and I'm constantly working on small things to try and develop my skills. I'm hoping someone could just give me a quick review if there's anything obviously horrible about the code from my main PHP class. It's not feature-complete, but I'm hoping at this point I should be able to get a good review that will help me prevent future mistakes.

My full source can be found here and a live demo is temporarily working here:

http://162.243.44.206/gif2html5/

class Gif2Html5
{
    private $uploadDir;
    private $fileSizeLimitBytes;
    private $isWindows;

    function __construct($uploadDir, $fileSizeLimitMB = 4)
    {
        $this->uploadDir = $uploadDir;
        $this->fileSizeLimitBytes = $fileSizeLimitMB * 1048576;
        $this->isWindows = strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? true : false;
    }


    public function create()
    {
        if (!isset($_FILES['uploadImage'])) {
            echo "Please select the file you want to upload.";
            return;
        }

        if (filesize($_FILES['uploadImage']['tmp_name']) > $this->fileSizeLimitBytes) {
            echo "The file you uploaded exceeds the size limit of " . $this->fileSizeLimitBytes / 1048576 . "MB";
            return;
        }

        if ($_FILES['uploadImage']['error'] == UPLOAD_ERR_OK) {
            $deleteKey = $this->generateRandomString(15);

            do {
                $randomDir = $this->generateRandomString(5);
            } while (file_exists($this->uploadDir . $randomDir));

            $newDir = $this->uploadDir . $randomDir . '/';
            mkdir($newDir, 0777, true);

            $uploadFile = $newDir . $randomDir . '.gif';

            if (!imagecreatefromgif($_FILES['uploadImage']['tmp_name'])) {
                echo "The uploaded image is not a valid .gif file.";
                return;
            }

            if (!move_uploaded_file($_FILES['uploadImage']['tmp_name'], $uploadFile)) {
                echo "There was a problem moving the file to the upload directory.";
                return;
            }

            if (!$fp = fopen($newDir . $deleteKey . '.txt', 'wb')) {
                echo "There was a problem generating the delete key. Please try again later.";
                return;
            } else {
                fclose($fp);
            }

            if ($this->isWindows) {
                $exif = 'libraries/exif/WIN/exiftool.exe -v ';
                $h264 = 'libraries/ffmpeg/WIN/bin/ffmpeg.exe -r ';
                $webm = 'libraries/ffmpeg/WIN/bin/ffmpeg.exe -r ';
            } else {
                $exif = 'libraries/exif/LINUX/exiftool -v ';
                $h264 = 'libraries/ffmpeg/LINUX/ffmpeg -r ';
                $webm = 'libraries/ffmpeg/LINUX/ffmpeg -r ';
            }

            /*
             * ------------------------------------------------------------------------
             * Use exiftool to determine gif framecount and duration and calculate FPS.
             * http://sno.phy.queensu.ca/~phil/exiftool/
             * ------------------------------------------------------------------------
             */
            $exif .= $uploadFile;
            $c1 = $this->isWindows ? str_replace("/", "\\", $exif) : $exif;
            exec($c1, $out, $return);
            try {
                $x = explode(" = ", $out[sizeof($out) - 1]);
                $y = explode(" = ", $out[sizeof($out) - 2]);

                if(strtoupper(substr($x[0], 2, 11)) === "FRAMECOUNT"){
                    $fps = 10;
                } else if(strtoupper(substr($y[0], 2, 10)) === "FRAMECOUNT") {
                    $fps = $y[1] / $x[1];
                } else {
                    throw new Exception('Potentially non-animated .gif file.');
                }
            } catch (Exception $e) {
                echo "There was a problem converting the uploaded .gif file.";
                return;
            }

            list($width, $height) = getimagesize($uploadFile);
            if ($width % 2 != 0) {
                --$width;
            }
            if ($height % 2 != 0) {
                --$height;
            }

            /*
             * -----------------------------------------------
             * Use ffmpeg to convert the gif to webm and h264.
             * http://www.ffmpeg.org/
             * http://ffmpeg.gusari.org/static/
             * http://ffmpeg.zeranoe.com/builds/
             * -----------------------------------------------
             */
            $h264 .= $fps . ' -i ' . $uploadFile . ' -y -strict experimental -acodec aac -ac 2 -ab 160k -vcodec libx264 -s ' . $width . 'x' . $height . ' -pix_fmt yuv420p -preset slow -profile:v baseline -level 30 -maxrate 10000000 -bufsize 10000000 -b 1200k -f mp4 -threads 0 ' . $newDir . $randomDir . '.mp4';
            $webm .= $fps . ' -i ' . $uploadFile . ' -c:v libvpx -b 2000k ' . $newDir . $randomDir . '.webm';
            $c2 = $this->isWindows ? str_replace("/", "\\", $h264) : $h264;
            $c3 = $this->isWindows ? str_replace("/", "\\", $webm) : $webm;
            exec($c2);
            exec($c3);

            echo "./image/" . $randomDir . '/' . $deleteKey;
            return;
        }

        switch ($_FILES['uploadImage']['error']) {
            case 2:
                echo "The file you uploaded exceeds the size limit of " . $this->fileSizeLimitBytes / 1048576 . "MB";
                break;
            case 3:
                echo "The file upload failed part way - please try again.";
                break;
            case 4:
                echo "No file was uploaded. Please select a file and try again.";
                break;
            default:
                echo "An error occurred while uploading the file.";
                break;
        }

        return;
    }

    public function get($id)
    {
        $requestedDir = $this->uploadDir . $id;
        $fileNoExt = $requestedDir . '/' . $id;
        $data['id'] = $id;
        if (@file_exists($requestedDir)) {
            $data['gif'] = @file_exists($fileNoExt . '.gif') ? true : false;
            $data['h264'] = @file_exists($fileNoExt . '.mp4') ? true : false;
            $data['webm'] = @file_exists($fileNoExt . '.webm') ? true : false;
            return $data;
        } else {
            return false;
        }
    }

    public function delete($id, $key)
    {
        $requestedDelete = $this->uploadDir . $id . '/' . $key . '.txt';
        if (@file_exists($requestedDelete)) {
            if($this->isWindows){
                $cmd = "RMDIR /s /q " . str_replace("/", "\\", $this->uploadDir . $id);
                exec ($cmd);
            } else {
                system('/bin/rm -rf ' . escapeshellarg($this->uploadDir . $id));
            }
            return true;
        } else {
            return false;
        }
    }

    function generateRandomString($stringLength)
    {
        $characters = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
        $randomString = '';

        for ($i = 0; $i < $stringLength; $i++) {
            $randomString .= $characters[rand(0, strlen($characters) - 1)];
        }

        return $randomString;
    }
}
\$\endgroup\$
4
\$\begingroup\$
  • $this->isWindows = strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? true : false; : Your check already evaluates to true. Essentially this reads as if (true) then true else false. It could be shorted to $this->isWindows = strtoupper(substr(PHP_OS, 0, 3)).
  • Your class has too many responsibilities: input validation, analyzing the gif, converting the gif, retrieving information for existing files as well as deleting files. I'd split this up into three classes: a service class for orchestrating the conversation, a file management class for retrieving information about existing files, deleting or creating them, and a converter class responsible for converting an existing and gif. This class should only check for constraints required by the conversation, not stuff like file-size and so on. (Single Responsibility Principle)
  • It's probably a good idea to have two classes for converation: one for linux and one for windows. Of course they should generalize common behavior in an abstract parent class. A nice side-effect would be to be able to add other conversion methods (or OS) easily without modifying the code (open/close principle).
  • You are depending and modifying the global application state. The reusability of this class(es) is limited to a very specific use-case as the file needs to be in $_FILES and the information is echod. I could not resuse this class somewhere else (e.g. for converting gifs in my command line tool).
  • I suppose the generateRandomString method is a helper-method. It probably should be private.
  • The gag-operator (@) is highly discouraged. Fix the problem instead of hiding it.
  • Validate your keys and ids. A possible attack vector would be directory traversal: ->delete('../../../../../etc/', ''); or a like.
\$\endgroup\$
  • \$\begingroup\$ Thank you. I'm still at the stage where I find it difficult how to break up an application. My only question from your points is how do I avoid using $_FILES? \$\endgroup\$ – SIL40 Feb 15 '14 at 13:51
  • \$\begingroup\$ @SIL40: You can't $_FILES at all costs. Yet you could the caller decide to use $_FILES instead of the callee. This way your class doesn't know how the file got at the given location (be it by $_FILES or by already existing file) nor does it know the names of the form elements. And this class really doesn't need to know. In the MVC-Pattern this would be the job of the controller (with this class being your model or service). \$\endgroup\$ – Fge Feb 17 '14 at 13:18

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.