Need help optimizing & improving this php form script I usually program in python/javascript and am very new to php. I would appreciate some advice on how to improve the following form script.

Since this is for a highly trafficked website, I want it to be extremely secure, reliable and optimized.

Feel free to point out minor issues as well.

&lt;?php

// avoid errors when in production
// ini_set("display_errors", "0");
error_reporting(-1);

# get value from array
function get_string($array, $index, $default = '') {
    if (isset($array[$index]) &amp;&amp; strlen($value = trim($array[$index])) &gt; 0) {
        return get_magic_quotes_gpc() ? stripslashes($value) : $value;
    } else {
        return $default;
    }
}

function test($field) {
    global $errors, $required_error;
    if ($field['value'] == '') {
        if ($field['required']) {
            $required_error = true;
        }
        return;
    }
    elseif ($field['validation'] == 'email' &amp;&amp; !filter_var($field['value'], FILTER_VALIDATE_EMAIL)) {
        array_push($errors, $field['value'] . ' is an invalid email address.');
    }
    elseif ($field['validation'] == 'number' &amp;&amp; !preg_match('/^[0-9 ]+$/', $field['value'])) {
        array_push($errors, $field['value'] . ' is an invalid number.');
    }
    elseif ($field['validation'] == 'alpha' &amp;&amp; !preg_match('/^[a-zA-Z ]+$/', $field['value'])) {
        array_push($errors, $field['value'] . ' contains invalid characters. This field only accepts letters and spaces.');
    }
}

$post = filter_input_array( INPUT_POST, FILTER_SANITIZE_SPECIAL_CHARS );
$errors = array();
$required_error = false;
$ajax = get_string($post, "request_method") == 'ajax';

# select data that needs validation
$fields = array(
    'fullname' =&gt; array(
        'value' =&gt; get_string($post, "full-name"),
        'validation' =&gt; 'alpha',
        'required' =&gt; true,
    ),
    'emailaddress' =&gt; array(
        'value' =&gt; get_string($post, "email-address"),
        'validation' =&gt; 'email',
    ),
    'activites' =&gt; array(
        'value' =&gt; get_string($post, "activites"),
        'validation' =&gt; 'default',
    ),
    'country' =&gt; array(
        'value' =&gt; get_string($post, "country"),
        'validation' =&gt; 'default',
    ),
    'contactpreference' =&gt; array(
        'value' =&gt; get_string($post, "contact-preference"),
        'validation' =&gt; 'default',
    ),
    'message' =&gt; array(
        'value' =&gt; get_string($post, "message"),
        'validation' =&gt; 'alpha',
    ),
);

// test each field
foreach ($fields as $field) {
    test($field);
}

if (count($errors) or $required_error) {
    if ($ajax){
        echo 'FAIL';
        return;
    }

    $template_dict = array(
        'errors' =&gt; ''
    );

    if ($required_error == true) {
        $template_dict['errors'] .= "&lt;li&gt;Please fill out all required fields&lt;/li&gt;";
    }

    foreach ($errors as $error) {
        $template_dict['errors'] .= "&lt;li&gt;$error&lt;/li&gt;";
    }

    // show errors
    echo $template_dict['errors'];
} else {
    $template_dict = $fields;

    // send mail
    $formcontent = "Full Name: $fullname \nEmail Address: $emailaddress \nActivites: $activites \nCountry: $country \nContact preference: $contactpreference \nMessage: $message \n";
    $formcontent = wordwrap($formcontent, 70, "\n", true);

    $recipient = 'email@address.com'; $subject = "Form Name"; $mailheader = "From: $emailaddress \r\n";

    mail($recipient, $subject, $formcontent, $mailheader);


    if ($ajax) {
        echo 'PASS';
        return;
    }

    echo 'Success';
}
?&gt;
I've tried to address as many things as I could think of, though I'm not sure how relevent a lot of them will be :).

Anyway, on a lot of things I just glossed over the subject for brevity, however, if you have any questions or want me to defend any statements, I gladly will if prompted.

error_reporting

I tend to leave error_reporting and display_errors out of PHP scripts as much as possible.  Those should ideally be server-wide settings more so than application specific.  A development server should always have error_reporting(E_ALL) and display_errors = On and a production server should always have error_reporting(E_ALL) and display_errors = Off.  If error reporting ever has to be lowered to accomdate variable doesn't exist or undefined index type notices, that's a sign of bad code.

Magic Quotes

If magic quotes is on, either turn it off, or if not possible, move servers.  Magic quotes is an ancient, horrible mistake PHP made, and no server admin should ever have it enabled.

get_string

You're doing too much in this function.  The name and the function signature gives the impression that it gets a string if it exists in an array, and otherwise returns the default.

What is actually happening though is much more than this.

Also, the logic in it is a bit misleading.  What if you call it with a default that is not the empty string, but then an empty string is in the array?  Because of the strlen check, the non empty default is going to be picked.  Also, I would not expect this function to be calling trim.

In my opinion, this function needs either better documentation, a better name, or both.  (Or pull the multiple behaviors into multiple functions.)

I would probably implement this function as follows:

function array_get(array $array, $key, $default = null)
{
    return (array_key_exists($key, $array)) ? $array[$key] : $default;
}


I might then build on top of this:

