Class upload

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
webgroundz
Forum Commoner
Posts: 58
Joined: Thu Jun 21, 2007 1:20 am
Location: Philippines

Class upload

Post by webgroundz »

This is my class upload, it is simply a class that upload a file depending on the allowed file types.
any comments or suggestions is highly appreciated..thanks all...

Code: Select all

<?php
class Upload
{
	
	public $path, $field, $allowed_types, $filename, $max_size;
	protected $error_messages = array();
	
	/**
	 * Initialize the parameter for file upload
	 *
	 * @param array $config
	 * 
	*/
	public function __construct($config = array())
	{
		foreach($config as $conf => $value)
		{
			$this->$conf = $value;
		}
	}
	
	/**
	 * Execute The Uploading Of File
	 * @access  public
	 */
	public function uploadFile()
	{
		$file = pathinfo($this->getUploadedFileName());
		//get file name
		$filename =  $file['filename'] = $this->filename;
		//get file type
		$type = $file['extension'];
		//get file path
		$file_path = $this->path . '/';
		//get all file name, type and path	
		$file_path = $file_path . $filename . "." . $type;
		
		//check if the file browser has a file selected
		if($this->getUploadedFileName() != null)
		{
			//validate if the type is known in the array
			if($this->isAllowedTypes($type))
			{	
				if(is_executable($file_path))
				{
					$this->error_messages[] = "This File is Not Allowed to upload Due To Some Security Reason!";	
				}
				else 
				{
					//check if the the document file size is not reach the maximum file size
					if($this->getUploadedFileSize() <= $this->max_size)
					{
						//upload document if no errors occur
						if(move_uploaded_file($this->getUploadedTemporaryName(), $file_path))
						{
						 	echo "The file ".  basename($this->getUploadedFileName()). " has been uploaded";
						} 
					}
					else 
					{
						$this->error_messages[] = "You Have Reached The Maximum File Size Limit!";	
					}
				}	
			}		
			else 
			{
				$this->error_messages[] = "Unknown File Type";	
			}	
		}
		else 
		{
			$this->error_messages[] = "No File Selected";	
		}
	}
	
	/**
	 * Validate If Uploading File Has an Errors
	 *
	 * @return bool
	 */
	public function isUploaded()
	{
		$count_error = count($this->displayErrorMessages());
		$return = true;
		if($count_error > 0)
		{
			$return = false;
		}
			return $return;		
	}
	
	private function isExecutable()
	{
		$return = true;
		if (is_executable($this->getUploadedFileName())) 
		{
   		 	$return = false;
		} 
		    return $return;
	}
	
	/**
	 * Get Upload Path
	 *
	 * @access	public
	 * @return	string
	 * 
	 */	
	public function getFilePath()
	{
 		return $this->path;
	}
	
	/**
	 * Get File Name of the file being uploaded
	 * @access public
	 * @return string 
	 */
	public function getFileName()
	{
		return $this->filename;		
	}
	
	/**
	 * Filter all allowed file types
	 *
	 * @param string $file_extension
	 * @return bool
	 */
	public function isAllowedTypes($file_extension)
	{
		$this->allowed_types = explode('|', $this->allowed_types);
		if(is_array($this->allowed_types))
		{
			$return = false;
			if(in_array(strtolower($file_extension), $this->allowed_types))	
			{
				$return = true;
			}
				return $return;
		}
	}
	
	/**
	 * Get Document File Size
	 * 
	 * @return int
	 */
	private function getUploadedFileSize()
	{
		return $_FILES[$this->field]['size'];
	}
	
	/**
	 * Get Document Name
	 *
	 * @return string
	 */
	private function getUploadedFileName()
	{
		return $_FILES[$this->field]['name'];
	}
	
	/**
	 * Get Temporary Document Name
	 *
	 * @return string
	 */
	private function getUploadedTemporaryName()
	{
		return $_FILES[$this->field]['tmp_name'];
	}
	
	/**
	 * Get error messages
	 * 
	 * @return array
	 */
	private function getErrorMessages()
	{
		return $this->error_messages;
	}
	
