Multiple file upload class

Question

I've written this a few days back, which is not too big. Do you see anywhere I can improve my logic?

<?php
class Upload
{
 /*
 TODO: Allow multiple mime groups
 Usage:
 Simplest:
 ----------------------------------------------------------------
 $upload = new Upload();
 $upload->Process();
 ----------------------------------------------------------------
 Basic:
 ----------------------------------------------------------------
 $upload = new Upload();
 if( $uploaded = $upload->Process() ) {
 // Prettify output if you want single line success / error messages
 $pretty = $upload->PrettyPrint( $uploaded );
 // Get success and error messages
 foreach ( $pretty as $data ) {
 // Do something different with each message type (good, bad)
 // Or output all the messages
 foreach( $data as $message ) {
 echo $message .'<br>';
 }
 }
 }
 ----------------------------------------------------------------
 */
 // Make some protected defaults so you can extend it and change them
 protected $file_directory = 'uploads/';
 protected $file_max_size = 30; // MB
 protected $file_keep_name = true;
 // Keep track of errors to handle multiple files
 private $errors = 0;
 private $error_messages = array();
 // Construct
 public function __construct( $directory = NULL, $maxSize = NULL, $keepName = NULL )
 {
 // Directory where the files are stored
 $this->file_directory = ( $directory ? $directory : $this->file_directory );
 // Maximum size a file can be
 $this->file_max_size = ( $maxSize ? ( ( $maxSize * 1024 ) * 1024 ) : ( ( $this->file_max_size * 1024 ) * 1024 ) );
 // To keep the original name when uploading or use the tmp_name
 $this->file_keep_name = ( $keepName ? $keepName : $this->file_keep_name);
 }
 // Process the files being sent to the server for upload
 public function Process( $group = 'image', $element = 'upload' )
 {
 // Our returned array of results when complete
 $all_file_data = array();
 // Check for post
 if ( $_SERVER['REQUEST_METHOD'] !== 'POST' ) {
 return array();
 // Check for uploaded files
 } elseif ( $_SERVER['REQUEST_METHOD'] == 'POST' && count( $_FILES[$element] ) == 0 ) {
 // Fail. post_max_size needs to be increased in php.ini
 // When this happens, php silently blanks out the post data completely, not just $_FILES
 return array( 'bad' => array( 'error_messages' => array( 'The servers maximum allowed post size has been reached') ) );
 }
 // Check if our upload directory exists and actually is a directory
 if ( !file_exists ( $this->file_directory ) ) {
 return array( 'bad' => array( 'error_messages' => array( 'The folder ' . $this->file_directory . ' does not exist') ) );
 } elseif ( !is_dir( $this->file_directory ) ) {
 return array( 'bad' => array( 'error_messages' => array( $this->file_directory . ' is not a directory') ) );
 }
 // Sort the files to a nice readable array
 $files = $this->SortFiles( $_FILES[$element] );
 // To handle an array of groups, check if we have a string and put it into an array to save developer space / time
 if ( !is_array ( $group ) ) {
 $group = array( $group );
 }
 // Start processing the files
 foreach ( $files as $file ) {
 // Set a marker for the errors we have seen so far
 $check_errors = $this->errors;
 // Run our file through a check to verify size, group, mimetype, etc
 $this->Verify( $file, $group );
 // Check to see if we acquired new errors, if we did, silently skip the file and continue processing the rest of the files
 if ( $this->errors == $check_errors ) {
 // No new errors
 // Get extension and name
 $ext = array_reverse( explode( '.', basename( $file['name'] ) ) );
 $name = ( $this->file_keep_name ? $this->file_directory.basename( $file['name'] ) : $this->file_directory.basename( $file['tmp_name'] ).'.'.$ext[0] );
 if ( move_uploaded_file($file['tmp_name'], $name ) ) {
 // Move the file and give back some data
 $all_file_data[] = array (
 'name' => $file['name'],
 'type' => $file['type'],
 'extension' => $ext[0],
 'size' => ( ( $file['size'] / 1024^3 ) / 1024 ),
 'directory' => $this->file_directory,
 'new_name' => $name
 );
 } elseif( !empty( $file['name'] ) ){
 // Could not move the file
 $this->errors++;
 $this->error_messages[] = 'The file "' .$file['name']. '" was not uploaded.';
 $all_file_data[] = array ( 'errors' => $this->errors, 'error_messages' => $this->error_messages);
 }
 } else {
 // The file hit some errors so set the array and reset the errors
 $all_file_data[] = array ( 'errors' => $this->errors, 'error_messages' => $this->error_messages, 'data' => $file);
 $this->errors = 0;
 $this->error_messages = array();
 }
 }
 return $all_file_data;
 }
 // Sort the files into a more readable array
 private function SortFiles( &$files_data )
 {
 $files = array();
 $keys = array_keys ( $files_data );
 for ( $i = 0; $i < count ( $files_data ); $i++ ) {
 foreach ( $keys as $file ) {
 @$files[ $i ][ $file ] = @$files_data[ $file ][ $i ];
 }
 }
 return $files;
 }
 // Run through some checks
 private function Verify( &$file, $groups )
 {
 // Set the mimes
 $mimes = $this->MimeGroups();
 foreach ( $groups as $group ) {
 if( array_key_exists ( $group, $this->MimeGroups() ) ) {
 /* Good mime group */
 // Check errors reported by php
 if( !empty( $file['name'] ) && $file['error'] > 0 ) {
 $this->CheckError( $file['error'], $file['name'] );
 }
 // Check file size
 if( $file['size'] > $this->file_max_size ) {
 $this->errors++;
 $this->error_messages[] = 'The file size for "' .$file['name']. '" exceeds the maximum allowed size';
 }
 // Strictly check the mime type
 if( !empty( $file['name'] ) && !in_array( $file['type'], $mimes[$group], true ) ) {
 $this->errors++;
 $this->error_messages[] = 'The file type is not allowed';
 }
 } else {
 // End here because this would be the developer sending an invalid mime group name
 $this->errors++;
 $this->error_messages[] = 'The specified mime group (' .$group. ') is invalid';
 }
 }
 }
 // Gives you some nicer to manage success and error messages
 public function PrettyPrint( $file_data )
 {
 $good = array();
 $bad = array();
 $total = array();
 foreach ( $file_data as $file ) {
 if( array_key_exists( 'name', $file ) ) {
 $good[] = 'Success: ' . $file['name']. ' The file was uploaded successfully';
 } else {
 $name = @$file['data']['name'];
 $errors = ( count ( $file['error_messages'] ) > 1 ? implode( ', ', $file['error_messages'] ) : $file['error_messages'][0] );
 $bad[] = 'Error: ' . $name . ' -> ' . $errors;
 }
 }
 if(count($good) > 0){ $total['good'] = $good; }
 if(count($bad) > 0){ $total['bad'] = $bad; }
 return $total;
 }
 // Check PHP errors
 private function CheckError( $error_message, $name )
 {
 switch ( $error_message ) {
 case UPLOAD_ERR_INI_SIZE:
 $this->errors++;
 $this->error_messages[] = '-The file size for "' .$name. '" exceeds the maximum allowed size-';
 break;
 case UPLOAD_ERR_NO_FILE:
 $this->errors++;
 $this->error_messages[] = 'No file was provided for upload';
 break;
 case UPLOAD_ERR_NO_TMP_DIR:
 $this->errors++;
 $this->error_messages[] = 'Temporary directory is innaccessible';
 break;
 case UPLOAD_ERR_CANT_WRITE:
 $this->errors++;
 $this->error_messages[] = 'The file "' .$name. '" can not be written to the disk';
 break;
 case UPLOAD_ERR_PARTIAL:
 $this->errors++;
 $this->error_messages[] = 'Only partial data was received for the file "' .$name. '"';
 break;
 default:
 $this->errors++;
 $this->error_messages[] = 'The file "' .$name. '" could not be uploaded';
 break;
 }
 }
 // This is in it's own function at the bottom just because it is a long list
 private function MimeGroups()
 {
 return array(
 'audio' => array (
 'audio/aiff','audio/x-aiff','audio/basic','audio/x-au','audio/mpeg','audio/x-mpequrl','audio/midi',
 'audio/x-mid','audio/x-midi','application/x-midi','music/crescendo','x-music/x-midi','audio/x-mpeg',
 'audio/mpeg3','audio/x-mpeg-3','audio/s3m','audio/wav','audio/x-wav'
 ),
 'video' => array (
 'application/x-troff-msvideo','video/avi','video/msvideo','video/x-msvideo','video/mpeg',
 'video/x-motion-jpeg','video/quicktime','video/x-sgi-movie','video/x-mpeg','video/x-mpeq2a',
 'video/vnd.rn-realvideo','application/x-shockwave-flash'
 ),
 'image' => array (
 'image/bmp','image/x-windows-bmp','image/gif','image/x-icon','image/jpeg','image/pjpeg','image/png','image/tiff','image/x-tiff'
 ),
 'document' => array (
 'application/msword','text/plain','application/pdf','application/mspowerpoint','application/vnd.ms-powerpoint',
 'application/powerpoint','application/x-mspowerpoint','text/richtext','text/vnd.rn-realtext','application/rtf',
 'application/x-rtf','application/plain','application/excel','application/x-excel','application/x-msexcel',
 'application/vnd.ms-excel','application/xml','text/xml'
 ),
 'zip' => array (
 'application/x-bzip','application/x-bzip2','application/x-compressed','application/x-gzip','multipart/x-gzip',
 'application/x-rar-compressed','application/x-tar','application/gnutar','application/x-zip-compressed','multipart/x-zip'
 )
 );
 }
}
?>

