Code review (directory handling) working, but not pretty

[anderson_catchme]

$username = $loggedInUser->username; // This is the logged in username
$time = time();
$makedir = $username.'_'.$time;

$var = getcwd();
$var = str_replace('\users', '\imageuploads', $var);
$dirlocation = $var."\\".test_directory($username, $mysqli);

 

function test_directory ($username, $mysqli) {
     $stmt = $mysqli->prepare("SELECT Temp_Directory FROM uc_users WHERE user_name LIKE ?");
    $stmt->bind_param("s", $username);
    $stmt->execute();
    $stmt->bind_result($Tempdir);
    while ($stmt->fetch()){
    return $Tempdir;
        }
    }

if((!empty(test_directory($username, $mysqli))) && is_dir($dirlocation)){
//echo "this is it";
$thedirectory = $dirlocation;
}

if(empty(test_directory($username, $mysqli))){
//echo "it's not a directory";
$newdir = $var."\\".$makedir;
$query = mysqli_query($mysqli, "UPDATE uc_users SET Temp_Directory='$makedir' WHERE user_name='$username'");
if(!$query){
//echo mysqli_error($mysqli);
}
mkdir($newdir);
//security
chmod($newdir, 0644);
$thedirectory = $newdir;
}

if(!is_dir($dirlocation) && (!empty(test_directory($username, $mysqli)))){
//echo "third one";
mkdir($dirlocation);
chmod($dirlocation, 0644);
$thedirectory = $dirlocation;
}

 

 

 

 

Ok, so what I'm doing here is testing to see whether a a record exists of the user having a folder in the MySQL database. Then, if it does, make sure that a folder exists at that location. If there is no folder, we create one for the user. If there is already a folder, we leave it alone.

 

This is for image uploads, and $thedirectory, is where we upload images later on in the script.

 

Hope that makes sense.

 

The code seems to work. But how can I improve it and make it more robust? Or should I just leave it alone? Should I return FALSE from the function for better reliability over empty()?

 

[Jacques1]

Please use code tags so that we can actually read the code.

 

Sorry, but I don't think the script is good. First of all, what's the whole point of those user folders? Are you trying to prevent filename collisions when your users upload images? That's not how an upload works. Letting people choose their own filenames is a major security risk and will lead to collisions in any way. Instead, you have to choose the name using a random number generator or an AUTO_INCREMENT column from the database (random numbers are preferable). The user-provided filename is only informational and may only be stored as metadata. And when you choose proper filenames, you don't need all those user folders.

 

I also don't understand the purpose of appending the time. Shouldn't the usernames be unique? If they're not unique, then a simple timestamp doesn't help you, because two people may very well upload their files within the same second.

 

Besides the conceptual issues, there are several technical problems:

• Using the working directory is a bad idea, because it depends on the server architecture. You could end up pretty much anywhere. So always specify the concrete path you want, don't rely on your server to figure it out for you.
• While you do use a prepared statement on top of the script, for some reason you've decided to go back to unescaped queries later in the script. That's obviously a security vulnerability or at least a bug.
• The control flow is hard to follow, because there's no proper code structure. For example, the three if statements actually belong together, but you just write them down one after another.
• Why do you use a LIKE query to fetch the user data?
• Why do you fetch the result with a loop when you only expect one row? The usernames are unique, right?
• ...

Personally, I'd start from scratch. Before you write down any code, think about the filename problem I mentioned above.

[anderson_catchme]

Thanks for this response. Would it be a good idea to just store all the images in one folder? I'm actually renaming the images with UNIX_TIMESTAP, which will be unique.

 

The problem I see with storing everything in one folder it that I will have to store the names of the images in the MySQL Db corresponding to each user's post.

 

Agreed though, all those folders are NOT good.

 

Also, I don't know a better way than a LIKE query. And wasn't aware that a loop is a bad idea for only one row.

Edited September 4, 2014 by anderson_catchme

[Jacques1]

Would it be a good idea to just store all the images in one folder?

 

Yes.

 

 

 

I'm actually renaming the images with UNIX_TIMESTAP, which will be unique.

 

Who says the timestamp is unique? Nothing prevents two users from uploading an image within the same second.

 

 

 

The problem I see with storing everything in one folder it that I will have to store the names of the images in the MySQL Db corresponding to each user's post.

 

That's a good thing. This way the user doesn't have to worry about unique filenames, and you don't have to go through complicated uniqueness checks.

 

 

 

Also, I don't know a better way than a LIKE query.

 

What about a simple equality check like you do in your second query?

 

 

 

And wasn't aware that a loop is a bad idea for only one row.

 

It's just not necessary. When you only have one row, you only need one fetch() call. 

[anderson_catchme]