	/**
	 * Display error message depends on the type of error
	 *
	 * @return string
	 */
	public function displayErrorMessages()
	{
		foreach($this->getErrorMessages() as $error)
		{
			$str .= $error;
			$str .= "<br>";
		}
		return $str;
	}
		
}

//call form class
include 'form.php';
$form = new Form;
if($_POST['submit'])
{
	//call upload class
	$config['path'] = 'test';
	$config['field'] = 'uploaded_file';
	$config['filename'] = 'tolits';
	$config['allowed_types'] = 'jpg|jpeg|gif|png|doc|pdf|html|exe';
	$config['max_size'] = 350000;
	
	$uploader = new Upload($config); 
/*	$uploader->uploadFile();
	echo $uploader->displayErrorMessages();*/
	
	$uploader->uploadFile();
    if (!$uploader->isUploaded())
	{
    	echo $uploader->displayErrorMessages();
    }
	
}	

//begin form
echo $form->startMultipartForm('form_upload', 'upload_file.php', 'post');
//echo '<form action=upload_file.php method=post>';
echo $form->fileBrowser('uploaded_file');
echo '<input type=submit name=submit value=submit>';
//end form
$form->endForm();
?>
:D :D :D :D :D :D :D
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

A file extension doesn't necessarily make a file that type. Be careful.
User avatar
webgroundz
Forum Commoner
Posts: 58
Joined: Thu Jun 21, 2007 1:20 am
Location: Philippines

Post by webgroundz »

ah ok thanks feyd, so what do you think i have to go in that way.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

You can provide two ways to authenticate the file. One being extension, the other being a developer supplied callback (or object) that will accept the actual file path (temporary file) and authenticate based on the file's content.

mime_content_type(), getimagesize() among others may be of interest in the implementation of these.
User avatar
webgroundz
Forum Commoner
Posts: 58
Joined: Thu Jun 21, 2007 1:20 am
Location: Philippines

Post by webgroundz »

thanks feyd for the info, i will work on it... :wink:
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Looks pretty good - the only big problem was already mentioned by ~feyd. I found a few housekeeping recommendations:
  1. I'd recommend not using an array to pass the config parameters, but use separate parameters. In my opinion, doing it that way is a little more proper.
  2. I'd also suggest setting those variables to default values & have accessor methods (setPath(), setField(), etc) to change them.
  3. Why not have the allowed filetypes be an array rather than a string? It'll be easier to handle.
  4. In your isUploaded() function, I think you mean to call getErrorMessages() rather than displayErrorMessages()
  5. You can make your isUploaded() & isExecutable() functions smaller:

    Code: Select all

    function isUploaded()
    {
      return (count($this->getErrorMessages())) ? FALSE : TRUE;
    }
    
    function isExecutable()
    {
      return (is_executable($this->getUploadedFileName())) ? TRUE: FALSE ;
    }
    
  6. The way you've got it set up, isExecutable() returns boolean FALSE if the function is executable - shouldn't it be TRUE (I've made that correction in my version of the function above)
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Code: Select all

function isUploaded()
{
  return (bool) count($this->getErrorMessages()));
}

function isExecutable()
{
  return (is_executable($this->getUploadedFileName()));
}
:P
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

The only thing is_executable tells you is if the file has execute permissions, or if a folder has traverse permissions. Not whether the file itself is an executable.
User avatar
zyklone
Forum Commoner
Posts: 29
Joined: Tue Nov 28, 2006 10:25 pm

Post by zyklone »

astions wrote:The only thing is_executable tells you is if the file has execute permissions, or if a folder has traverse permissions. Not whether the file itself is an executable.
is_executable — Tells whether the filename is executable.

see http://www.php.net/function.is_executable
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

zyklone wrote:is_executable — Tells whether the filename is executable.

see http://www.php.net/function.is_executable
It's permissions, not the typical "executable" many think about.
User avatar
webgroundz
Forum Commoner
Posts: 58
Joined: Thu Jun 21, 2007 1:20 am
Location: Philippines

Post by webgroundz »

my intention of using is_executable() is to filter out all the executable (*.exe) file for security reason,

for example there is some user want to upload a .jpg file then if you double click that file it contains a malicious script that is an executable.

what do you think would be the best function to filter out that scenario?.thanks
Post Reply