patternphpMinor
Simple PHP Validation Routine
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....
It all works correctly but would like to try and simplify it if possible.
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
What you're doing is extracting
Suppose someone was to edit your form, or the POST request, and add a field of "woocommerce",
This would overwrite whatever your existing
You can avoid overwritting existing variables by using
See the PHP documentation for more information.
I advise you drop the
In your case, it's much safer to rely on
Should become,
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.