Uploading multiple files using a class

Question

Below I have implemented a class that will validate and upload files based on the specified rules.

class Uploader
{
 protected $finfo;
 // extensions corresponding mime types
 protected $mimes = [
 'jpg' => 'image/jpeg', 
 'pdf' => 'application/pdf',
 'png' => 'image/png', 
 'gif' => 'image/gif'
 ];
 // default settings
 protected $defaults = [
 'auto_rename' => true, // rename the uploaded file to a random ID
 'new_name' => null, // rename the uploaded file to this name
 'ext_whitelist' => null, // mime types corresponding to the types of files allowed to be uploaded
 //'max_height' => null, // maximum height (in pixels) that the file can be; null for no limit
 'max_size' => null, // maximum size (in megabytes) that the file can be; null for no limit
 //'max_width' => null, // maximum width (in pixels) that the file can be; null for no limit 
 'path' => null, // path to the folder where the upload should be placed
 'required' => false // require that file needs to be uploaded
 ];
 // settings for fields to be uploaded
 protected $rules = array();
 // files that were processed properly
 protected $processed = array();
 // errors found during processing or uploading
 protected $errors = array();
 /**
 * Initialize uploader.
 */
 public function __construct()
 {
 $this->finfo = new finfo();
 }
 /**
 * Set default settings.
 * @param array $params
 */
 public function setConfig(array $params)
 {
 foreach ($this->defaults as $name => $value) {
 isset($params[$name]) && $this->defaults[$name] = $params[$name];
 }
 }
 /**
 * Set the settings for the fields to be uploaded.
 * @param array $params
 */
 public function setRules(array $params)
 {
 foreach ($params as $name => $value) {
 // merge the default settings with field's settings
 if (is_array($value)) {
 $this->rules[$name] = $value + $this->defaults;
 } else {
 $this->rules[$value] = $this->defaults;
 }
 }
 }
 /**
 * Process files uploaded based on the settings given.
 * @param array $params
 * @return true on success or false on failure
 */
 public function process()
 {
 foreach ($this->rules as $name => $rules) {
 // check file was given
 if (empty($_FILES[$name]) || !$this->isUploadedFile($_FILES[$name])) {
 if ($rules['required']) {
 $this->errors[$name] = 'No file was provided.';
 }
 continue;
 }
 $file = $_FILES[$name];
 // check file error
 if ($file['error'] !== UPLOAD_ERR_OK) {
 $this->errors[$name] = 'An error occurred while trying to upload file.';
 continue;
 }
 $file['ext'] = $this->getExtension($file);
 $file['size'] = $this->getSize($file);
 // create directory if it does not exist
 if (!$rules['path'] || !$this->mkdir($rules['path'])) {
 $this->errors[$name] = 'An error occurred while trying to upload file.';
 error_log('Unable to write to directory, ' . $rules['path'], 3, 'log.txt');
 continue;
 }
 // check file extension
 if ($rules['ext_whitelist'] && strpos($rules['ext_whitelist'], $file['ext']) === false) {
 $this->errors[$name] = 'This file is not a valid format.' ;
 continue;
 }
 // check file size
 if ($rules['max_size'] && $file['size'] > $rules['max_size']) {
 $this->errors[$name] = 'This file is too large to upload.';
 continue;
 }
 // rename filename using given new name
 if ($rules['new_name']) {
 $file['name'] = $rules['new_name'];
 // rename filename using random number
 } elseif ($rules['auto_rename']) {
 $file['name'] = uniqid() . '.' . $file['ext'];
 }
 // mark as processed
 $this->processed[$name] = $file;
 }
 return !$this->errors;
 }
 /**
 * Upload processed files.
 * @return true on success or false on failure
 */
 public function run()
 {
 foreach ($this->processed as $name => $file) {
 $rules = $this->rules[$name];
 $destination = $rules['path'] . '/' . $file['name'];
 // save file to destination
 if (!$this->move($file, $destination)) {
 $this->errors[$name] = 'An unexpected error has occurred m.';
 error_log('Unable to upload file, ' . $destination, 3, 'log.txt');
 }
 }
 return !$this->errors;
 }
 /**
 * Get processed file.
 * @return array
 */
 public function getFile($name)
 {
 return isset($this->processed[$name]) && !isset($this->errors[$name]) ? $this->processed[$name] : false;
 }
 /**
 * Get errors found during processing or uploading.
 * @return array
 */
 public function getErrors()
 {
 return $this->errors;
 }
 /**
 * Gets file extension.
 * @param array $file
 * @return extension of the file if it is found
 */
 protected function getExtension(array $file)
 {
 return array_search($this->finfo->file($file['tmp_name'], FILEINFO_MIME_TYPE), $this->mimes, true);
 }
 /**
 * Gets file size in MB.
 * @param array $file
 * @return size of the file 
 */
 protected function getSize(array $file)
 {
 return round((filesize($file['tmp_name']) / 1048576), 2);
 }
 /**
 * Tells whether the file was uploaded via HTTP POST.
 * @param array $file
 * @return true on success, false on failure
 */
 protected function isUploadedFile(array $file)
 {
 return file_exists($file['tmp_name']) && is_uploaded_file($file['tmp_name']);
 }
 /**
 * Make directory if it does not already exist.
 * @param array $file
 * @return true on success, false on failure 
 */
 protected function mkdir($path) 
 {
 return is_dir($path) && is_writable($path) || mkdir($path, 0755, true);
 }
 /**
 * Delete file if it does exist.
 * @param array $file
 * @return true on success, false on failure 
 */
 protected function unlink($path) 
 {
 return !is_file($path) || unlink($path);
 }
 /**
 * Moves an uploaded file to a new location.
 * @param array $file
 * @return true on success, false on failure 
 */
 protected function move(array $file, $path)
 {
 // delete old file if one already exist and upload new file
 return !$this->unlink($path) ?: move_uploaded_file($file['tmp_name'], $path);
 }
}

