1
\$\begingroup\$

I am creating the dashboard to create an ad campaign. The user needs to upload a campaign name, campaign title, and up to 3 images. These 3 images will have designated "slots" on the page so it makes more sense to me to use separate file uploads as opposed to one multiple file selector. But they also need to be able to opt for only 1 or 2 images as well. I wrote the PHP and it works but it quickly got ugly:

PHP:

<?php
  header("Cache-Control: no-cache, must-revalidate");
  $errorMessage = false;
  $target_dir = "campaignImages/";
  $servername = "localhost";
  $username = **username**;
  $password = **password**;
  $dbname = "swimmin0_SwimSpot";
  //Check if it is a form submission
  if (isset($_POST['submit'])) { 
    try {
      //First validate that name and title are entered
      if((isset($_POST['campaignName']) && !empty($_POST["campaignName"])) &&
        (isset($_POST['campaignTitle']) && !empty($_POST["campaignTitle"]))) {
        //Validate that name and title aren't too long
        if (strlen($_POST['$campaignName'] >50)){
          $errorMessage = 'Author is limited to 25 characters';
        }
        else if (strlen($_POST['$campaignTitle'] > 100)){
          $errorMessage = 'Title is limited to 50 characters';
        }
        else{
          $fileErrors = false;
          //Call validate image for first image
          if (isset($_FILES["imageOne"])){
             if (validateImage($_FILES["imageOne"])){ $fileErrors = true;}
          }
          //Call validate image for second image
          if (isset($_FILES["imageTwo"])){
            if (validateImage($_FILES["imageTwo"])){$fileErrors = true;}
          }
          //Call validate image for third image
          if (isset($_FILES["imageThree"])){
            if (validateImage($_FILES["imageThree"])){ $fileErrors = true;}
          }
          //If no errors, create the campaign on the database
          if (!$fileErrors){
            $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $conn->prepare("INSERT INTO `Campaigns` (campaignName, campaignTitle) VALUES (:campaignName, :campaignTitle)");
            $stmt->bindParam(':campaignName', $campaignName);
            $stmt->bindParam(':campaignTitle', $campaignTitle);
            $campaignName = $_POST["campaignName"];
            $campaignQuestion = $_POST["campaignTitle"];
            $stmt->execute();
            $lastId = $conn->lastInsertId();

            //Use the ID to create an entry image in the Images database, and upload images
            $stmt = $conn->prepare("INSERT INTO `CampaignImages` (campaignId, imagePath ) VALUES (:campaignId, :imagePath)");
            if (isset($_FILES["imageOne"])){
               $target_file = $target_dir  .basename($_FILES["imageOne"]["name"]);
               $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
               $newpath = $target_dir . uniqid() . "." .$imageFileType;
               if (move_uploaded_file($_FILES["imageOne"]["tmp_name"],$newpath)) {
                 $stmt->bindParam(':imagePath', $newpath);
                 $stmt->bindParam(':campaignId', $lastId);
                 $stmt->execute();
               }
            }
            if (isset($_FILES["imageTwo"])){
                $target_file = $target_dir  .basename($_FILES["imageTwo"]["name"]);
                $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
                $newpath = $target_dir . uniqid() . "." .$imageFileType;
                if (move_uploaded_file($_FILES["imageTwo"]["tmp_name"],$newpath)) {
                  $stmt->bindParam(':imagePath', $newpath);
                  $stmt->bindParam(':campaignId', $lastId);
                  $stmt->execute();
                }
            }
            if (isset($_FILES["imageThree"])){
                $target_file = $target_dir  .basename($_FILES["imageThree"]["name"]);
                $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
                $newpath = $target_dir . uniqid() . "." .$imageFileType;
                if (move_uploaded_file($_FILES["imageThree"]["tmp_name"],$newpath)) {
                  $stmt->bindParam(':imagePath', $newpath);
                  $stmt->bindParam(':campaignId', $lastId);
                  $stmt->execute();
                }
            }
          }
          else{
            $errorMessage="Could not upload images";
          }
             }
    }
    else{
      $errorMessage = "Please give your campaign a title and author";
    }
  }
    catch(PDOException $e)
    {
    echo $e;
        $errorMessage =  "Sorry, this Doodle could not be submitted";
    }
    $conn = null;
}

