Help with OOP

[Andy-H]

Hi, I have written a class to handle file uploads but am not very confident with OOP. I think I have got the concepts right - bar one. I wanted to add exception error handeling to this but am unsure how to implement it, just wanted your opinions on how you would go about it?

 

Please also let me know if I've got anything wrong in the concept of this class as I usually try to overcomplicate the classes I write and end up writing more single-purpose stuff than generic plug-and-play kind of stuff, duno if that makes sense?

 

<?php
class uploadManager
{
   protected $_files = array();
   protected $_numUploaded;
   protected $_workingDir;
   
   protected $_uploadDir;
   protected $_type;
   
   protected $_allowedTypes = array(
                                    'image' =>   
                                       array(
                                             'image/gif',
                                             'image/png',
                                             'image/jpeg'
                                             ),
                                    'music' => 
                                       array(
                                             'audio/basic',
                                             'audio/mpeg',
                                             'audio/mp4'
                                             )
                                    );
   
   const MAX_SIZE = 3670016;
   
   public function __construct($files)
   {
      $this->_files       = array_values($files);
      $this->_numUploaded = count($files);
      $this->_workingDir  = getcwd();
      ini_set('upload_max_filesize', number_format( (self::MAX_SIZE / 1048576), 2) . 'M');
   }
   
   public function setType($type)
   {
      $this->_type = array_key_exists(strtolower($type), $this->_allowedTypes) ? strtolower($type) : false;
   }
   
   public function setUploadDir($dir)
   {
      if ( substr($dir, 0, 1) != DIRECTORY_SEPARATOR && substr($this->_workingDir, 0, -1) != DIRECTORY_SEPARATOR )
      {
         $this->_workingDir .= DIRECTORY_SEPARATOR;
      }
      
      if ( substr($dir, 0, -1) != DIRECTORY_SEPARATOR )
      {
         $dir .= DIRECTORY_SEPARATOR;
      }
      
      $this->_uploadDir = $this->_workingDir . $dir;
   }
   
   public function uploadFiles()
   {
      if (!$this->_checkDir())
      {
         return 'Files could not be uploaded, ' . $this->_uploadDir . ' is not a valid directory.';
      }
      
      if (!$this->_checkType())
      {
         return 'Files could not be uploaded, one or more files did not validate as ' . $this->_type . ' files.';
      }
      
      if (!$this->_checkSize())
      {
         return 'Files could not be uploaded, one or more files exceeded the ' . number_format( (self::MAX_SIZE / 1048576), 2) . 'mb size limit.';
      }
      
      $successfulUploads = 0;
      $failedUploads     = 0;
      
      for($i = 0; $i < $this->_numUploaded; $i++)
      {
         $name    = basename($this->_files[$i]['name']);
         $newName = $this->_uploadDir . $name;
         
         if ( !file_exists($newName) )
         {
            if ( move_uploaded_file($this->_files[$i]['tmp_name'], $newName) )
            {
               ++$successfulUploads;
            }
            else
            {
               ++$failedUploads;
            }
         }
         else
         {
            ++$failedUploads;
         }
      }
      $msg  = $successfulUploads   ? $successfulUploads  . ' files uploaded successfully. <br >' : '';
      $msg .= $failedUploads       ? $failedUploads      . ' files could not be uploaded.'       : '';
      return $msg;

   }
   
   protected function _checkType()
   {
      if ($this->_type == false)
      {
         return false;
      }
      for($i = 0; $i < $this->_numUploaded; $i++)
      {
         if ( !in_array($this->_files[$i]['type'], $this->_allowedTypes[$this->_type]) )
         {
            return false;
         }
      }
      
      return true;
   }
   
   protected function _checkSize()
   {
      for($i = 0; $i < $this->_numUploaded; $i++)
      {
         if ( $this->_files[$i]['size'] > self::MAX_SIZE )
         {
            return false;
         }
      }
      
      return true;
   }
   protected function _checkDir()
   {
      return is_dir($this->_uploadDir) ? true : false;
   }
}

 

<?php

include 'class/uploadManager.php';

if (isset($_POST['submit'])):

   $upload = new uploadManager($_FILES);
   
   $upload->setType('image');
   $upload->setUploadDir('images');

   echo $upload->uploadFiles();
endif;
?>
<form enctype="multipart/form-data" action="<?php echo stripslashes(htmlentities($_SERVER['PHP_SELF'], ENT_QUOTES)); ?>" method="POST">
<input type="hidden" name="MAX_FILE_SIZE" value="3670016" />
Choose a file to upload: <input name="uploadedfile" type="file" ><br >
<input name="uploadedfile1" type="file" ><br >
<input type="submit" name="submit" value="Upload File" >
</form>

 

Thanks for any help.

[roopurt18]

...
ini_set('upload_max_filesize', number_format( (self::MAX_SIZE / 1048576), 2) . 'M');
...
<input type="hidden" name="MAX_FILE_SIZE" value="3670016" />

The value declared in the class is not used as the value in the form; therefore you allow the possibility of changing one and forgetting the other.  Also, if I'm remembering correctly, you can not set the maximum upload file size via ini_set() because the PHP script will not execute until after the upload has been finished.

 

if ( !in_array($this->_files[$i]['type'], $this->_allowedTypes[$this->_type]) )

Your private member $this->_files is really just the global $_FILES array, which is fine.  But the point I want to make is that $_FILES['name']['type'] is set by upload agent (i.e. web browser) and can be faked or spoofed.  This could allow an attacker to create their own upload agent that uploads a script and tells you that it's an image.  In this way they could add their own executable code to your server!  The correct way to validate uploaded file types is to pass them through functions that work on those file types.  For example, if you are allowing images to be uploaded, try to open them with the built-in image functions.  If the functions fail then the files are not images.  Googling should provide some examples of this type of validation.

 

