0
\$\begingroup\$

I'm working on an upload file program in PHP in OOP style. I need some feedback about code.

index.php

<?php require_once('./lib/upload.php'); ?> <?php if(isset($_FILES['file'])){ $fileupload = new upload(); if(!$fileupload -> sizeck()){ if($fileupload -> extens()){ if($fileupload -> uploadfile()){ echo 'Fisierul a fost uploadat'; } } } } ?> <html> <head></head> <body> <form align="center" enctype="multipart/form-data" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]); ?>" method="post"> Select upload file: <input type="file" name="file" required="yes" /> <input type="submit" value="Trimite" /> <p> <p> <p> <br>If you want to view file and download <a href="./upload/"> click here </a> </form> </body> </html>

lib/upload.php

<?php class upload{ public $src = "upload/"; public $tmp; public $filename; public $typefl; public $uploadfile; public $type = array("php", "css", "js", "html", "htm", ".php"); function __construct(){ $this -> filename = $_FILES["file"]["name"]; $this -> tmp = $_FILES["file"]["tmp_name"]; $this -> uploadfile = $this -> src . basename($this -> filename); } public function sizeck(){ if($_FILES["file"]["size"] > 50000000){ echo "Fisier prea mare"; return true; } } public function extens(){ $this -> typefl = pathinfo($this -> filename, PATHINFO_EXTENSION); if(in_array($this -> typefl, $this -> type)){ echo "Fisier nepermis!!!"; return false; } else{ return true; } } public function uploadfile(){ if(move_uploaded_file($this -> tmp, $this -> uploadfile)){ return true; } } } ?>
\$\endgroup\$
5
  • \$\begingroup\$Just want to mention... Are you allowing uploads of .php files? Because that seems very dangerous. You might want to include some kind of MIUME checker. There are a couple ones out there using CURL to get the actual MIME type, but uploads are always dangerous.\$\endgroup\$
    – somethinghere
    CommentedJul 16, 2015 at 11:05
  • \$\begingroup\$No, i don't allow upload php. "public $type = array("php", "css", "js", "html", "htm", ".php");"\$\endgroup\$
    – cindea sorin
    CommentedJul 16, 2015 at 11:07
  • \$\begingroup\$Ah sorry, skim reading caught me on that one.\$\endgroup\$
    – somethinghere
    CommentedJul 16, 2015 at 11:08
  • \$\begingroup\$@cindeasorin It maybe an idea to change your type checker to contain allowed types rather than disallowed types. In this currency file I could upload an .exe file, .bat, .sh ect.... which you certainly do not want on your server.\$\endgroup\$
    – Liam Sorsby
    CommentedJul 22, 2015 at 15:40
  • \$\begingroup\$Yes, i make this change, and i change more in script.\$\endgroup\$
    – cindea sorin
    CommentedJul 24, 2015 at 21:04

1 Answer 1

Reset to default
2
\$\begingroup\$

Your code is not really object oriented, but here are some improvements:

  • Use upper case names for classes (see CamelCase)
  • Do not use an empty spaces between varibale $object and the method pointer ->, use e. g. $object->method()
  • Do not use global variables like $_FILES in your classes. You should add a parameter which contains the file.
  • Use human readable method names, like e. g. verifyExtension() instead of extens()
  • Make your target uploading path on your server configurable (reusability)
  • Do not use too many nested if conditions.
  • Simpilfy your if nesting, e. g. combine your conditions and make configurable
  • You can seperate your Validation and Upload, in two classes e. g. FileValidation and FileUpload
  • You do not use your type checking attribute $type (type checking is missing)
  • Do not use echo in your file uploading and validation classes.

These are only the most important improvements.

\$\endgroup\$

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.