8
\$\begingroup\$

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.

\$\endgroup\$
8
\$\begingroup\$

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' ));
} 
\$\endgroup\$
4
\$\begingroup\$

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).
\$\endgroup\$
  • \$\begingroup\$ In validate(), shouldn't $reg_errors be &$reg_errors since we're adding to it without returning? \$\endgroup\$ – jsanc623 Oct 29 '14 at 16:27
  • 3
    \$\begingroup\$ @jsanc623: No, because it's an object. And objects are passed by reference by default since PHP 5. \$\endgroup\$ – Elias Van Ootegem Oct 29 '14 at 16:41
  • 1
    \$\begingroup\$ Ah! Yep, correct. My apologies! \$\endgroup\$ – jsanc623 Oct 29 '14 at 17:24
3
\$\begingroup\$

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;
}
\$\endgroup\$
2
\$\begingroup\$

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!

\$\endgroup\$

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.