function array_get_string(array $array, $key, $default = '')
{
    $val = array_get($array, $key);
    return (is_string($val)) ? $val : $default;
}


Or, if you wanted to bake in the trim functionality:

function array_get_string(array $array, $key, $default = '', $trim = true)
{
    $val = array_get($array, $key);
    if (is_string($val)) {
        return ($trim) ? trim($val) : $val;
    }
    return $default;
}


Another example of how you could build on it:

function array_get_digits(array $array, $key, $default = 0)
{
    $val = array_get($array, $key);
    if (is_int($val) || ctype_digit($val)) {
        return (int) $val;
    } else {
        return $default;
    }
}


test

Globals are evil.  I'm too lazy to back it up in this post, but the key points:
* ties your code to them (you must always use variables named $errors and $required_errors.  What if you want to call test and have it populate a different set of variables?)
* your code no longer gets along with code you didn't write.  What if a different piece of code happens to use $errors and your code silently clobbers it?
* much harder to maintain -- it's difficult to follow the mutation of a variable when it's done in a far off place with no easily traceable execution flow

What you should do instead is to return an array containing the errors.

Also, test is a bad name.  What are you testing?  Perhaps validateField or testInputField or something along those lines.

When sanitizing should happen

$post = filter_input_array( INPUT_POST, FILTER_SANITIZE_SPECIAL_CHARS );


This is done too early.  Your validations should see text as it actually is, not as valid HTML.  SANITIZE is a bit of a misnomer here.  It should actually be something like FILTER_ESCAPE_HTML or something.  HTML escaping only makes sense in the context of HTML.  Your validations are seeing text, not HTML.

This becomes a lot more important if you're storing information in a database.  Once you store text transformed, it becomes harder to search for it, and it becomes harder to predict the length of it.

The length could actually apply to your situation as well.  a &amp; b is 5 characters, however, a &amp;#38; b is 9.  What if you had a field that had a max of 5 characters?  Suddenly the user is very confused as to why his input is failing.  (Also, a &amp;#38; b is meaningless outside of HTML.)  Since your email is plaintext, you would actually see &amp;#38; in your email.

In short, escape at the last possible moment.

Random design input

I might approach your forms a bit more abstractly.  For example, I might take your array idea ($fields) and consider that a "form".  Then a validateForm function could take all of the elements in the form and check them for errors.  This function would return an array of any errors (or perhaps: array('errors' =&gt; [errors], 'valid' =&gt; [valid field's values]).  This would still use most of the same code you have now, just structed a bit differently.

Also, I would consider making 'validation' be an array.  What if at some point in the future you want to apply multiple validations to a field?  ("If it's wrong in way X, show one message; if it's wrong in way Y, show a different message").

Another design point: I would make 'required' be handled like any other error.  There's not really a reason to only validate until you come across the first empty required field, and this requires complicating the error handling code.

Type strictness

PHP's implicit type system is a bit odd at times.  For example, 0 == '' is true.  That means that in situations like $field['value'] == '' you should likely use $field['value'] === ''.

Or perhaps:

$field['value'] === '' || $field['value'] === null


I highly advise learning the type system and related materials (isset/empty/array_key_exists/etc).  PHP has lots of little quirks that can bite you (and have bitten me many times before I became paranoid :p).

Optimizing

The script looks fine in terms of performance.  The main time-taker in it will be the mail() call, and assuming your mail server is quick to respond, that shouldn't be significant.

mail

I like to use packages like Zend_Mail or PHPMailer.  For a simple email with a simple server setup, though, mail should be fine.  (For example, mail and SSL would be an unpleasant combination.)

I might worry about whether or not mail succedes though.  Might be nice to warn users that mailing failed.

Style

This section is 100% opinion



array_push is pretty rare to see in modern code.  There's borderline cases where it's actually better, but for visual reasons, most people prefer the $array[] syntax.

The following are equivalent:

$arr[] = 5;
array_push($arr, 5);




if (count($errors) or $required_error)


Would usually be written as:

if (count($errors) || $required_error)


(Though or and || are not interchangeable, they are in this situation.  In situations that do not explicitly call for or, || is typically used.)



if ($required_error == true)


Is a bit redundant.  It should just be:

if ($required_error)


The only time to use == for true is when you're strictly checking:

if ($required_error === true) //true iff $required_error is boolean true.  For example, $required_error = 1 would be false because 1 !== true.




I wouldn't mix # and // comments.  Just a being-picky thing, but it looks a bit odd to have both in one file (or really both in one codebase).  // is pretty much the standard in PHP, though both are just as well supported.



I would consider using heredoc style comments.  They can be parsed by IDEs like Netbeans and can be used to generate HTML documentation.  For example:

/**
 * Takes an array, an index for the array and an optional $default and returns $array[$index]
 * if it exists, or otherwise $default.
 * 
 * @param array $array The array from which to select the data
 * @param string|int $index The index of the array
 * @param mixed $default The value to return if $index is not in $array
 * @return string $array[$index] if it exists, otherwise, $default
*/
function get_string(array $array, $index, $default = '')


Depending on how clear names are, documentation is often not needed for small code bases, but it's very helpful when you come back a year later and have to figure out what's going on in a function.

Note also that I made it so that $array must be an array.  This is only possible in PHP for certain types (array and classes).