function validateImage($image){
  $validateError = false;
  $target_file = $target_dir  .basename($image["name"]);
  $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
  //Check if file exists
  if (file_exists($target_file)) {
    $validateError = "Sorry, file already exists.";
  }
  // Check file size
  if ($image["size"] > 5000000) {
    $validateError = "Sorry, your file is too large.";
  }
  // Allow certain file formats
  if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg") {
    $validateError = "Sorry, only JPG, JPEG, & PNG files are allowed.";
  }
  echo $validateError;
  return $validateError;
}
?>

HTML:

<!DOCTYPE html>
<html lang="en">
    <head></head>
    <body class="uploadPage">           
        <form  method="post" enctype="multipart/form-data"  class="createFlyer">
            <input class="styledInput" id="campaignName" name="campaignName" placeholder="Campaign Name" maxlength="50" type="text" />
            <input class="styledInput" id="campaignTitle" name="campaignTitle" placeholder="Campaign Title" maxlength="100" type="text" />
            <div class="imageUpload">
                <p>Upload image</p>
                <input type="file" name="imageOne" id="imageOne">
                <input type="file" name="imageTwo" id="imageTwo">
                <input type="file" name="imageThree" id="imageOne">
            </div>
            <p style="color:red;">
            <?php if ($errorMessage != null){echo $errorMessage; }?><p>
            <input name = "submit" type="submit" value="Upload Campaign"/>
        </form>
            </div>
        </div>
    </body>
</html>

The reason my PHP form is ordered the way it is, I don't want to allow the campaign to be created until everything is validated. But I need the campaign to be created first so I can use its ID in the image table. So I validate first including the images, then create the campaign table, then upload the images.

\$\endgroup\$
2
\$\begingroup\$

Consider renaming your file input fields to images[] (note that array notation). That will allow you to access all your $_FILES values like an array, allowing you to simplify your code. I will also take the addvantage of the fact that bindParam() binds the a variable to a parameter not a value, meaning you don't need to rewrite these lines of code repeatedly.

For example,

// after your statement is prepared
$stmt->bindParam(':imagePath', $filepath);
$stmt->bindParam(':campaignId', $lastId);
foreach($_FILES['images']['name'] as $filename) {
    // Calculate $filepath as you do now.
    // No need to re-bind in this loop at all as whatever
    // value is currently set to $filepath variable will be used.
    $stmt->execute();
}

You are duplicating work in your validateImages() function and in making your database inserts. Consider a method like getImagePaths(). To both validate the images and read them into an array that can be used directly in code shown above (i.e. removing need to recalculate file location on server), or provide false result on validation error. So perhaps a usage pattern like this:

$imagePaths = getImagePaths($_FILES);
if($images === false) {
    // error & exit
}

// do database inserts using $imagePaths

Invert your conditionals to clean up your nesting.

For example:

// at main conditional
if(!isset($_POST['submit'])) {
    // error if needed, and exit execution
}

