How can I improve this code?

[spudz]

Hi, I'm trying to automate basic form processing as much as possible, is there any way to improve this code, make it faster, easier to work with, anything.

<?php

$required = array('name', 'email', 'message');
$errors = array();
$data = array();

if (is_array($_POST))
{
    foreach ($_POST as $key => $value)
    {
        if (in_array($key, $required))
        {
            if (empty($value)) $errors[$key] = 'empty';
            else $data[$key] = clean($value);
        }
        else
        {
            $data[$key] = clean($value);
        }
    }
}


function clean($str)
{
    return mysql_real_escape_string($str);
}

?>

[decowski]

You shouldn't entity encode the data before inserting into the database. Rather, you should entity encode it immeditely before displaying.

[spudz]

Yeah, good call Paul, i was trying to add a bit of extra assurance that quotes wouldn't screw with my db

[Cups]

Couple of ideas:

1
run array_map() over the post vars with trim() first.

2
$errors[$key] = 'empty'

Could you do this instead?

$errors['empty'] = $key ;

In case you later have $errors['invalid'] ?

The final formatting of errors may differ from invalid attempts.

3
I haven't tested the robustness of this check:

if (is_array($_POST))

Could that be ( count( $_POST ) >= 1 ) - I prefer testing for positives rather than relying on testing whether an empty array passes that test or not.

And what if you run my suggestion 1 before running suggesion 3?

$_POST['email'] = " " ;

Use var_dump a LOT.

this seems wasteful:

            else $data[$key] = clean($value);
        }
        else
        {
            $data[$key] = clean($value);
        } 

If you are running clean() on everything, just do it in one place.

Huh,

if count( $required ) is equal to 3, then if your count of POST vars is less than 3 then send them far, far away. (header to your home page)

[spudz]

Edited based on Cups advice.

<?php

$required     = array('name', 'email', 'message');
$errors        = array();
$data         = array();

if (count($_POST) >= 1)
{

    array_map('trim', $_POST);
    foreach ($_POST as $key => $value)
    {
        if (in_array($key, $required))
            if (empty($value)) $errors[$key] = 'required';
                    
        $data[$key] = clean($value);
    }
    
    if (!preg_match('/^([a-z0-9])(([-a-z0-9._])*([a-z0-9]))*\@([a-z0-9])(([a-z0-9-])*([a-z0-9]))+(\.([a-z0-9])([-a-z0-9_-])?([a-z0-9])+)+$/i', $data['email']))
    {
        $errors['email'] = 'invalid';
    }
}

function clean($str)
{
    return mysql_real_escape_string($str);
}

?>

2
$errors[$key] = 'empty'
Could you do this instead?
$errors['empty'] = $key ;
In case you later have $errors['invalid'] ?
The final formatting of errors may differ from invalid attempts.

Writing the errors in the keys would overwrite on each iteration of the loop, so it would probably be best to use the $_POST keys in the $errors array aswell

Huh,
if count( $required ) is equal to 3, then if your count of POST vars is less than 3 then send them far, far away. (header to your home page)

This seems too specialized at the moment, i'm trying to create a generic form processing template to work from. Also why would you check if $required is equal to 3, there could be a textbox for website which isn't required and if someone enters that I'd be sending someone away for no reason.

Thanks for your advice, very helpful

[crmalibu]

Your current code can't detect whether or not the required fields are all present. While it will work if the user just doesn't fill in a value, what if they don't even send the variable at all(they edit the form source)? It won't exist in _POST at all, and your loop won't check them.

[spudz]

Your current code can't detect whether or not the required fields are all present. While it will work if the user just doesn't fill in a value, what if they don't even send the variable at all(they edit the form source)? It won't exist in _POST at all, and your loop won't check them.

so what would you suggest?

[crmalibu]

This just makes sure the desired array keys are present in post

if ($missing_fields = array_diff($required, array_keys($_POST))) {
    print_r($missing_fields);
} 

Then you can later go and check the values and stuff.

[Cups]

Writing the errors in the keys would overwrite on each iteration of the loop, so it would probably be best to use the $_POST keys in the $errors array aswell

Would have been slightly more revealing if I had described it correctly ...

$arr['empty'][] = 'email';
$arr['empty'][] = 'password';

$arr['incorrect'][] = 'email' ;

var_dump( $arr['empty'] );

This seems too specialized at the moment, i'm trying to create a generic form processing template to work from. Also why would you check if $required is equal to 3, there could be a textbox for website which isn't required and if someone enters that I'd be sending someone away for no reason.

I suppose you are right. I honestly do a lot of pre-validation checks in JS (which dont count as real security, I know ...).

If what arrives has clearly circumvented those checks somehow, I tend to be pretty brutal.

I'd do the trim() first too.

crmalibu's check seems fast and to the point. Fail early on with that if you can why bother validating 2 fields if you are only going to go ahead and execute changes with 3?

[antirealm]

Your current code can't detect whether or not the required fields are all present.

Good point. Iterate over the required fields array and check isset($_POST[$requiredVar]).

I would wrap this functionality inside of a class so It can easily be reused, and with a fair bit more messing around you'd be able to extend it easily, conforming to the open-closed principle. Are all forms in your app going to follow this exact structure?

Also, I wouldn't use explicit constants for errors, use a defined type such as $errors[$key] = FORM_REQUIRED.

Oh, and I wouldn't iterate through $_POST, because you might find yourself checking form vars that arn't even used, say if the client edited the form.

If you want to validate a form var according to a application-wide rule (a rule that is enforced in other parts of the application) you will want to define that rule in one central place, so you don't have to copy paste your regex in every form that deals with an email addr. E.g. preg_match( EMAIL_REGEX )