Verify $_POST script will work correctly

Question

can someone read through this script really quick and verify that I didn't miss anything... I'm not getting any errors in my IDE so just have to make sure the structure is correct

<?php
require_once '/usr/local/cpanel/3rdparty/lib/php/Mail.php';

$db_server = 'localhost';
$db_user = '-----';
$db_pass = '-----';

$dbc = mysql_connect ($db_server, $db_user, $db_pass);
if (!$dbc) { 
    die(mysql_error());
    header ('Location: /contact');
    exit;
}

if ($_POST['contactsent'] != 'yes') {
    header ('Location: /contact');
    exit;

} else {

    if (is_array($_POST)) {
        foreach ($_POST as $key => $value) {
            $_POST[$key] = mysql_real_escape_string(stripslashes($value));
        }
    }

    $RequestType      = $_POST["RequestType"];
    $ConsumerBusiness = $_POST["ConsumerBusiness"];
    $GlobalLocation   = $_POST["GlobalLocation"];
    $FirstName        = strtolower(str_replace("'","''",$_POST["FirstName"]));
    $FirstName        = strtoupper(substr($FirstName,0,1)).substr($FirstName,1);
    $LastName         = strtolower(str_replace("'","''",$_POST["LastName"]));
    $LastName         = strtoupper(substr($LastName,0,1)).substr($LastName,1);
    $Email            = strtolower(str_replace("'","''",$_POST["Email"]));
    $Title            = strtolower(str_replace("'","''",$_POST["Title"]));
    $Title            = strtoupper(substr($Title,0,1)).substr($Title,1);
    $Company          = strtolower(str_replace("'","''",$_POST["Company"]));
    $Company          = strtoupper(substr($Company,0,1)).substr($Company,1);
    $Address          = strtolower(str_replace("'","''",$_POST["Address"]));
    $Address          = strtoupper(substr($Address,0,1)).substr($Address,1);
    $City             = strtolower(str_replace("'","''",$_POST["City"]));
    $City             = strtoupper(substr($City,0,1)).substr($City,1);
    $State            = $_POST["State"];
    $Zip              = $_POST["Zip"];
    $Phone            = $_POST["Phone"];
    $F              = $_POST["F"];
    $ProductDesc      = $_POST["ProductDesc"];
    $Comment          = $_POST["Comment"];

    if ($GlobalLocation == "Canada"):
        $SendTo="canadainfo@------.com";
    elseif ($GlobalLocation == "Central America"): 
        $SendTo="customer.service@------.com.pa";
    elseif ($GlobalLocation == "Europe"):
        $SendTo="marketing@-----.uk";
    elseif ($GlobalLocation == "Mexico"):
        $SendTo="ventas@------.com.mx";
    else:
        $SendTo="info@------.com";
    endif;

function dbSet($fields, $source = array()) {
    $set='';
    if (!source) $source = &$_POST;
    foreach ($fields as $field) {
        if (isset($source[$field])) {
            $set.="`$field`='".mysql_real_escape_string($source[$field])."', ";
        }
    }
    return substr($set, 0, -2);
}

// INSERT INTO DATABASE
            mysql_select_db("new_contact",$dbc) or die("Could not select new_contact");

$fields =   explode(" ", "RequestType ConsumerBusiness GlobalLocation FirstName LastName Email Title Company Address City State Zip Phone F ProductDesc Comment");
            $query = "INSERT INTO new_contact SET ".dbSet($fields, $_POST);
            mysql_query($query);

// SETUP EMAIL
        $Bodycopy = "This information was submitted via the ------.com website and sent to you because of the location 
        identified by the user. <br>If this has reached you in error, please forward this email to info@------.com";
        $Bodycopy. "<br>----------------------------------------------------------------------------------------------<br><br>";

        if ($RequestType != "")      $Bodycopy. "What kind of information do you need? : " .$RequestType. "<br>";   
        if ($ConsumerBusiness != "") $Bodycopy. "What type of customer or vendor are you? : " .$ConsumerBusiness. "<br>";
        if ($GlobalLocation != "")   $Bodycopy. "Global Location : " .$GlobalLocation. "<br>";
        if ($Company != "")          $Bodycopy. "Company : " .$Company. "<br>";
        if ($FirstName != "")        $Bodycopy. "First Name : " .$FirstName. "<br>";
        if ($LastName != "")         $Bodycopy. "Last Name : " .$LastName. "<br>";
        if ($Title != "")            $Bodycopy. "Title : " .$Title. "<br>";
        if ($Email != "")            $Bodycopy. "Email : " .$Email. "<br>";
        if ($Address != "")          $Bodycopy. "Address : " .$Address. "<br>";
        if ($City != "")             $Bodycopy. "City : " .$City. "<br>";   
        if ($State != "")            $Bodycopy. "State : " .$State. "<br>";
        if ($Zip != "")              $Bodycopy. "Zip/Postal Code : " .$Zip. "<br>";
        if ($Phone != "")            $Bodycopy. "Phone : " .$Phone. "<br>";
        if ($F != "")              $Bodycopy. "F : " .$F. "<br>";
        if ($ProductDesc != "")      $Bodycopy. "UPC or product description : " .$ProductDesc. "<br>";

        $Bodycopy. "<br>----------------------------------------------------------------------------------------------<br><br>";

        if ($Comment != "")          $Bodycopy. "Comments : <br>" .$Comment. "<br>";

        $Bodycopy. "<br><br>";
        $Bodycopy. $IP = $_SERVER["remote_addr"];

// PROCESS EMAIL

// mail server info...

        $from = $SendTo;
        $to = "Do Not Reply <donotreply@------>";
        $subject = "Website Contact : " . $GlobalLocation;
        $body = $Bodycopy;
        $host = "mail.------";
        $port = "25";
        $username = "donotreply@-------";
        $password = "-------";

        $headers = array ('From' => $from,
        'To' => $to,
        'Subject' => $subject);
        $smtp = Mail::factory('smtp',
        array ('host' => $host,
         'auth' => true,
         'port' => $port,
         'username' => $username,
         'password' => $password));

        $mail = $smtp->send($to, $headers, $body);

        if (PEAR::isError($mail)) {
        echo("<p>" . $mail->getMessage() . "</p>");
        } else {
        echo("<p>Message successfully sent!</p>");
        }

// MAKE SURE DB CONN IS CLOSED
        mysql_close($dbc);

// REDIRECT TO THANK YOU PAGE
        header ('Location: /index.php?option');
        exit();

}