function create_image($ext, $filepath, $newfilepath, $jpegquality){
$ext = strtolower($ext);
switch ($ext){
case "jpg":
    $im = @imagecreatefromjpeg($filepath);
    if($im){
    imagejpeg($im, $newfilepath, $jpegquality);
}
break;
case "jpeg":
    $im = @imagecreatefromjpeg($filepath);
    if($im){
    imagejpeg($im, $newfilepath, $jpegquality);
break;
}
case "gif":
    $im = @imagecreatefromgif($filepath);
    if($im){
    imagegif($im, $newfilepath);
}
break;
case "png":
    $im = @imagecreatefrompng($filepath);
    if($im){
    imagepng($im, $newfilepath, 5);
}
break;
    }
if(!$im){
return FALSE;
    }
}

$var = getcwd();
$var = str_replace('\users', '\tempimages\\', $var);
$output_dir = $var;

if(!is_array($_FILES["myfile"]["name"])) //single file
    {
        $fileName = $_FILES["myfile"]["name"];
        //security
        // Rename files
        $splitName = explode(".", $fileName); //split the file name by the dot
        $fileExt = end($splitName); //get the file extension
        $newFileName = strtolower(time().'_'.uniqid().'.'.$fileExt);
        if(create_image($fileExt, $_FILES["myfile"]["tmp_name"], $output_dir.$newFileName, 80) !== FALSE){
        $query = mysqli_query($mysqli, "SELECT temp_json FROM uc_users WHERE user_name ='$username'");
        while ($row = mysqli_fetch_array($query)){
        $json = $row['temp_json'];
        }
        if((json_decode($json) == FALSE || NULL)){
        // No JSON objects in db
        $json = $newFileName;
        $json = trim($json);
        $json = json_encode($json, JSON_FORCE_OBJECT);
        $query = mysqli_query($mysqli, "UPDATE uc_users SET temp_json='$json' WHERE user_name='$username'");
            }
        else {
        // Already have JSON objects in db
        $json = json_decode($json,true);
        $json = array_values($json);
        array_push($json, $newFileName);
        $json = json_encode($json, JSON_FORCE_OBJECT);
        $query = mysqli_query($mysqli, "UPDATE uc_users SET temp_json='$json' WHERE user_name='$username'");
                }
        }
</code>

Thanks for your wisdom Jacques1.

 

Anyway, I rewrote it entirely. I'm now saving JSON filenames to database. Took some time & effort. Hopefully this (working?!) code looks better?

[anderson_catchme]

Still not working 100%. So far:

function create_image($ext, $filepath, $newfilepath, $jpegquality){
$ext = strtolower($ext);
switch ($ext){
case "jpg":
	$im = @imagecreatefromjpeg($filepath);
	if($im){
	imagejpeg($im, $newfilepath, $jpegquality);
}
break;
case "jpeg":
	$im = @imagecreatefromjpeg($filepath);
	if($im){
	imagejpeg($im, $newfilepath, $jpegquality);
break;
}
case "gif":
	$im = @imagecreatefromgif($filepath);
	if($im){
	imagegif($im, $newfilepath);
}
break;
case "png":
	$im = @imagecreatefrompng($filepath);
	if($im){
	imagepng($im, $newfilepath, 5);
}
break;
	}
if(!$im){
return FALSE;
	}
}



//By this point in the script, a directory should exist
//$output_dir = "uploads/";
//$output_dir = getcwd()."/ImageUploads/imguploads/".test_directory($username, $mysqli)."/";
/*
$output_dir = $thedirectory."/";
*/

$var = getcwd();
$var = str_replace('\users', '\tempimages\\', $var);
$output_dir = $var;
//Scan directory to count uploaded images
/*
$count_files = array_diff(scandir($output_dir), array('..', '.'));
if(count($count_files) >= 10){
die();
};
*/


//echo $output_dir;
if(isset($_FILES["myfile"]))
{
	$ret = array();
//	This is for custom errors;	
/*	$custom_error= array();
	$custom_error['jquery-upload-file-error']="File already exists";
	echo json_encode($custom_error);
	die();
*/
	$error =$_FILES["myfile"]["error"];
	//You need to handle  both cases
	//If Any browser does not support serializing of multiple files using FormData() 
	if(!is_array($_FILES["myfile"]["name"])) //single file
	{
		$fileName = $_FILES["myfile"]["name"];
		//security
		// Rename files
		$splitName = explode(".", $fileName); //split the file name by the dot
		$fileExt = end($splitName); //get the file extension
	//	if(!check_ext($fileExt)){ die();}
	//	$newFileName  = strtolower($time.'.'.$fileExt);
		$newFileName = strtolower(time().'_'.uniqid().'.'.$fileExt);
        if(create_image($fileExt, $_FILES["myfile"]["tmp_name"], $output_dir.$newFileName, 80) !== FALSE){
		//	mysqli_query('SET CHARACTER SET utf8');
	   $query = mysqli_query($mysqli, "SELECT temp_json FROM uc_users WHERE user_name ='$username'");
        while ($row = mysqli_fetch_array($query)){
        $json = $row['temp_json'];
        }
		if(!empty($json)){
        // Already have JSON objects in db
        $json = json_decode($json);
        $json = array_values($json);
        array_push($json, $newFileName);
        $json = json_encode($json);
        $query = mysqli_query($mysqli, "UPDATE uc_users SET temp_json='$json' WHERE user_name='$username'");
                }
        else {
        // No JSON objects in db
        $json = $newFileName;
        $json = json_encode($json);
	//	$json = $mysqli->real_escape_string;
		//$json = 'first';
        $query = mysqli_query($mysqli, "UPDATE uc_users SET temp_json='$json' WHERE user_name='$username'");
            }
        }		
	//  UPDATE `csv_db`.`uc_users` SET `Temp_Directory` = '' WHERE `uc_users`.`id` = 1; 
		
	//	move_uploaded_file($_FILES["myfile"]["tmp_name"],$output_dir.$fileName);
	//	rename($output_dir.$fileName, $output_dir.$newFileName);
		// Done Renaming files
    	$ret[]= $fileName;
	}

[Ch0cu3r]

 

 

 I'm now saving JSON filenames to database.

What? Why? That is unnecessary

 

The details of the images a user uploads should not need be inserted into the users table. 

 

When a user uploads an image you insert a new row into a table called images (or whatvever you want to name it as) to store the image details (such as filename, filesize etc) along with the users id/username.

 

When you want to show the images a user has uploaded you query the images table filtering the results by the users id/username. 

[Jacques1]

Indeed. Storing complex data like lists, JSON objects or whatever in an SQL database is generally wrong, because it violates the First normal form (values must be atomic). I mean, just look at how much code you need for a trivial task like adding a new upload. That should be a single INSERT query, not 20 lines of PHP! And now try to actually do something with the data in MySQL: It's almost impossible. You cannot use a normal query, because all information is mixed into this one string.

 

On top of that, you'll run into big problems when people try to upload multiple files in quick succession. Imagine a situation like this:

Request 1                       Request 2
-----------------------------------------------------

user uploads file A

                                user uploads file B

you read the JSON object
and append file A

                                you read the JSON object
                                and append file B

you save the new JSON
object with file A

                                you save the new JSON
                                object with file B,
                                overwriting the previously
                                updated object

-> The new JSON object only contains file B

So, no, this is not a good idea.

 

A couple of other things:

• Why do you re-create all images?
• You're still relying on the working directory.
• To get the extension of a file, just use pathinfo($file_path, PATHINFO_EXTENSION). No need for this home-made explode()/end() stuff.
• You really need to start using prepared statements. Manual escaping is fragile and obsolete.
• Do not use SET CHARACTER SET or SET NAMES to change the character encoding of the database connection! This doesn't inform the MySQL client about the new encoding, so it thinks you're still using the old one. As a result, critical functions like mysqli_real_escape_string() may break entirely.

 

[anderson_catchme]

You both make good points, which I will inplement. But one last question.

The code is for posting on a site. Sometimes, a user will upload images but not complete the post.

 

My thoughts are is to have all images uploaded to a temporary folder named /tempimages. Insert names of images and usernames in a table called 'temp_images' like suggested. When a user loads the posting page, delete all old rows from previous posts in 'temp_images' associated with that username. Then, once the user hits the submit button, move all rows in 'temp_images' to 'images' table, and move all files from /tempimages folder to /images. Table 'temp_images' and folder /tempimages would be deleted from manually for maintaince every now and then.

 

Is above a good solution?

[Jacques1]

So there are two stages? One image upload and then a separate page for other data?

 

Can't you make that a single page? Then you don't have to mess with temporary data.

[anderson_catchme]

Yes, well the posting stage is actually a 3 step process, the last step being hit the submit. 

 

I'm using an ajax uploader script I found on github, and the php script is getting called every time a file gets uploaded. It is:

 * jQuery Upload File Plugin * version: 3.1.10 * @requires jQuery v1.5 or later & form plugin * Copyright (c) 2013 Ravishanker Kusuma * http://hayageek.com/

That's a good point though, I'll have to check to see whether I can configure the upload functionality upon submit rather than just whenever a file is selected.

[anderson_catchme]

Turns out, it can be. And that's a much better design. Thanks for everyone's excellent ideas.