5
\$\begingroup\$

I'm a complete PHP beginner and would like some feedback on this registration page I've created. Now, the one thing I know right off that bat that's wrong with it is that there's WAY too much commenting. This was for my benefit to help teach myself as I went along and help avoid confusion as I read back through my code.

What this page does in a nutshell: Presents a form for the user to register for the site. Some of the fields are validated. It allows the user to upload an avatar, then the code renames the file, moves it to a directory on the server, then re-sizes it and saves it as a .jpg. Once everything is okay it adds the new user to the database.

<?php
/*Give all POST variables the 'var' prefix*/
import_request_variables("p", "var");
?>
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<link rel="stylesheet" type="text/css" href="styles/styles.css" />
<title>Register</title>
</head>
<body>
<?php
/*Call header*/
include 'header.php';
?>
<!--Registration form.  The 'name' attributes will have 'var' added to 
them to become variables.  The 'value' attribute will be populated only if
there is an error, so the user doesn't have to type everything in again.-->
<form enctype="multipart/form-data" method="post">
    <table id="regTable">
        <tr>
            <td>*Desired Screen Name:</td><td> <input type="text" name="username" value="<?php print $varusername ?>" maxlength="30" autofocus /> </td>
            <td id="usernameast" class="ast">*</td>
        </tr>       
        <tr>
            <td>*Password:</td><td> <input type="password" name="password" value="<?php print $varpassword ?>" /></td>
            <td id="passwordast" class="ast">*</td>
        </tr>
        <tr>
            <td>*Verify Password: </td><td><input type="password" name="verpassword" value="<?php print $varverpassword ?>" /></td>
            <td id="verpasswordast" class="ast">*</td>
        </tr>
        <tr>
            <td>*Email address: </td><td><input type="text" name="email" value="<?php print $varemail ?>" /></td>
            <td id="emailast" class="ast">*</td>
        </tr>
        <tr>
            <td>First Name: </td><td><input type="text" name="firstname" value="<?php print $varfirstname ?>" /></td>
            <td></td>
        </tr>
        <tr>
            <td>Last Name: </td><td><input type="text" name="lastname" value="<?php print $varlastname ?>" /></td>
            <td></td>
        </tr>
        <tr>
            <td>Gender: </td><td><select name="gender">
                            <option value="">Select</option>
                            <option value="male">Male</option>
                            <option value="female">Female</option>
                            <option value="other">Other</option>
                        </select>
            </td>
            <td></td>
        </tr>
        <tr>
            <td>Birthdate: </td>
                <?php
                    include 'birthdate.php';
                ?>
            <td></td>
        </tr>
        <tr>
            <td>City: </td><td><input type="text" name="locationcity" value="<?php print $varlocationcity ?>" /></td>
        <td></td>
    </tr>
    <tr>
        <td>State/Province: </td><td><input type="text" name="locationstate" value="<?php print $varlocationstate ?>" /></td>
        <td></td>
    </tr>
    <tr>
        <td>Country: </td><td><input type="text" name="locationcountry" value="<?php print $varlocationcountry ?>" /></td>
        <td></td>
    </tr>
    <tr>
        <td>Upload an avatar: </td><td><input type="file" name="avatar" /></td>
        <td></td>
    </tr>
    <tr>
        <td></td>
        <td><span style="font-size: .75em;">(Your image should be as close to a perfect square as possible and less than
                2mb in size.<br />
                Accepted file types are .jpg, .gif, and .png)</span></td>
    </tr>
    <tr>
        <td><br /></td>
        <td></td>
    </tr>
    <tr>
        <td></td>
        <td><input type="submit" name="submit" value="Register"/></td>
    </tr>