Answer 1

Usability

• Your example code should really also contain HTML code. Especially considering that the upload HAS to contain multiple files, as it will not work otherwise.
• The upload should also work with single files (or at the very least show a reasonable error message if it doesn't).
• mime groups should be customizable. If you want to keep it simple, just make the function protected/public, otherwise add setter/adder methods.

Security

$_FILES['file']['type'] is user controlled, and thus not trustworthy. An attacker can easily bypass your type check and upload files of dangerous type, such as PHP files.

What you should do is check the file extension as well as the actual file type. The functions that are generally recommended for this are pathinfo and finfo_file respectively.

Style

Generally, I like your style. Your code is quite readable, so just a couple of minor points:

• Function names should start with a lowercase character.
• You are not consistent with your spacing when calling functions. Sometimes you use a space, eg file_exists ( and sometimes you do not, eg array_reverse(. Consistency would increase the readability of your code. Personally, I would recommend the second style without space.
• Same thing with arrays, sometimes it's array ( and sometimes array(, or with elseif: sometimes elseif(, sometimes elseif (.
• You use quite a lot of superficial parentheses. Eg file_max_size = ( $maxSize ? ( ( $maxSize * 1024 ) * 1024 ) : ( ( $this->file_max_size * 1024 ) * 1024 ) ). It's not necessarily bad, but I think sometimes it's too much and actually hurts readability, eg here: 'size' => ( ( $file['size'] / 1024^3 ) / 1024 ),

Comments

The usability of your class would increase if you would add PHPDoc style comments, especially for the Process function and its arguments.

A lot of your in-code comments aren't really needed, such as Construct, Directory where the files are stored, Check for post, etc.

These kinds of comments are not necessary, as they just repeat what your code already says. They also distract from the actually important comments, thus making your code less readable.

Most of these comments, you can just remove. But some are in places where a comment would actually be helpful, and should be extended.

For example: Sort the files into a more readable array for SortFiles. Here, a comment describing what "more readable" is would be very helpful, because the method name is very generic.

Another example is Run through some checks for Verify. What are "some" checks?

Naming

• PrettyPrint doesn't print, which is confusing. Something like formatMessages might fit better.

• Jesse: My first post on code review and I have to say I am thoroughly impressed. Thank you for your answer.