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).

• jsanc623: In validate(), shouldn't $reg_errors be &$reg_errors since we're adding to it without returning?
• Elias Van Ootegem: @jsanc623: No, because it's an object. And objects are passed by reference by default since PHP 5.
• jsanc623: Ah! Yep, correct. My apologies!

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!