</table>
</form>
<?php
/*Connect to database*/
mysql_connect("server", "username", "password");
mysql_select_db("databaseName");
/*Leading and trailing whitespace is trimmed from username.*/
$varusername = trim($varusername);
/*Query to check if username already exists in the database*/
$exists = mysql_query("SELECT * FROM users where user_name = '$varusername'");
/*Places the 'exists' query columns into an object variable named '$existsusername'*/
$existsusername = mysql_fetch_object($exists);
/*Query to check if email already exists in the database*/
$exists2 = mysql_query("SELECT * FROM users where email = '$varemail'");
/*Places the 'exists2' query columns into an object variable named '$existsemail'*/
$existsemail = mysql_fetch_object($exists2);
/*Checks if the submit button from the form above has been triggered*/
if(isset($_POST['submit'])) {
    /*Sets $fileext variable to the extension of selected file, then places the
    accepted extensions into an array.  This is used in the next section to 
    ensure the file is of the correct type and size.*/
    if (($_FILES['avatar']['tmp_name'])) {
        $name = $_FILES["avatar"]["name"];
        $srcExt = end(explode(".", $name));
        $allowedexts = array("jpeg", "JPEG", "jpg", "JPG", "gif", "GIF", "png", "PNG");
        $avatarok = true;
    }
    /*Sets the $error variable to the number of errors.*/
    $error = $_FILES['avatar']['error'];
    /*Checks that user-entered data is valid.  If not, an error
    is thrown and asterisk is shown.*/
    /*Checks that user entered both username and password*/
    if($varpassword == ''|| $varusername == '') {
        print "Error: Please input both a username and password.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block'
        document.getElementById('passwordast').style.display = 'block';
        document.getElementById('verpasswordast').style.display = 'block';</script>";
    /*Checks that username is not taken by comparing against '$ob'.*/
} elseif ($existsusername->user_name == $varusername){
    print "Error: Sorry, that username is already taken.  Please select a different one.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block';</script>";
    /*Checks that passwords match.*/
    } elseif ($varpassword != $varverpassword){
        print "Error: Please ensure the passwords match.<br />";
        echo "<script type='text/javascript'>document.getElementById('verpasswordast').style.display = 'block';</script>";
    /*Checks that username contains only letters and numbers.*/
    } elseif (!preg_match("/[a-z0-9_\.-]+$/i", $varusername)) {
        print "Error: Your username can contain only letters and numbers and must be less than 30 characters long.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block';</script>";
    /*Checks that password is at least six characters long.*/
    } elseif (strlen($varpassword) < 6) {
        print "Error: Your password must be at least six characters long.<br />";
        echo "<script type='text/javascript'>document.getElementById('passwordast').style.display = 'block';</script>";
    /*Checks that email is valid.*/
    } elseif (!preg_match("/^[a-z0-9_\.-]+@[a-z0-9_\.-]+\.[a-z0-9\.]{2,6}$/i", $varemail)) {
        print "Error: Please enter a valid email address.<br />";
        echo "<script type='text/javascript'>document.getElementById('emailast').style.display = 'block';</script>";
    /*Checks that email does not already exist in database*/
    } elseif ($existsemail->email == $varemail){
        print "Error: Sorry, that email address is already in use.<br />";
        echo "<script type='text/javascript'>document.getElementById('emailast').style.display = 'block';</script>";
    } elseif ($avatarok == true & !in_array($srcExt, $allowedexts) || $error == 1) {
            print "Your avatar must be a .jpg, .gif, .png and must be smaller than 2mb.";
            $avatarok = false;
    /*If all fields are valid, the password is hashed...*/
    } else {   
        $hashpassword = sha1($varpassword);
        /*The Month, Date and year are placed into variables, concatonated,
         and placed into a single variable to write to the database...*/
        $month = $varmonth;
        $date = $vardate;
        $year = $varyear;
        $birthdate = $year."-".$month."-".$date;
        /*Check if the user selected an avatar.  If they did, the image is moved to the avatar
    folder.  If not, the generic avatar is assigned.*/ 
    if (($_FILES['avatar']['tmp_name'])) {
        $avatarFullPath = '<img alt=Avatar src=images/avatars/' . $varusername . '_avatar />';
        $avatarThumbPath = '<img alt=Avatar src=images/avatars/' . $varusername . '_avatar width=45px height=45px />';
    } else {
        $avatarFullPath = '<img alt=Avatar src=images/avatars/generic.gif />';
        $avatarThumbPath = '<img alt=Avatar src=images/avatars/generic.gif width=45px height=45px />';
    }
    if ($avatarok == true) {
        /*Get the extension of the uploaded file*/
        $name = $_FILES["avatar"]["name"];
        $ext = end(explode(".", $name));
        /*Create full path from $varuserame*/
        $oldImagePath = "images/avatars/" . $varusername . "_avatar." . $ext;
        /*Move uploaded file to avatars directory*/
        move_uploaded_file($_FILES['avatar']['tmp_name'], $oldImagePath);
        /*Resize the image*/
        /*Get uploaded image height and width*/
        $srcSize = getimagesize($oldImagePath);
        /*Create source image based on file extension*/
        switch ($ext) {
            case "jpeg":
            case "jpg": $srcImage = imagecreatefromjpeg($oldImagePath); break;
            case "gif": $srcImage = imagecreatefromgif($oldImagePath); break;
            case "png": $srcImage = imagecreatefrompng($oldImagePath); break;
        }
        /*Create new image*/
        $destImage = imagecreatetruecolor(100, 100);
        /*Resample the image*/
        imagecopyresampled($destImage, $srcImage, 0, 0, 0, 0, 100, 100, $srcSize[0], $srcSize[1]);
        /*Create new path with .jpg extension*/
        $newImagePath = "images/avatars/" . $varusername . "_avatar.jpg";
        /*Save resized image*/
        imagejpeg($destImage, $newImagePath, 85);
        /*Remove images from memory*/
        imagedestroy($srcImage);
        imagedestroy($destImage);
        /*Delete the original file from the server as long as it has a 
         different name than the new one (since if it has the same name, the
         new one will have already overwritten it anyway and we don't want
         to delete the new file.  This also prevents the new file from being 
         deleted in the unlikely event that someone uploads an avatar in 
         the exact "username_avatar.jpg" format.)*/
        if ($oldImagePath != $newImagePath) {
            unlink($oldImagePath);
        }
    }
    /*...and the user is inserted into the users table.*/
    mysql_query("insert into users (user_name, first_name, last_name, email,
        gender, birthdate, location_city, location_state, location_country, password, avatar_full, avatar_thumb)
        values ('$varusername', '$varfirstname', '$varlastname', '$varemail',
            '$vargender', '$birthdate', '$varlocationcity', '$varlocationstate', 
            '$varlocationcountry', '$hashpassword', '$avatarFullPath', '$avatarThumbPath');");
        /*Redirect to home page*/
        print "<script type='text/javascript'>window.location = 'login.php'</script>";
    }
}   
?>
</body>
</html>
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Try to separate your PHP and HTML code as much as possible. Also, move away from the old MySQL code (mysql_query etc) and towards using something like PDO. ALSO I notice that you have JS within PHP...no bueno. Again, separation, separation, separation. \$\endgroup\$
    – jsanc623
    Oct 4 '12 at 20:57
  • \$\begingroup\$ jsanc623, thanks for the quick feedback. Would you mind expanding a bit? By separation do you mean using includes to point to the HTML files? And could you give an example of how to separate my JS from my PHP? For instance, near the very end I have print "<script type='text/javascript'>window.location = 'login.php'</script>"; What would be a better way to implement this? Thanks. \$\endgroup\$
    – caiman
    Oct 4 '12 at 21:04
  • 2
    \$\begingroup\$ Please read Create your own framework... on top of the Symfony2 Components to find out about some best practises and how can you separate HTML presentation from PHP and also keep JS separate. Also read Separation of concerns article from Wikipedia. \$\endgroup\$
    – peterhil
    Oct 4 '12 at 22:23
