patternphpMinor

GIF to HTML5 video conversion

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

videoconversiongifhtml5

Problem

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 pro

Solution

$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.

Context

StackExchange Code Review Q#41711, answer score: 4

Revisions (0)

No revisions yet.