Help me reformat my PHP Validation Code

Question

I have designed a file upload form in PHP where it takes 3 pictures and stores it as a varchar datatype in database and move it to the destination directory.

before uploading or inserting the values to the database i want to make sure that

a) it is of file type image/jpeg

b) the three images should have a different fixed dimensions values (which i am fetching the dimension values from getimagesize(); and validating it)

c) and to check if the duplicate of file name exist in the directory (which i am checking through file_exists())

if any one of the conditions returns false then the script should die()

I have defined the code for it but the problem with my code is, if the last condition returns false then the upper conditions will execute the code and do some operations like move_uploaded_file which i dont want to happen, please have a look at the code

Please check the last 4 if conditions as i want that to reformat.

$w_title = 685;
    $h_title = 50;
    $w_brief = 685;
    $h_brief = 177;
    $w_detail = 685;

        if(empty($_POST['ns_title']) || empty($_FILES["ns_pic_title"]["name"]) || empty($_FILES["ns_pic_brief"]["name"]) || empty($_FILES["ns_pic_detail"]["name"])) {
           echo "<script type=\"text/javascript\">" . "alert(\"Please Fill All the Required Fields\");" . "</script>"; 
           echo "<meta http-equiv=\"refresh\" content=\"0;post-news.php\"/>";
           die();
        }
        else {
            $ns_title = htmlspecialchars(strip_tags(mysql_real_escape_string($_POST['ns_title'])));
            if($_FILES["ns_pic_title"]["type"] == "image/jpeg" && $_FILES["ns_pic_brief"]["type"] == "image/jpeg" && $_FILES["ns_pic_detail"]["type"] == "image/jpeg") {

                $ns_pic_title_loc= $_FILES["ns_pic_title"]["tmp_name"];
                $ns_pic_title_name = $_FILES["ns_pic_title"]["name"];
                list($width_title, $height_title) = getimagesize($ns_pic_title_loc);

                $ns_pic_brief_loc = $_FILES["ns_pic_brief"]["tmp_name"];
                $ns_pic_brief_name = $_FILES["ns_pic_brief"]["name"];
                list($width_brief, $height_brief) = getimagesize($ns_pic_brief_loc);

                $ns_pic_detail_loc = $_FILES["ns_pic_detail"]["tmp_name"];
                $ns_pic_detail_name = $_FILES["ns_pic_detail"]["name"];
                list($width_detail, $height_detail) = getimagesize($ns_pic_detail_loc);

                if(file_exists($ns_target.$ns_pic_title_name)) {
                    echo "<script type=\"text/javascript\">" . "alert(\"File Already Exists, Please Choose a Different Name for the File\");" . "</script>";
                    echo "<meta http-equiv=\"refresh\" content=\"0;post-news.php\"/>";
                    die();
                }

                if(!$width_title == $w_title && !$height_title == $h_title) {
                    echo "<script type=\"text/javascript\">" . "alert(\"Incorrect File Dimension for Title News, please make sure it is (685 X 50)\");" . "</script>";
                    echo "<meta http-equiv=\"refresh\" content=\"0;post-news.php\"/>";
                    die();
                }
                else {
                    move_uploaded_file($ns_pic_title_loc, $ns_target.$ns_pic_title_name);
                }

                if(!$width_brief == $w_brief && !$height_brief == $h_brief) {
                    echo "<script type=\"text/javascript\">" . "alert(\"Incorrect File Dimension for Brief News, please make sure it is (685 X 177)\");" . "</script>";
                    echo "<meta http-equiv=\"refresh\" content=\"0;post-news.php\"/>";
                    die();
                }
                else {
                    move_uploaded_file($ns_pic_brief_loc, $ns_target.$ns_pic_brief_name);
                }
                if(!$width_detail == $w_detail) {
                    echo "<script type=\"text/javascript\">" . "alert(\"Incorrect File Dimension for Detail News, please make sure it is (685 in width)\");" . "</script>";
                    echo "<meta http-equiv=\"refresh\" content=\"0;post-news.php\"/>";
                    die();
                }
                else {
                    move_uploaded_file($ns_pic_brief_loc, $ns_target.$ns_pic_brief_name);
                }

how do i reformat the code so that

a) it should check all the three condition

b) and if any one of it returns false then it should stop the execution at once

thank you

Brandon Horsley · Accepted Answer · 2010-08-23 17:35:04Z

2

I would recommend moving your logic to a function to reduce duplication.

Note: I have no way to test any of this, so won't try, but it should get you started.