1
\$\begingroup\$

Ok, this is your form.php code - what the users browse to. I've dropped in comments within the code - and also notice the addition of require_once('pre_header.php') on line 4. This will contain the PHP code at the bottom.

<?php
/*Give all POST variables the 'var' prefix*/
import_request_variables("p", "var");
require_once('pre_header.php');
?>

<!DOCTYPE html>
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <link rel="stylesheet" type="text/css" href="styles/styles.css" />
        <title>Register</title>
    </head>

    <body>

        <?php
        /* We can deduct that we're calling header, no need to tell us. REMOVE THIS COMMENT */
        include 'header.php';
        ?>


        <!-- This is an HTML comment, meaning users (and malicious users) can read them. If you need to 
        put a comment about how the form works, hide it in a PHP comment. -->

        <!--Registration form.  The 'name' attributes will have 'var' added to 
        them to become variables.  The 'value' attribute will be populated only if
        there is an error, so the user doesn't have to type everything in again.-->
        <form enctype="multipart/form-data" method="post">
            <table id="regTable">
                <tr>
                    <td>*Desired Screen Name:</td>
                    <td><input type="text" name="username" value="<?php print $varusername ?>" maxlength="30" autofocus /></td>
                    <td id="usernameast" class="ast">*</td>
                </tr>       
                <tr>
                    <td>*Password:</td>
                    <td><input type="password" name="password" value="<?php print $varpassword ?>" /></td>
                    <td id="passwordast" class="ast">*</td>
                </tr>
                <tr>
                    <td>*Verify Password: </td>
                    <td><input type="password" name="verpassword" value="<?php print $varverpassword ?>" /></td>
                    <td id="verpasswordast" class="ast">*</td>
                </tr>
                <tr>
                    <td>*Email address: </td><td><input type="text" name="email" value="<?php print $varemail ?>" /></td>
                    <td id="emailast" class="ast">*</td>
                </tr>
                <tr>
                    <td>First Name: </td>
                    <td><input type="text" name="firstname" value="<?php print $varfirstname ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>Last Name: </td>
                    <td><input type="text" name="lastname" value="<?php print $varlastname ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>Gender: </td>
                    <td>
                        <select name="gender">
                            <option value="">Select</option>
                            <option value="male">Male</option>
                            <option value="female">Female</option>
                            <option value="other">Other</option>
                        </select>
                    </td>
                    <td></td>
                </tr>
                <tr>
                    <td>Birthdate: </td>
                    <?php include 'birthdate.php'; ?>
                    <td></td>
                </tr>
                <tr>
                    <td>City: </td>
                    <td><input type="text" name="locationcity" value="<?php print $varlocationcity ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>State/Province: </td>
                    <td><input type="text" name="locationstate" value="<?php print $varlocationstate ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>Country: </td>
                    <td><input type="text" name="locationcountry" value="<?php print $varlocationcountry ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>Upload an avatar: </td>
                    <td><input type="file" name="avatar" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td></td>
                    <td>
                        <span style="font-size: .75em;">
                            (Your image should be as close to a perfect square as possible and less than
                            2mb in size.<br />
                            Accepted file types are .jpg, .gif, and .png)
                        </span>
                     </td>
                </tr>
                <tr>
                    <td><br /></td>
                    <td></td>
                </tr>
                <tr>
                    <td></td>
                    <td><input type="submit" name="submit" value="Register"/></td>
                </tr>
            </table>
        </form>
    </body>
