Adding artist info into MySQL

Question

This takes a name, 2 images and a birthday for an artist, uploads images and insert info into MySQL. I seem to be missing something and I'm looking to see if I'm following best practices.

<?php
require_once '../config.php';
require 'includes/functions.php';
?>

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8" />
        <title>Admin Area</title>
    </head>
    <body>

        <h1>Add Artist</h1>
<?php
    if (isset($_FILES['artist_thumb'], $_FILES['artist_profile_image']) === true) {

        $thumb_name = $_FILES['artist_thumb']['name'];
        $profile_image_name = $_FILES['artist_profile_image']['name'];
        // Checking if extension allowed
        $allowed_ext = array('jpeg','jpg','gif','png');
        $thumb_ext = explode('.',$thumb_name);
        $thumb_ext = strtolower(end($thumb_ext));
        $profile_image_ext = explode('.',$profile_image_name);
        $profile_image_ext = strtolower(end($profile_image_ext));

        $errors =array();
        if (in_array($thumb_ext, $allowed_ext) === false || in_array($profile_image_ext, $allowed_ext) === false) {
            $errors[] = 'File type not allowed';
        }
        // If there is any error echo it
        if (!empty($errors)) {
            echo output_errors($errors);
        } else {
            // Upload image
            $thumb_tmp = $_FILES['artist_thumb']['tmp'];
            $profile_image_tmp = $_FILES['artist_profile_image']['tmp'];

            $thumb_dir = upload_image($thumb_tmp, $thumb_name);
            $profile_image_dir = upload_image($profile_image_tmp, $profile_image_name);
        }

    }

    if (empty($_POST) === false) {
        $required_fields = array('artist_name', 'artist_thumb', 'artist_profile_image', 'birthday');
        foreach ($_POST as $key => $value) {
              if (empty($value) && in_array($key, $required_fields) === true) {
                 $errors[] = 'Fields marked with an asterstrik are required';
                 break 1;
              }
        }
    }

  if (isset($_GET['success'])) {
     echo 'You\'ve added a new artist.';
  } else {
        if (empty($_POST) === false && empty($errors) === true) {
            $artist_data = array(
                'artist_name'    => $_POST['artist_name'],
                'artist_thumb'    => $thumb_dir,
                'artist_profile_image'  =>  $profile_image_dir,
                'birthday'   => $_POST['birthday'],
            );

            add_artist($artist_data);
            header('Location: add_artist.php?success');
            exit();
        } else if (empty($errors) === false) {
            echo output_errors($errors);
        }
?>
<form action="" method="post" enctype="multipart/form-data">
            <ul>
                <li>
                    Artist Name*: <input type="text" name="artist_name">
                </li>
                <li>
                    Choose an image: <input type="file" name="artist_thumb">
                    <!--Artist Thumbnail*: <input type="text" name="artist_thumb">-->
                </li>
                <li>
                    Choose an image: <input type="file" name="artist_profile_image">
                <!--Artist Profile Image*: <input type="text" name="artist_profile_image">-->
                </li>
                <li>
                    Birthday*:
                <input type="date" name="birthday">
                </li>                    
                <li>
                <input type="submit" value="Add Artist">
                </li>
            </ul>
</form>
        <?php  } ?>

    </body>
</html>

Upload image function:

<?php
    // Upload image function
    function upload_image($tmp,$name) {
            $name = time() . $name;
            $upload_dir = 'images/new/';
            move_uploaded_file($tmp,$upload_dir.$name);
                return $upload_dir . $name;
        }
    ?>

nibra · Accepted Answer · 2013-05-07 21:34:56Z

I'm not a fan of mixing PHP and HTML, but I'll leave that alone for now.