function fail($error)
{
  echo '<script type="text/javascript">alert("' . $error . '");</script>';
  echo '<meta http-equiv="refresh" content="0;post-news.php"/>';
}

function valid_image($image, $width, $height = 0)
{
  if ($image['type'] != 'image/jpeg')
  {
    fail('File must be of type image/jpeg');
    return false;
  }

  if(file_exists($ns_target . $image['name']))
  {
    fail('File Already Exists, Please Choose a Different Name for the File');
    return false;
  }

  list($image_width, $image_height) = getimagesize($image['tmp_name']);
  if ($image_width != $width || ($image_height && $image_height != $height))
  {
    fail('Incorrect File Dimension for ' . $image['name'] .
      ', please make sure it is (' . $width .
      ($height ? ' X ' . $height  : ' in width'). ')');
    return false;
  }
  return true;
}

if(empty($_POST['ns_title']) ||
  empty($_FILES["ns_pic_title"]["name"]) ||
  empty($_FILES["ns_pic_brief"]["name"]) ||
  empty($_FILES["ns_pic_detail"]["name"]))
{
  fail('Please Fill All the Required Fields');
  die();
}

if (valid_image($_FILES['ns_pic_title'], 685, 50) &&
  valid_image($_FILES['ns_pic_brief'], 685, 177) &&
  valid_image($_FILES['ns_pic_detail'], 685))
{
  move_uploaded_file($_FILES['ns_pic_title']['tmp_name'],
    $ns_target . $_FILES['ns_pic_title']['name']);

  move_uploaded_file($_FILES['ns_pic_brief']['tmp_name'],
    $ns_target . $_FILES['ns_pic_brief']['name']);

  move_uploaded_file($_FILES['ns_pic_detail']['tmp_name'],
    $ns_target . $_FILES['ns_pic_detail']['name']);
}

answered Aug 23, 2010 at 17:35

Brandon Horsley

8,1741 gold badge32 silver badges31 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Majid Fouladpour Over a year ago

Brandon, I would suggest one change: In fail() use die instead of echo. That way you don't need return statements in valid_image(), also in the current implementation you continue to check all conditions even when an earlier one has failed. If die is used in fail, you'd quit at first exception.

Ibrahim Azhar Armar Over a year ago

+1 This is Brilliant i am still a bit newbie, while i will try and change my code to functions, that looks pretty decent and neat too, i appreciate the pain you took to go through my post and replying it.

Ibrahim Azhar Armar Over a year ago

I am unable to use the fail() in PHP? isnt it supposed to be the PHP function,? i would appreciate if i get any reference from php site, i am unable to find it

Ibrahim Azhar Armar Over a year ago

Just FYI i changed my code to function with reference of your code, and you cannot imagine the amount of code i reduced. this is so awesome.

Majid Fouladpour · Accepted Answer · 2010-08-23 17:50:38Z

1

I can offer a few tips:

No need for if ... else
You can simplify the code by reducing nesting.

This is bad

if(cond) {
  do this;
  die();
} else {
  do other things
}

Change above to:

if(cond) {
  do this;
  die();
}
do other things

If in the above code, all you want to do is to output some text you can further simplify it as

if(cond) die('text to output');
do other things

Check for exception, not conformance
Instead of

if($_FILES["ns_pic_title"]["type"] == "image/jpeg" && ...) {
  do stuff
  ...

Use

if($_FILES["ns_pic_title"]["type"] !== "image/jpeg" || ...) exit;
do stuff
...

Use comments
even if the code is for your personal consumption, use as much comments as you can, virtually no amount of comment is too much.

edited Aug 23, 2010 at 17:50

answered Aug 23, 2010 at 17:44

Majid Fouladpour

30.4k21 gold badges80 silver badges129 bronze badges

4 Comments

Ibrahim Azhar Armar Over a year ago

Thank you for that kind and helpful that, i am on the verge of solving my problem and i am using the exact same pattern suggested by you.. i appreciate your effort.

Majid Fouladpour Over a year ago

@Ibrahim Glad it helped. Take a look at the fail function in Brandon's answer, it is much better than warping your error message in <script type ... every time.

Ibrahim Azhar Armar Over a year ago

I am unable to use the fail() in PHP? isnt it supposed to be the PHP function,? i would appreciate if i get any reference from php site, i am unable to find it

Majid Fouladpour Over a year ago

In PHP you can write your own functions and they are called user-defined. fail is a user defined function writen by Brandon in his answer above (it is lines 1 to 5 of his answer).

Collectives™ on Stack Overflow

Help me reformat my PHP Validation Code

2 Answers 2

4 Comments

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related