Simple PHP Validation Routine

Question

I have created a simple validation routine in php and i'm wanting to make sure I am doing things efficiently....

function reg_err_validation($reg_errors) {
        global $woocommerce;
        extract($_POST);

        if($firstname == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
        }   
        if($lastname == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Last Name.', 'woocommerce' ));
        }
        if($phone == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Phone.', 'woocommerce' ));
        }
        if($address1 == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter address.', 'woocommerce' ));
        }
        if($city == '' )        
        {
            $reg_errors->add( 'registration-error', __( 'Please enter City.', 'woocommerce' ));
        }
        if($postcode == '' )        
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Postcode.', 'woocommerce' ));
        }
        return $reg_errors;
}

It all works correctly but would like to try and simplify it if possible.

Answer 1

There is one huge security oversight here, and that's the use of extract().

What you're doing is extracting $_POST data straight into $variables.

Suppose someone was to edit your form, or the POST request, and add a field of "woocommerce", $_POST['woocommerce'] would then become $woocommerce.

This would overwrite whatever your existing $woocommerce variable is storing, and as it'a global I'm guessing it's pretty important.

You can avoid overwritting existing variables by using EXTR_OVERWRITE and EXTR_SKIP. See the PHP documentation for more information.

I advise you drop the extract() function, especially for $_POST/$_GET/$_FILES, $_REQUESTS. In your case, it's much safer to rely on isset() or empty(). This will also prevent any E_NOTICES occurring when referencing a variable which may not exist, when using extract().

extract($_POST);

if($firstname == '' )
{
    $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
}   

Should become,

if(empty($_POST['firstname']))
{
    $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
} 

Answer 2

You could extract the validation into its own function. It's shorter, and in case you change how you handle errors, it's easier to change:

function reg_err_validation($reg_errors) {
        global $woocommerce;
        extract($_POST);

        validate($firstname, 'First Name', $reg_errors);
        validate($lastname, 'Last Name', $reg_errors);
        validate($phone, 'Phone', $reg_errors);
        validate($address1, 'address', $reg_errors);
        validate($city, 'City', $reg_errors);
        validate($postcode, 'Postcode', $reg_errors);
        return $reg_errors;
}

function validate($input, $name, $reg_errors) {
    if($input == '' )        
    {
        $reg_errors->add( 'registration-error', __( 'Please enter ' . $name . '.', 'woocommerce' ));
    }
}

Misc

• $woocommerce seems unused (and in general, using global might indicate a design flaw).
• reg_err_validation isn't such a good name. registration_validation might be a better fit (with validation, error is already implied, and reg is a bit unclear).

Answer 3

If you are just checking for "emptiness", you can also treat your fields themselves as data and iterate through them using foreach. This approach also handles the security flaw of extracting $_POST mentioned in Adrian's answer, since you are explicitly declaring the POST values you wish to check.

function reg_err_validation($reg_errors) {
    global $woocommerce;

    // Define each required field as an associative array, which has the field's name and a display name
    $required_fields = array(
        array(
            'field_name'   => 'firstname',
            'display_name' => 'First Name'
        ),
        array(
            'field_name'   => 'lastname',
            'display_name' => 'Last Name'
        ),
        array(
            'field_name'   => 'phone',
            'display_name' => 'Phone'
        ),
        array(
            'field_name'   => 'address1',
            'display_name' => 'Address'
        ),
        array(
            'field_name'   => 'city',
            'display_name' => 'City'
        ),
        array(
            'field_name'   => 'postcode',
            'display_name' => 'Postcode'
        ),
    );

    // Iterate throught each field and add an error if the field is empty.
    foreach ($required_fields as $field) {
        if( empty($_POST[$field['field_name']]) ) {
            // Compose the error message by using a string template
            $message = sprintf('Please enter %s.', $field['display_name']);
            $reg_errors->add( 'registration-error', __( $message, 'woocommerce' ));
        }
    }

    return $reg_errors;
}

Answer 4

The other solutions mentioned could pretty much do the trick. As for me, I personally have a rule where I say "if I see an if statement, attempt to convert it into a mapping array. Here's what resulted:

$KEY_TO_ERROR_MAPPING = array(
    'firstname' => 'Please enter First Name',
    'lastname' => 'Please enter Last Name',
    'phone' => 'Please enter Phone',
    'address1' => 'Please enter address',
    'city' => 'Please enter City',
    'postcode' => 'Please enter Postcode'
);

function reg_err_validation($reg_errors) {
    global $woocommerce; //can't tell if its in use

    foreach($KEY_TO_ERROR_MAPPING as $key => $error){
        $value = $_POST[$key];

        if($value == ''){                
            $reg_errors->add( 'registration-error', __($error, 'woocommerce' ));
        }
    }

    return $reg_errors;
}

Hopefully it looks a bit cleaner to you. Of course I have other rules I follow like:

• One line per condition
• One line per loop

But for now I hope this helps a bit!