<?php
    if (isset($_FILES['artist_thumb'], $_FILES['artist_profile_image']) === true) {

There is no need for any function returning boolean values to test them against true or false.

    if (isset($_FILES['artist_thumb'], $_FILES['artist_profile_image'])) {

is absolutely sufficient.

In the following code you're doing everythig twice, once for the thumb and once for the profile image. By extracting the upload handling into an own method (function), the maintainability will be enhanced. But let's go through what you have now (removing doubled code for simplicity).

        $thumb_name = $_FILES['artist_thumb']['name'];
        // Checking if extension allowed
        $allowed_ext = array('jpeg','jpg','gif','png');
        $thumb_ext = explode('.',$thumb_name);
        $thumb_ext = strtolower(end($thumb_ext));

        $errors =array();
        if (in_array($thumb_ext, $allowed_ext) === false) {
            $errors[] = 'File type not allowed';
        }

        // If there is any error echo it
        if (!empty($errors)) {
            echo output_errors($errors);
        } else {
            // Upload image
            $thumb_tmp = $_FILES['artist_thumb']['tmp'];
            $thumb_dir = upload_image($thumb_tmp, $thumb_name);
        }

Here again you have a unnecessary test against false.

        if (!in_array($thumb_ext, $allowed_ext)) {

seems much more straight and understandable to me.

The extension check can be simplified by using regular expressions, since you don't use the extension afterwards. The chunk above will then look like

        $thumb_name = $_FILES['artist_thumb']['name'];

        $errors = array();

        // Checking if extension allowed
        if (!preg_match('~(jpeg|jpg|gif|png)$~i', $thumb_name)) {
            $errors[] = 'File type not allowed';
        }

        // If there is any error echo it
        if (!empty($errors)) {
            echo output_errors($errors);
        } else {
            // Upload image
            $thumb_tmp = $_FILES['artist_thumb']['tmp'];
            $thumb_dir = upload_image($thumb_tmp, $thumb_name);
        }
    }

Now we can move this into its own function:

function process_upload($file_tag, $allowed_ext)
{
    $file_name = $_FILES[$file_tag]['name'];

    // Check if extension is allowed
    if (!preg_match('~(' . implode('|', $allowed_ext) . ')$~i', $file_name)) {
        throw new ErrorException('File type not allowed');
    }

    // Upload image
    $file_tmp = $_FILES[$file_tag]['tmp'];
    return upload_image($file_tmp, $file_name);
}

The original chunk is then reduced to

        $errors = array();
        if (isset($_FILES['artist_thumb'], $_FILES['artist_profile_image'])) {
            $allowed_ext = array('jpeg','jpg','gif','png');
            try {
                $thumb_dir         = process_upload('artist_thumb', $allowed_ext);
                $profile_image_dir = process_upload('artist_profile_image', $allowed_ext);
            } catch (ErrorException $e) {
                $errors[] = $e->getMessage();
                echo output_errors($errors);
            }
        }

Next,

    if (empty($_POST) === false) {
        $required_fields = array('artist_name', 'artist_thumb', 'artist_profile_image', 'birthday');
        foreach ($_POST as $key => $value) {
            if (empty($value) && in_array($key, $required_fields) === true) {
                $errors[] = 'Fields marked with an asterstrik are required';
                break 1;
            }
        }
    }

can be simplified by looping through the required fields instead of the POST array:

    if (!empty($_POST) ) {
        $required_fields = array('artist_name', 'artist_thumb', 'artist_profile_image', 'birthday');
        foreach ($required_fields as $key) {
            if (empty($_POST[$key])) {
                $errors[] = 'Fields marked with an asterisk are required';
                break;
            }
        }
    }

After this cleanup, we can start to repair the structure. You are currently handling POST, FILES and GET in a quite unstructured manner. That will most likely give problems, once you want to extend your script. So what we're doing now is to put a structure on it:

Handle GET: if success, say so.
Handle POST: if all required fields are present, do the upload and the database update.
Create the HTML.

The revised code comes here:

<?php
require_once '../config.php';
require 'includes/functions.php';

function process_upload($file_tag, $allowed_ext) // This belongs into functions.php
{
    $file_name = $_FILES[$file_tag]['name'];

    // Checking if extension allowed
    if (!preg_match('~(' . implode('|', $allowed_ext) . ')$~i', $file_name)) {
        throw new ErrorException('File type not allowed');
    }

    // Upload image
    $file_tmp = $_FILES[$file_tag]['tmp'];
    return upload_image($file_tmp, $file_name);
}

$message = '';
$errors  = array();

// Set default switches
$show_form = true;

// Handle GET
if (isset($_GET['success'])) {
    $message  = "You've added a new artist.";
    $show_form = false;
}

// Handle POST
if (!empty($_POST)) {
    try {
        $required_fields = array('artist_name', 'artist_thumb', 'artist_profile_image', 'birthday');
        foreach ($required_fields as $key) {
            if (empty($_POST[$key]) && empty($_FILES[$key]) {
                throw new ErrorException('Fields marked with an asterisk are required');
            }
        }

        // All required fields are there, so we can do the upload
        if (isset($_FILES['artist_thumb'], $_FILES['artist_profile_image'])) {
            $allowed_ext       = array('jpeg','jpg','gif','png');
            $thumb_dir         = process_upload('artist_thumb', $allowed_ext);
            $profile_image_dir = process_upload('artist_profile_image', $allowed_ext);
        }

        // Upload went well, so we can write to the database
        $artist_data = array(
            // Don't forget to escape the values before writing them into the database!
            'artist_name'          => $_POST['artist_name'],
            'artist_thumb'         => $thumb_dir,
            'artist_profile_image' => $profile_image_dir,
            'birthday'             => $_POST['birthday'],
        );
        add_artist($artist_data);
        header('Location: add_artist.php?success');
        exit();

    } catch (ErrorException $e) {
        $errors[] = $e->getMessage();
    }
}
?>

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8" />
        <title>Admin Area</title>
    </head>
    <body>

        <h1>Add Artist</h1>

        <?php
        echo $message;
        if (!empty($errors)) {
            echo output_errors($errors);
        }
        ?>

        <?php if ($show_form) : ?>
            <form action="" method="post" enctype="multipart/form-data">
                <ul>
                    <li>
                        Artist Name*: <input type="text" name="artist_name">
                    </li>
                    <li>
                        Choose an image: <input type="file" name="artist_thumb">
                        <!--Artist Thumbnail*: <input type="text" name="artist_thumb">-->
                    </li>
                    <li>
                        Choose an image: <input type="file" name="artist_profile_image">
                        <!--Artist Profile Image*: <input type="text" name="artist_profile_image">-->
                    </li>
                    <li>
                        Birthday*:
                        <input type="date" name="birthday">
                    </li>
                    <li>
                        <input type="submit" value="Add Artist">
                    </li>
                </ul>
            </form>
        <?php endif; ?>

    </body>
</html>

Thanks alot @nibra, it keeps throwing an error [Fields marked with an asterisk are required] How would you about not mixing html with php? if you have an article or anything that can help that would be appreciated, thanks again, I've been waiting for answer for sometime. — Saeb Masarwa, Commented May 7, 2013 at 19:26
I already separated PHP and HTML - processing first, output last. In my answer, I changed the 'required' test a bit, so it should work now as intended. — nibra, Commented May 7, 2013 at 21:06
thanks again, worked, but I seem to still have a problem with uploading the files. Everything goes and works as it suppose too except the localhost doesn't move the image to the specified location. Thanks again. — Saeb Masarwa, Commented May 7, 2013 at 21:32
Well, that's the job of your upload_image() function. I can't tell anything about it without seeing it. — nibra, Commented May 7, 2013 at 21:36
Please check the return value of move_uploaded_file() and make sure that $upload_dir relly addresses what you think it does (using realpath()). — nibra, Commented May 8, 2013 at 0:49

Stack Exchange Network

Adding artist info into MySQL

1 Answer 1

Your Answer

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

Hot Network Questions

Adding artist info into MySQL

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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