As for your initial question about errors via exceptions, just change lines like this:

return 'Files could not be uploaded, ' . $this->_uploadDir . ' is not a valid directory.';

To:

throw new Exception( 'Files could not be uploaded, ' . $this->_uploadDir . ' is not a valid directory.' );

And then surround your class's use inside of try...catch blocks.

 

You could go as far as creating your own exceptions for every possible type of error you expect in your class:

class Exception_NotAnUploadDirection extends Exception { }
class Exception_NotAnAcceptableUploadFileType extends Exception {}

(I'd recommend using better names than I came up with; I'm in a hurry!)

[Andy-H]

Thanks Roopurt18, I have removed the ini_set call, replaced the posted max file size with:

 

<input type="hidden" name="MAX_FILE_SIZE" value="<?php echo uploadManager::MAX_SIZE; ?>" >

 

I understand how to use custom exceptions but I wanted a way to report multiple errors to make it more user friendly. Your example gave me an idea tho I can get it working the way I'd like by making a custom class and passing it an array of the occuring errors

 

I did consider the mime type checking but I don't know how to check music files validity without ffmpeg???

 

Thanks for the help and taking the time to reply

 

Much appreciated.

[roopurt18]

I did consider the mime type checking but I don't know how to check music files validity without ffmpeg???

I'm not authority on music files so I'm not much help.  Although I can reiterate what you probably already know: ffmpeg seems to be the solution here. 

 

As far as generating a list of errors, here is what I usually do:

<?php
function validate() {
  // pretend were in a class-instance context...


  $errors = array();
  if( $this->someCondition() === false ) {
    $errors[ 'someCOndition' ] = 'Some condition was not met.';
  }
  if( $this->someConditionB() === false ) {
    $errors[ 'someCOnditionB' ] = 'Some conditionB was not met.';
  }
  // etc...
  $this->_errors = $errors;
  $n = count( $errors );
  if( $n > 0 ) {
    throw new Exception_CustomErrorException(); // The constructor to custom exception may not require arguments.  
  }
  return true;
}
?>

 

And would be called like:

<?php
try {
  $out = '<p>Save successful!</p>';
  $foo = new MyObject();
  $foo->save(); // assume save() calls validate();
} catch( Exception_CustomErrorException() ) {
    $out = '<p>Errors Encountered:</p><ul><li>' . implode( '</li><li>', $foo->errors() ) . '</li></ul>';
}
?>

 

It doesn't quite look like that in practice but that's the quick and dirty of it.

[Andy-H]

Yeh, thats the kind of thing I was thinking. Thanks, I'll implement that tomorrow

 

Thanks for the help

[ignace]

$upload = new Uploader();
$upload->attach(new ImageUploader('.jpg|.png|.gif'))
       ->attach(new ImageThumbnailer('50%'))
       ->execute('/directory');

 

The upload process varies depending on your context (images, documents, ..) all with different rules (thumbnail to .., remove original, ..), capturing these variations into separate classes leads to MANY non-reusable classes instead you should assemble your implementation.

 

I used the Observer pattern as a simple example but the Decorator pattern is better suited for this process.

[Andy-H]

$upload = new Uploader();
$upload->attach(new ImageUploader('.jpg|.png|.gif'))
       ->attach(new ImageThumbnailer('50%'))
       ->execute('/directory');

 

The upload process varies depending on your context (images, documents, ..) all with different rules (thumbnail to .., remove original, ..), capturing these variations into separate classes leads to MANY non-reusable classes instead you should assemble your implementation.

 

I used the Observer pattern as a simple example but the Decorator pattern is better suited for this process.

 

Thanks, that stuffs way over my head tho. You know any good tutorials I can use to help me understand what you're getting at?

 

I'm not really any good with design patterns. I have used singleton but that was a long time ago.

[ignace]

You know any good tutorials I can use to help me understand what you're getting at?

 

Unfortunately no. On the bright sight though Zend framework already implements this with it's Zend_File component and contains some documentation.

 

What I tried to get at is this: you write your code on a per-case basis something like:

 

source: http://www.blazonry.com/scripting/upload-size.php

if (is_uploaded_file($imgfile))
{
   $newfile = $uploaddir . "/" . $final_filename";
   if (!copy($imgfile, $newfile))
   {
      // if an error occurs the file could not
      // be written, read or possibly does not exist
      print "Error Uploading File.";
      exit();
   }
}

/*== where storing tmp img file ==*/
$tmpimg = tempnam("/tmp" "MKPH");
$newfile = "$uploaddir/scaled.jpg";

/*== CONVERT IMAGE TO PNM ==*/
if ($ext == "jpg") { system("djpeg $imgfile >$tmpimg"); }
else { echo("Extension Unknown. Please only upload a JPEG image."); exit(); }

/*== scale image using pnmscale and output using cjpeg ==*/
system("pnmscale -xy 250 200 $tmpimg | cjpeg -smoo 10 -qual 50 >$newfile");

 

However this only works for this particular case and copying this code to a new project with slight variations in the upload process means it needs to be amended (possibly also for each new case) until at the end you are copy-pasting from so many projects..

 

What I tried to explain to you is slice this code up into distinct parts and put these into separate classes, this way you can assemble your code (instead of copy-pasting it from many sources) like:

 

$upload = new Upload($_FILES['upload']);
$upload->attach(new FileMask('.jpg|.png|.gif')); // only allow jpg, png, and gif
$upload->attach(new ScaleImageTo('50%', $removeOriginal)); // scale them down to 50%
$upload->attach(new ConvertToJpeg());
$upload->uploadTo('/directory');

 

This would go in place of the above code. So instead of hard-coding (like the above example) your code, you can change/assemble your code at run-time.