HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Simple PHP Validation Routine

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phproutinesimplevalidation

Problem

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.

Solution

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' ));
}

Code Snippets

extract($_POST);

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

Context

StackExchange Code Review Q#68279, answer score: 8

Revisions (0)

No revisions yet.