?>

Answer 1

I don't know about real quick. What you have doesn't really compliment a quick review. First off this is procedural code, which is fine if you are just getting started, don't let anyone tell you otherwise. But procedural code is not "quickly" read. I will suggest that once you have a firm grasp on the language, you should definitely take a look at OOP, but not before. You may notice me throwing this OOP (Object Oriented Programming) term around a lot. I tell a lot of people that I am against pushing OOP. No, not against OOP, just against pushing it. I won't tell you to stop what you are doing and learn OOP and no one else should either. Far from it. You need a solid understanding of the language first. Whenever I mention OOP I am merely pointing out that doing those suggestions now will prepare you for it and will make more sense as you progress into OOP. So take my OOP talk with a grain of salt, follow the suggestions, but don't go out and learn it just because I mentioned it. Now on to the review. I will read your code and write my thoughts as I have them, so you will be able to follow along in your own code. There is only one spot where I jump, but after that it should all be in order.

Review

The first problem I have with your code is that it is very redundant and inefficient. When you tell a program to die() or exit or go to a different header(), you are telling it to stop immediately, and, if applicable, process the argument between the parenthesis. Yet you have code after that and it will not process because you've already told it to stop. Rewrite your mysql_connect() like so:

$dbc = mysql_connect ($db_server, $db_user, $db_pass) or die(mysql_error());
// OR, if you want the header, remove the "or die" from above and use the following
if ( ! $dbc) { header ('Location: /contact'); }

You'll notice I did not use the mysql_error() in that second instance. That is because if you load the header it will send you to a different page and you will never see it. If you want to see it on the new page, I would suggest passing it as a GET parameter. Also, careful where you throw out those header()'s. Headers can not be called after information has already been sent to the browser. If possible, I would suggest setting up a function to hold all of the statements that set a new header so that it is all in one place.

Checking and Sanitizing POST

As mikeythemissile said, you should be checking your globals. And not just that they are set. You should be sanitizing them as well. If you have PHP version >= 5.2, check out filter_input(). NEVER trust user input. Always sanitize, scrub, and blast the hell out of it in any way that you can. It is better to be overly cautious than not at all, especially when dealing with SQL. Google SQL injection, its a real thing and can cause all sorts of problems for you.

Why are you checking if POST is an array? Of course it is, it should never not be an array. What you really want to check is if it is being used, which, if it isn't, would have thrown errors before this point. You should always check unreliable variables or arrays (GET, POST, etc...) before using them. So I would start by checking my POST like so.