This is how it would be used when a form is submitted:

require 'Uploader.php';
if (isset($_POST['submit'])) {
 $uploader = new Uploader();
 // set global settings
 $uploader->setConfig(array(
 'path' => dirname(__FILE__) . '/uploads' // where to upload files
 ));
 // set settings for individual fields
 $uploader->setRules(array(
 'file1' => array(
 'auto_rename' => false, // do not rename file
 'ext_whitelist' => 'gif|jpg|png', // accept only GIF, JPEG, and PNG
 'max_size' => 1 // accept only sizes up to 1 MB
 ),
 'file2' => array(
 'required' => true, // require file
 'ext_whitelist' => 'pdf', // accept only PDF
 'max_size' => 20 // accept only sizes up to 20 MB
 )
 ));
 if ($uploader->process() && $uploader->run()) {
 if ($file1 = $uploader->getFile('file1')) {
 echo $file1['name'] . ' was uploaded<br>';
 }
 if ($file2 = $uploader->getFile('file2')) {
 echo $file2['name'] . ' was uploaded';
 }
 } else {
 var_dump($uploader->getErrors());
 }
}

I know there are many existing uploading classes, but I have yet to find one that is exactly to my liking. So I wanted to try and write my own. My goal with this class is to be able to validate files (for now, images and PDFs) and upload files. Though this class is probably not as strong as existing ones, it works as intended.

So, any suggestions on how my class can be improved, especially when it comes to error handling?

Answer 1

I like your code. The public interface is mostly well thought out, it seems easy to use, the code is mostly well structured and readable, well commented, well formatted, etc.

A couple of issues do exist though:

Security: PHP File Upload

It is possible to bypass finfo check (depending on the system), and thus to upload files with a dangerous type, such as PHP files.

You should get the extension from the file name, not from a mimetype check.

You could add an additional mimetype check, but the extension check needs to be independent of it.

Security: Weak Extension Check

Now, even if your extension check would work, it is rather weak as it doesn't check for equality. Instead, it checks if the extension exists in a given string, which is a weaker check.

For example, a user may want to allow phtml (which doesn't lead to code execution with most current server configurations), but not pht (which does lead to code execution with most current server configurations). The user will set 'ext_whitelist' => 'foobar|phtml', and they will expect that phtml files can be uploaded, and that pht files cannot. But that isn't what happens.

Misc

• process and run are a bit generic. run could probably be upload, and process may be checkValidity or something.
• $defaults should be renamed, to either $defaultConfig or $defaultRules (whichever one is actually correct).
• Do you really need separate process and run methods? It seems to just complicate the interface. Or is there ever a situation in which a user does want to call one, but not the other? If not, just make both private, and add a new upload method which calls both.
• The difference between config and rules isn't quite clear to me. Config seems to be the default rules + the upload path? Or just the default rules? If that is the case, setConfig should really be named setDefaultRules (no reason to confuse a user with config, if there isn't anything to configure except rules). If that's wrong, the interface and documentation should make the difference clearer.
• $rules doesn't just contain rules, but actually contains the file reference + its associated rules. The naming here is a bit confusing. Reading foreach ($this->rules as $name => $rules), it looks as if you were iterating over rules, while you are actually iterating over files.
• magic numbers: they make code hard to understand. What's 1048576? I would save it in a properly named variable, or at a minimum add a comment (or even a function) which makes it clear what is happening here.

• Mikey: I must have been living in a bubble as I never heard of phtml or pht. Great suggestions and critiques! I stupidly spent a longtime trying to figure out appropriate names for the variables and functions. Much appreciated.