</html>

This is the content of pre_header.php - the old PHP code that used to be at the bottom of the code.

<?php

/* Connect to database TODO: THIS IS BAD. USE PDO OR SOME OTHER ORM. */
function connectToMySQL(){
    mysql_connect("server", "username", "password");
    mysql_select_db("databaseName");
}

/* Check if a user exists, PLEASE DO SOME CLEANUP. YOU'RE LETTING YOURSELF OPEN TO SQLINJECTION 
   OR OTHER SQL VULNERABILITIES */
function checkUserExists($varusername){
    /*Leading and trailing whitespace is trimmed from username.*/
    $varusername = trim($varusername);

    connectToMySQL();

    /*Query to check if username already exists in the database*/
    $exists = mysql_query("SELECT * FROM users where user_name = '$varusername'");

    /*Places the 'exists' query columns into an object variable named '$existsusername'*/
    $existsusername = mysql_fetch_object($exists);

    /*Query to check if email already exists in the database*/
    $exists2 = mysql_query("SELECT * FROM users where email = '$varemail'");

    /*Places the 'exists2' query columns into an object variable named '$existsemail'*/
    $existsemail = mysql_fetch_object($exists2);
}




/*Checks if the submit button from the form above has been triggered*/
if(isset($_POST['submit'])) {
    /*Sets $fileext variable to the extension of selected file, then places the
    accepted extensions into an array.  This is used in the next section to 
    ensure the file is of the correct type and size.*/
    if (($_FILES['avatar']['tmp_name'])) {
        $name = $_FILES["avatar"]["name"];
        $srcExt = end(explode(".", $name));
        $allowedexts = array("jpeg", "JPEG", "jpg", "JPG", "gif", "GIF", "png", "PNG");
        $avatarok = true;
    }

    /*Sets the $error variable to the number of errors.*/
    $error = $_FILES['avatar']['error'];

    /*Checks that user-entered data is valid.  If not, an error
    is thrown and asterisk is shown.*/
    if($varpassword == ''|| $varusername == '') {
        print "Error: Please input both a username and password.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block'
        document.getElementById('passwordast').style.display = 'block';
        document.getElementById('verpasswordast').style.display = 'block';</script>";
    /*Checks that username is not taken by comparing against '$ob'.*/
} elseif ($existsusername->user_name == $varusername){
    print "Error: Sorry, that username is already taken.  Please select a different one.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block';</script>";
    /*Checks that passwords match.*/
    } elseif ($varpassword != $varverpassword){
        print "Error: Please ensure the passwords match.<br />";
        echo "<script type='text/javascript'>document.getElementById('verpasswordast').style.display = 'block';</script>";
    /*Checks that username contains only letters and numbers.*/
    } elseif (!preg_match("/[a-z0-9_\.-]+$/i", $varusername)) {
        print "Error: Your username can contain only letters and numbers and must be less than 30 characters long.<br />";
        echo "<script type='text/javascript'>document.getElementById('usernameast').style.display = 'block';</script>";
    /*Checks that password is at least six characters long.*/
    } elseif (strlen($varpassword) < 6) {
        print "Error: Your password must be at least six characters long.<br />";
        echo "<script type='text/javascript'>document.getElementById('passwordast').style.display = 'block';</script>";
    /*Checks that email is valid.*/
    } elseif (!preg_match("/^[a-z0-9_\.-]+@[a-z0-9_\.-]+\.[a-z0-9\.]{2,6}$/i", $varemail)) {
        print "Error: Please enter a valid email address.<br />";
        echo "<script type='text/javascript'>document.getElementById('emailast').style.display = 'block';</script>";
    /*Checks that email does not already exist in database*/
    } elseif ($existsemail->email == $varemail){
        print "Error: Sorry, that email address is already in use.<br />";
        echo "<script type='text/javascript'>document.getElementById('emailast').style.display = 'block';</script>";
    } elseif ($avatarok == true & !in_array($srcExt, $allowedexts) || $error == 1) {
            print "Your avatar must be a .jpg, .gif, .png and must be smaller than 2mb.";
            $avatarok = false;
    /*If all fields are valid, the password is hashed...*/
    } else {   
        $hashpassword = sha1($varpassword);
        /*The Month, Date and year are placed into variables, concatonated,
         and placed into a single variable to write to the database...*/
        $month = $varmonth;
        $date = $vardate;
        $year = $varyear;
        $birthdate = $year."-".$month."-".$date;
        /*Check if the user selected an avatar.  If they did, the image is moved to the avatar
    folder.  If not, the generic avatar is assigned.*/ 
    if (($_FILES['avatar']['tmp_name'])) {
        $avatarFullPath = '<img alt=Avatar src=images/avatars/' . $varusername . '_avatar />';
        $avatarThumbPath = '<img alt=Avatar src=images/avatars/' . $varusername . '_avatar width=45px height=45px />';
    } else {
        $avatarFullPath = '<img alt=Avatar src=images/avatars/generic.gif />';
        $avatarThumbPath = '<img alt=Avatar src=images/avatars/generic.gif width=45px height=45px />';
    }
    if ($avatarok == true) {
        /*Get the extension of the uploaded file*/
        $name = $_FILES["avatar"]["name"];
        $ext = end(explode(".", $name));
        /*Create full path from $varuserame*/
        $oldImagePath = "images/avatars/" . $varusername . "_avatar." . $ext;
        /*Move uploaded file to avatars directory*/
        move_uploaded_file($_FILES['avatar']['tmp_name'], $oldImagePath);
        /*Resize the image*/
        /*Get uploaded image height and width*/
        $srcSize = getimagesize($oldImagePath);
        /*Create source image based on file extension*/
        switch ($ext) {
            case "jpeg":
            case "jpg": $srcImage = imagecreatefromjpeg($oldImagePath); break;
            case "gif": $srcImage = imagecreatefromgif($oldImagePath); break;
            case "png": $srcImage = imagecreatefrompng($oldImagePath); break;
        }
        /*Create new image*/
        $destImage = imagecreatetruecolor(100, 100);
        /*Resample the image*/
        imagecopyresampled($destImage, $srcImage, 0, 0, 0, 0, 100, 100, $srcSize[0], $srcSize[1]);
        /*Create new path with .jpg extension*/
        $newImagePath = "images/avatars/" . $varusername . "_avatar.jpg";
        /*Save resized image*/
        imagejpeg($destImage, $newImagePath, 85);
        /*Remove images from memory*/
        imagedestroy($srcImage);
        imagedestroy($destImage);
        /*Delete the original file from the server as long as it has a 
         different name than the new one (since if it has the same name, the
         new one will have already overwritten it anyway and we don't want
         to delete the new file.  This also prevents the new file from being 
         deleted in the unlikely event that someone uploads an avatar in 
         the exact "username_avatar.jpg" format.)*/
        if ($oldImagePath != $newImagePath) {
            unlink($oldImagePath);
        }
    }
    /*...and the user is inserted into the users table.*/
    mysql_query("insert into users (user_name, first_name, last_name, email,
        gender, birthdate, location_city, location_state, location_country, password, avatar_full, avatar_thumb)
        values ('$varusername', '$varfirstname', '$varlastname', '$varemail',
            '$vargender', '$birthdate', '$varlocationcity', '$varlocationstate', 
            '$varlocationcountry', '$hashpassword', '$avatarFullPath', '$avatarThumbPath');");
        /*Redirect to home page*/
        print "<script type='text/javascript'>window.location = 'login.php'</script>";
    }
}   
?>

I would really really recommend you go through and clean the code up - refactor refactor refactor, use best practices and as @peterhil said - read through the links and keep the HTML presentation away from PHP and JS and CSS.

\$\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.