if($_SERVER['REQUEST_METHOD'] == "POST") {//could also use isset($_POST)
    //here's an example of that filter_input method
    if(filter_input(INPUT_POST, 'contactsent', FILTER_VALIDATE_BOOLEAN) { header(Location: /contact'); }
    else {
        $new_array = array();//don't use the POST array after you have already gotten what you need from it, put it in a new array
        foreach($_POST as $key => $value) {
            $new_array[$key] = mysql_real_escape_string(stripslashes($value));
        }
    }
}

I would not crunch through POST blindly like that either. You've provided an array later in your script, $fields, that should serve as a list of items to check. This is where I jump, by the way. Loop over the $fields array to get the keys to search for in the POST array. First, I would change the way you get your $fields array. explode() is an ingenious way to get an array if the data you are trying to convert is dynamic and comes in that form already. However, you have manually typed these fields, so why bother with the extra processing power required to explode it? Just make it an array.

$fields = array(
    'RequestType',
    'ConsumerBusiness',
    'GlobalLocation',
    'FirstName',
    'LastName',
    'Email',
    'Title',
    'Company',
    'Address',
    'City',
    'State',
    'Zip',
    'Phone',
    'F', //What is this, make you variables clear and understandable all the rest are
    'ProductDesc',
    'Comment'
);

Now that you have this array, you can go back and do so many cool things with it. Such as getting only what you need from the POST array and ignoring everything else.

$new_array = array();
foreach($fields as $field) {
    $new_array[$field] = filter_input(INPUT_POST, $field, FILTER_SANITIZE_STRING | FILTER_SANITIZE_STRIPPED);//another example of filter_input
}

You can also use that same array to clean up your dbSet() function.

$set = '';
foreach($new_array as $field => $value) {
    $set .= "`$field`='" . mysql_real_escape_string($value) . "', ";
}

Now, I'm not saying this code is 100%, this is just to give you ideas of how to use your code intelligently. Make your code work for you, don't work for your code. Expand upon this and you will see immediate improvement in your coding.

Repetitive Code

I really dislike this wall of variables. I would see about getting rid of it if you can. If possible try to make these changes while iterating over them with the $fields array or something. There is no reason for there to be this many copies of the same information. Also, why are you not using ucfirst()? That is obviously what you are trying to do with that strtoupper substring mess. Use the functions provided by PHP. In almost all cases they have a function to do what you want, so search for it before you reinvent it. The documentation is, pardon the repitition, well documented and easily navigable. I also would not convert a string to lowercase if not necessary, such as the email address.

$FirstName        = strtolower(str_replace("'","''",$_POST["FirstName"]));//Not sure what you are doing here so I left it.
$FirstName        = ucfirst($FirstName);

Don't use repetitive if/else statements, use switch statements instead. They are proven faster and are cleaner, not to mention easily extendable.

switch($GlobalLocation) {
    case 'Canada':
        $SendTo="canadainfo@------.com";
    break;
    case 'Central America': 
        $SendTo="customer.service@------.com.pa";
    break;
    etc...
}

Functions

This function,dbSet(), just came out of no where. Because you are doing procedural code, you should make life easier on yourself and others who might eventually take over your code. Make it as easy to read as possible. The first step to this goal is placing your code logically. If you have custom functions, you should group them all together, preferably at the beginning of your program before you start your procedural code. Since you already know about require/include, I would suggest moving them to another file and requiring/including it at the very beginning. That's one of the first steps towards learning OOP, learning to separate your code, albeit in a not so OOP way, but still important...

Also, the dbSet() function is redundant. You already did mysql_real_escape_string() and stripslashes to the POST array in the foreach loop earlier. You will find that you repeat yourself a lot while writing procedural code. The best cure for this is to start breaking your procedural code up into functions. Functions are the first step to learning OOP.

Concatenating Variables

You are not concatenating your variables properly. To append data to the end of a variable use .=. If you just use the concatenate operator . it will not change the variable, it will only add it to the local memory for that instance. In other words, if you were to echo it directly echo $Bodycopy . 'extrastuff'; you would get the desired results, but since you are doing multiple instances, you will need to append it first $Bodycopy .= 'extrastuff';. So change the way you concatenate all of your $Bodycopy variables to .=. Also, check out heredoc, or nowdoc for long strings such as these.

If Statements

Even if it is only one line. Please, for the love of god, use braces {}. I know it is not necessary. I know it knocks off 2 bits of file size per statement. I know a lot of people do it. But please, DON'T. It makes reading your code so much harder without those braces and doesn't harm anything to add them. I hate that PHP allows this and really wish they would remove it in future releases. It teaches bad coding habits.

More Repetition

The first two chunks under //mail server info is redundant. Remove all of those variables and just set them in the $headers array. Then, if you don't want to use the array reference ($headers['To']) you can use extract() to convert all of the array references to variables.

Exit

See comment, you can ignore this section...

Lastly, stop using exit. Every single place you have used it, it is unnecessary. If you find yourself needing to stop your code like this, especially this frequently, you are doing something wrong. Mostly this extends from long procedural script and can be cured by OOP, but, in the mean time, if you just clean up your code and write it a bit more logically, you will notice that they are not necessary.

Answer 2

Although it'll lenghten your code quite a bit, in my opinion it's worth checking if the $_POST variables are set (using isset), to avoid any exceptions.