2
\$\begingroup\$

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.

<?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]) && strlen($value = trim($array[$index])) > 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' && !filter_var($field['value'], FILTER_VALIDATE_EMAIL)) {
        array_push($errors, $field['value'] . ' is an invalid email address.');
    }
    elseif ($field['validation'] == 'number' && !preg_match('/^[0-9 ]+$/', $field['value'])) {
        array_push($errors, $field['value'] . ' is an invalid number.');
    }
    elseif ($field['validation'] == 'alpha' && !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' => array(
        'value' => get_string($post, "full-name"),
        'validation' => 'alpha',
        'required' => true,
    ),
    'emailaddress' => array(
        'value' => get_string($post, "email-address"),
        'validation' => 'email',
    ),
    'activites' => array(
        'value' => get_string($post, "activites"),
        'validation' => 'default',
    ),
    'country' => array(
        'value' => get_string($post, "country"),
        'validation' => 'default',
    ),
    'contactpreference' => array(
        'value' => get_string($post, "contact-preference"),
        'validation' => 'default',
    ),
    'message' => array(
        'value' => get_string($post, "message"),
        'validation' => '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' => ''
    );

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

    foreach ($errors as $error) {
        $template_dict['errors'] .= "<li>$error</li>";
    }

    // 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';
}
?>
\$\endgroup\$
3
\$\begingroup\$

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 & b is 5 characters, however, a &#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 &#38; b is meaningless outside of HTML.) Since your email is plaintext, you would actually see &#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' => [errors], 'valid' => [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).

\$\endgroup\$
1
  • \$\begingroup\$ That really was really enlightening. Thanks @Corbin for the thorough answer. Will spend the next few days implementing your suggestions! :) \$\endgroup\$
    – kht
    Jun 14 '12 at 20:04
2
\$\begingroup\$

Corbin did a really good job +1 for him. I just wanted to add a couple of things.

Corbin mentioned "or" and "||" not being equivalent. He is correct, they accomplish the same thing, but "or" has a lower precedence than "||". So "||" is processed before "or" when computed in the same expression, think of PEMDAS in math. However, he might have been thinking about its other implementation as a pseudo "else" statement usually seen as or die(). This short-circuiting works but is frowned upon, and either way I would just suggest staying away from "or" all together. More than likely you will never need it.

My Suggestions

You should avoid declaring variables in statements. It is allowed, but could cause issues. For example, if not done on purpose PHP and your IDE will have no way of warning you because it is considered valid code. So the expression if($isLoggedIn = TRUE) is TRUE, and I'm sure you can see where this leads. So if you just don't do it then when you find it in your code you know it isn't supposed to be there. Another plus for not doing it is that it makes your code easier to read. Longer, but easier to read. And easier to read, means easier to debug :)

if (isset($array[$index]) && strlen($value = trim($array[$index])) > 0) { }
//becomes
if(isset($array[$index])) {
    $value = trim($array[$index]);
    if(strlen($value)) { }//You can also drop that greater than comparison ( 0 == FALSE )
}

When checking if a value is empty, you can use PHP's empty() function.

if ($field['value'] == '') { }
//OR
if($field['value'] === '') { }
//becomes
if(empty($field['value'])) { }

When checking the same value for different states, it is much better to use a switch than multiple if/else statements. They are quicker and easier to read, not to mention that it saves repitition in your code.

switch($field['validation']) {
    case 'email':
        if(!filter_var($field['value'], FILTER_VALIDATE_EMAIL)) {
            array_push($errors, $field['value'] . ' is an invalid email address.');
        }
    break;
    case 'number':
        if(!preg_match('/^[0-9 ]+$/', $field['value'])) {
            array_push($errors, $field['value'] . ' is an invalid number.');
        }
    break;
    case 'alpha':
        if(!preg_match('/^[a-zA-Z ]+$/', $field['value'])) {
            array_push($errors, $field['value'] . ' contains invalid characters. This field only accepts letters and spaces.');
        }
    break;
}
\$\endgroup\$
9
  • 1
    \$\begingroup\$ I actually was trying to hint that or should be avoided entirely, but going back and reading what I wrote, I can see how the shortcircuiting-abuse interpretation is there. Also, you should mention that empty has a lot of quirks. Like "0" being considered empty. I tend to avoid empty unless I'm fine with the quirks of it. (Can confuse a user quite a bit to see an error about an empty string when they provided 0.) Also, I seem to be in the minority on this one, but I avoid switch statements. I don't consider them very readable at all and much prefer if-else trees (just opinion though). \$\endgroup\$
    – Corbin
    Jun 14 '12 at 19:05
  • \$\begingroup\$ I am quite used to using "or" in python and I guess it just seemed familiar. Never knew it had issues. Now I do :) \$\endgroup\$
    – kht
    Jun 14 '12 at 20:08
  • \$\begingroup\$ @Corbin: You are quite right about empty. Assumed variable would be quirk free. Bad of me I know. Switch statements, for me, have more to do with not wanting to repeat code whenever possible rather than legibility. However, I do also find them easier to read, thus my preference for them :) \$\endgroup\$
    – mseancole
    Jun 14 '12 at 20:12
  • \$\begingroup\$ Not sure if I understand how switch statements lead to less repeated code than if-else branches? They're basically the same construct represented with different syntax. Unless you're referring to the ability for switch statements to fall through? \$\endgroup\$
    – Corbin
    Jun 14 '12 at 20:20
  • \$\begingroup\$ @Corbin: No not the fall through, don't like that feature honestly. No, its not much reduction but you only have to type the variable you are comparing once, "$field['validation']" in this case, which with me and my fat fingers and lysdexia that means less chance of typos. \$\endgroup\$
    – mseancole
    Jun 14 '12 at 21:21

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.