// checked posted data
if(empty($_POST['campaignName']) || strlen($_POST['campaignName') > 50) {
    // error & exit
}
// similar for campaignTitle

$imagePaths = getImagePaths($_FILES);
if($imagePaths === false) {
    // error & exit
}

// database inserts, note your code now has NO NESTING

In your validateImage() function you are both outputting and returning an error message. This is odd behavior as typically a function should do one or the other, not both. Why would you return the error message to the caller if a message had already been output. I would suggest that typically you would not want a function like this to directly output to standard out. Leave it up to the calling code to determine how and when to message the user, as it is better positioned to do so.

This sort of spurious output can also be problematic when building more complex applications, as you may need to do things like set headers (including cookies) and such throughout your code and having spurious output like this (rather than all output being delivered to browser at one specific point in the application) is going to make your application hard to maintain.


I would suggest you move away from hardcoding DB credentials into files such as this. DB credentials should ideally be defined in configuration outside of your codebase.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$
    try {

You put this around more text than it really needed. It could go just inside

          if (!$fileErrors){

As all the database stuff is in there.

            //Use the ID to create an entry image in the Images database, and upload images
            $stmt = $conn->prepare("INSERT INTO `CampaignImages` (campaignId, imagePath ) VALUES (:campaignId, :imagePath)");
            if (isset($_FILES["imageOne"])){
               $target_file = $target_dir  .basename($_FILES["imageOne"]["name"]);
               $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
               $newpath = $target_dir . uniqid() . "." .$imageFileType;
               if (move_uploaded_file($_FILES["imageOne"]["tmp_name"],$newpath)) {
                 $stmt->bindParam(':imagePath', $newpath);
                 $stmt->bindParam(':campaignId', $lastId);
                 $stmt->execute();
               }
            }
            if (isset($_FILES["imageTwo"])){
                $target_file = $target_dir  .basename($_FILES["imageTwo"]["name"]);
                $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
                $newpath = $target_dir . uniqid() . "." .$imageFileType;
                if (move_uploaded_file($_FILES["imageTwo"]["tmp_name"],$newpath)) {
                  $stmt->bindParam(':imagePath', $newpath);
                  $stmt->bindParam(':campaignId', $lastId);
                  $stmt->execute();
                }
            }
            if (isset($_FILES["imageThree"])){
                $target_file = $target_dir  .basename($_FILES["imageThree"]["name"]);
                $imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
                $newpath = $target_dir . uniqid() . "." .$imageFileType;
                if (move_uploaded_file($_FILES["imageThree"]["tmp_name"],$newpath)) {
                  $stmt->bindParam(':imagePath', $newpath);
                  $stmt->bindParam(':campaignId', $lastId);
                  $stmt->execute();
                }
            }

This could be

            $stmt = $conn->prepare("INSERT INTO `CampaignImages` (campaignId, imagePath ) VALUES (:campaignId, :imagePath)");
            saveImage('imageOne', $target_dir, $lastId, $stmt);
            saveImage('imageTwo', $target_dir, $lastId, $stmt);
            saveImage('imageThree', $target_dir, $lastId, $stmt);

with

function saveImage($key, $target_dir, $lastId, $stmt) {
    if (isset($_FILES[$key])) {
        $target_file = $target_dir . basename($_FILES[$key]['name']);
        $imageFileType = pathinfo($target_file, PATHINFO_EXTENSION);
        $newpath = $target_dir . uniqid() . '.' . $imageFileType;
        if (move_uploaded_file($_FILES[$key]['tmp_name'], $newpath)) {
            $stmt->bindParam(':imagePath', $newpath);
            $stmt->bindParam(':campaignId', $lastId);
            $stmt->execute();
        }
    }
}

The rough rule of thumb is that the first time you do something, you write it. The second time, you copy it and edit to make it work (e.g. change imageOne to imageTwo). The third time, you refactor so as not to need it.

We could actually go further and put all this in a class or classes to manage the database connection and saving. That would also allow us to call just

$image_store = new Image_Store($target_dir, $campaign_id);
$image_store->save('imageOne');

with proper setup for the $imageStore object.

I changed $lastId to $campaign_id as more descriptive and consistent with the snake_case naming of $target_dir. I happen to prefer snake_case for being easier to read internationally, so I standardized on that. The most important thing is to be consistent.

I moved $stmt entirely into the constructor. So now, we just call Image_Store with the right parameters initially, and we only need to call save with the key for the $_FILES array.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.