...

View Full Version : Handling errors in my Validation class



Chris Hick
01-15-2012, 01:14 AM
Ok, so I've built this pretty simple validation class for forms.
Here is code:



class FormValidation {

// public properties
public $errors;

// private properties

private $isValid = true;

function __construct($fields) {
$this->getRules($fields);
if(!$this->isValid) {
return $this->errors;
}
}

private function isEmpty ($field) {
if (!isset($_POST["{$field}"]) || empty($_POST["{$field}"])) {
return $this->errors["{$field}"] ="* ". ucfirst($field) . " cannot be empty.";
}
}
private function getRules($fields) {
foreach ($fields as $field=>$rules) {
if(array_key_exists($field, $_POST)){
foreach ($rules as $rule) {
if(method_exists($this,$rule)) {
$this->$rule($field);
}else {
return $this->errors["method"] = "This rule does not exist";
}
}
} else {
return $this->errors["exist"] = "This field does not exist";
}
}
}


private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if ((strlen($username) < 6) || (strlen($username) > 32)) {
return $this->errors["username"] = "* Your username must be greater than 6 characters and less than 32 characters.";
} else if (!preg_match($username_regex, $username)) {
return $this->errors["username"] = "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .";
} else {
$users = User::find_by_username($username);
if (!empty($user)) {
return $this->errors["username"] = "* The username you selected has already been taken. Please select another.";
}
}
}

private function checkErrors() {
if(!empty($this->errors)) {
$this->isValid = false;
}
}


}


I call the class like this:


$fields = array(
'username' =>
array("isEmpty", "checkUsername"),
);

$validation = new FormValidation($fields);


It appears to work, when I submit the field in without anything in it. Yet, it doesn't output the first error of checkUsername. It just says "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - ." It doesn't even out put that the field cannot be empty. Any ideas how to handle my errors in the class better??

12k
01-15-2012, 04:05 AM
In order to display more then one error at once, you will have to store the errors into an array, and use several if statements rather then if/else if/else. After the error array is set, you can then check to see if the error array is empty to know whether to proceed or not.

Chris Hick
01-15-2012, 05:09 AM
I think I get what you are going at. Mind showing me an example? I tried it on my side and I have something working but I wanna see what you come up with as opposed to what I wrote.

12k
01-15-2012, 05:18 AM
How its currently working, its set to

Do This:
Or Do This:
Else do this:

So only one is being called.

You would have to do something like this:




private function AddError($errorType, $errorMessage)
{
$pos = count($this->errors[$errorType]);
$this->errors[$errorType] = $errorMessage;
}


private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if ((strlen($username) < 6) || (strlen($username) > 32)) {
$this->AddError("username", "* Your username must be greater than 6 characters and less than 32 characters.");
}
if (!preg_match($username_regex, $username)) {
$this->AddError("username", "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .");
}
$users = User::find_by_username($username);
if (!empty($user)) {
$this->AddError("username", "* The username you selected has already been taken. Please select another.");
}
}



Then check to see if count($this->errors['username']) == 0 before you continue.

Chris Hick
01-15-2012, 05:48 AM
Yeah, I thought you meant something like that. I tried to incorporate what you had into mine but it didn't work. Hmm, I'll have to think about this even more than I thought.

12k
01-15-2012, 06:58 AM
If you want feel free to add me to msn @ twelve_k@live.com and I can help you out with it.

Chris Hick
01-15-2012, 04:39 PM
Ok, so I'm going to try to explain what I want it to be doing. I want to be able to list the fields of a form inside and array and inside that array I want to be able to list the rules for each field.


$fields = array(
'username' =>
array("isEmpty", "checkUsername"),
);


When I call my class, the method getRules is called and what this does is takes the rules array inside the fields array and check to see if that content is a method of the class, if it is it will then call the rule and place the field inside of it to run.

I want it to be able to run the first method called and if no errors occur then to run the second method. In this case, I want it to be able to run the isEmpty method, then if no errors occurs, it runs the checkUsername method. Inside the username method, I want it to first check the length. If it is less or more than the fixed lengths then it needs to place an error in the error array. If there isn't an error for that first part of the username function, I want it to then check the characters to see if they are in compliance and do the exact same as the first part of the function if there is an error. Finally, if there is no error for that second part of the username function, it then checks to see if the username is taken of not then no errors occured. If it is taken, then an error needs to be placed.

BluePanther
01-15-2012, 04:51 PM
Ok, so I'm going to try to explain what I want it to be doing. I want to be able to list the fields of a form inside and array and inside that array I want to be able to list the rules for each field.


$fields = array(
'username' =>
array("isEmpty", "checkUsername"),
);


When I call my class, the method getRules is called and what this does is takes the rules array inside the fields array and check to see if that content is a method of the class, if it is it will then call the rule and place the field inside of it to run.

I want it to be able to run the first method called and if no errors occur then to run the second method. In this case, I want it to be able to run the isEmpty method, then if no errors occurs, it runs the checkUsername method. Inside the username method, I want it to first check the length. If it is less or more than the fixed lengths then it needs to place an error in the error array. If there isn't an error for that first part of the username function, I want it to then check the characters to see if they are in compliance and do the exact same as the first part of the function if there is an error. Finally, if there is no error for that second part of the username function, it then checks to see if the username is taken of not then no errors occured. If it is taken, then an error needs to be placed.

There's a few ways you can do this. The way I would do it, is have a foreach on your $_POST (I assume it's POST) then check for array_key_exists(). Then, if it does, foreach that field to get the rules, then have a switch to bucket the different rules into methods. Something like this:


foreach($_POST as $key=>$value){
if(array_key_exists($key,$this->fields)){
foreach($this->fields as $rule){
try{
// Pretty sure you can do this, where $rule is a method name inside the class
$this->$rule($value);
catch(Exception $e){
echo '<br/>Exception: '.$e->getMessage();
}
}
}
else{
// No rule!
}
}

This will spit out an exception every time a rule failes. Although, in your rule, you will have to throw a new exception when you find an error.

Chris Hick
01-15-2012, 07:09 PM
Panther, that is a good suggestion. Certainly do able. But it will not help me with the way I want to return the errors. On my page, I have something like this beside each form input.


if(isset($validation->errors['username'])) { echo $validation->errors['username'];}
// substituting username for the name of that input field

MattF
01-15-2012, 07:31 PM
Panther, that is a good suggestion. Certainly do able. But it will not help me with the way I want to return the errors. On my page, I have something like this beside each form input.


if(isset($validation->errors['username'])) { echo $validation->errors['username'];}
// substituting username for the name of that input field


You're assigning all errors to an array and merely want to print those errors if any exist?

Chris Hick
01-15-2012, 07:52 PM
MattF, in a nutshell, yes. I want each error to have the key of its fieldname.

MattF
01-15-2012, 08:12 PM
If you're just wanting to show all errors once you've processed the form, do something along the lines of:



if (!empty($validation->errors))
{
foreach ($validation->errors as $key => $error)
{
print($key.': '.$error."\n");
}
}
else
{
#All inputs correct.
}



p.s: I've only skimmed this thread, so may have missed reference to specifics of what you're trying to achieve. Apologies if so.

Chris Hick
01-15-2012, 09:04 PM
MattF, I notice a lot of sites do the method you mentioned above, but for personal reasons I don't like that idea. I like the idea of stating the field specific errors next to the field. Instead of all in one area.

MattF
01-15-2012, 09:30 PM
In that case, you'll need to reload the form with the message next to each input, (you're using the input name? as the key, so that will be no problem to tally the respective message to each input), and you can also use JS to validate the form, (if available), in realtime, hence avoiding a reload for JS enabled clients.

Chris Hick
01-15-2012, 10:41 PM
MattF, that is what my class is doing... The issue is what the OP states. That is why I am trying to fix.

MattF
01-15-2012, 11:10 PM
That's what I get for skim reading threads. :D So the problem you're having is that you only get one error message for each input, (the last error), even if there are several errors for that input? Something like the following may be more along the lines of what you want:



private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if ((strlen($username) < 6) || (strlen($username) > 32)) {
$this->errors["username"][] = "* Your username must be greater than 6 characters and less than 32 characters.";
} else if (!preg_match($username_regex, $username)) {
$this->errors["username"][] = "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .";
} else {
$users = User::find_by_username($username);
if (!empty($user)) {
$this->errors["username"][] = "* The username you selected has already been taken. Please select another.";
}
}

if (!empty($this->errors["username"]))
{
return implode("\n", $this->errors["username"]);
}
}

Chris Hick
01-15-2012, 11:24 PM
Haha Yeah precisely what the problem is. :thumbsup: I'll definitely try your way and see how it works. The only issue I am seeing is that you might think that you can get more than one error for method checkUsername at a time. That isn't right. I'm suppose to only get one error at a time for the method checkUsername. The issue is that isEmpty runs first then puts an error if its empty. Then, checkUsername runs regardless of the results of isEmpty and puts an error if there is one. But its only returning one error.

MattF
01-16-2012, 12:14 AM
Bugger. :D Talk about not being on the ball. I'm not even on the pitch at the moment. :D If I've finally got my brain into gear and am thinking about this correctly, the way I'd personally do it is to ditch the returns in your validation functions for the errors. Just assign all errors, (including the isEmpty result etc.), to $this->errors[$fieldname][] without any returns. Add another function to the class specifically for returning the errors array, and call that function when you've finished running the validation on the form fields. I'd also change the if/else clauses in the username check to separate if clauses, as mentioned by someone else earlier. You may have more than one validation error with the input.

Hope that makes sense. :)

Chris Hick
01-16-2012, 02:15 AM
Ok, this is how I tried to do it, and got the same exact results.... =/
validation class


<?php
// this is a helper class Form Validation

class FormValidation {

// public properties
public $errors = array();

// private properties

public $isValid = true;

function __construct($fields) {
$this->getRules($fields);
if(!$this->isValid) {
return $this->errors;
}
}

private function isEmpty ($field) {
if (!isset($_POST["{$field}"]) || empty($_POST["{$field}"])) {
$this->errors["{$field}"] ="* ". ucfirst($field) . " cannot be empty.";
}
}
private function getRules($fields) {
foreach ($fields as $field=>$rules) {
if(array_key_exists($field, $_POST)){
foreach ($rules as $rule) {
if(method_exists($this,$rule)) {
$this->$rule($field);
}else {
$this->errors["method"] = "This rule does not exist";
}
}
} else {
$this->errors["exist"] = "This field does not exist";
}
}
}

private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if ((strlen($username) < 6) || (strlen($username) > 32)) {
$this->errors["username"] = "* Your username must be greater than 6 characters and less than 32 characters.";
}
if (!preg_match($username_regex, $username)) {
$this->errors["username"] = "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .";
}
$users = User::find_by_username($username);
if (!empty($user)) {
$this->errors["username"] = "* The username you selected has already been taken. Please select another.";
}
}

private function checkErrors() {
if(!empty($this->errors)) {
$this->isValid = false;
}
}


}


?>


the php page


<?php
$action="sign_up.php";
if (array_key_exists('submit',$_POST)) {
$username = trim($_POST['username']);


$fields = array(
'username' =>
array("isEmpty", "checkUsername"),
);

$validation = new FormValidation($fields);

if(!isset($validation->errors)) {

$user = new User();
$user->username = $username;
if($user->save()) {
redirect_to("select_character.php");
} else {
$message = "An error occurred when trying to input into the database.";
}
}

} else { // form has not been submitted.
$username = "";
}
?>
<?php require_once("../database_info/templates/header.php"); ?>
<?php if (isset($message)) echo output_message($message); ?>
<?php require_once("../database_info/templates/sign_up_template.php"); ?>
<?php require_once("../database_info/templates/footer.php"); ?>


the sign up template


<form method="post" action="<?php $action ?>">
<fieldset>
<legend>Sign Up</legend>
<label for="username">Username:</label>
<input type="text" name="username" value="<?php if (!empty($username)) echo htmlentities($username); ?>" /><br />
<?php if(isset($validation->errors["username"])) { echo "<p>".$validation->errors["username"]."</p>"; }?>
</fieldset>
<input type="submit" value="Sign Up" name="submit" />
</form>

Chris Hick
01-16-2012, 05:16 PM
Anyone able to help??

BluePanther
01-16-2012, 06:17 PM
One thing I can see is your isEmpty method containing this:


$this->errors["{$field}"]

You don't really need the {}'s or the quotes. $this->errors[$field] will work fine. But, that's more of a style option, as your one will work (but slightly slower) too.
Also, the isValid bool is only changed in the checkErrors method, which is never called. So, isValid will always be the initialised value - True.
In your construct method, you have the option to return the errors - construct shouldn't return anything.

Appart from that, I don't really see what's wrong. Put some debugging echo's into the methods you expect to be called to make sure they're actually being called.

Chris Hick
01-17-2012, 04:05 AM
Ok, so with a lot of experimentation and hours put in, I've come up with the following class and its implementation. Yet, I am not satisfied with how it looks. It doesn't seem like it has too much repetitive code . I have to create a new method anytime I want to check a field for more than one thing. Perhaps, after seeing this some of you can see what I am trying to do.

validation.php


<?php
// this is a helper class Form Validation

class TestValidation {

// public properties
public $errors = array();

// private properties

public $isValid = true;

function __construct($fields) {
$this->getRules($fields);
if(!$this->isValid) {
return $this->errors;
}
}

private function isEmpty ($field) {
if (!isset($_POST[$field]) || empty($_POST[$field])) {
$this->errors[$field] ="* ". ucfirst($field) . " cannot be empty.";
$this->isValid = false;
}
}
private function getRules($fields) {
foreach ($fields as $field=>$rules) {
if(array_key_exists($field, $_POST)){
foreach ($rules as $rule) {
if(method_exists($this,$rule)) {
$this->$rule($field);
}else {
$this->errors["method"] = "This rule does not exist";
}
}
} else {
$this->errors["exist"] = "This field does not exist";
}
}
}

private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if(!isset($_POST[$username]) || empty($_POST[$username])) {
$this->errors[$username] ="* Username cannot be empty.";
$this->isValid = false;
} else if ((strlen($_POST[$username]) < 6)|| (strlen($_POST[$username]) > 32)) {
$this->errors["username"] = "* Your username must be greater than 6 characters and less than 32 characters.";
$this->isValid = false;
} else if (preg_match($username_regex, $_POST[$username])) {
$this->errors["username"] = "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .";
$this->isValid = false;
} else {
$users = User::find_by_username($_POST[$username]);
if (!empty($users)) {
$this->errors["username"] = "* The username you selected has already been taken. Please select another.";
$this->isValid = false;
}
}
}
private function checkZipcode($zipcode) {
if(!isset($_POST[$zipcode]) || empty($_POST[$zipcode])) {
$this->errors[$zipcode] ="* Zipcode cannot be empty.";
$this->isValid = false;
}
else {
$zipcode_regex = '/[0-9]{5}/';
if(!preg_match($zipcode_regex, $_POST[$zipcode])) {
$this->errors["zipcode"] = "* Your zipcode is not valid.";
$this->isValid = false;
}
}
}

private function checkErrors() {
if(!empty($this->errors)) {
$this->isValid = false;
}
}


}
?>


This is the page with the checks:


<?php // sign up test
require_once('../database_info/includes/initialize.php');
if($session->is_logged_in()) {
redirect_to("profile.php");
}
$action="sign_up.php";
if (array_key_exists('submit',$_POST)) {
$username = trim($_POST['username']);
$zipcode = trim($_POST['zipcode']);

$fields = array(
'username' =>
array("checkUsername"),
'zipcode' =>
array("checkZipcode")
);

$validation = new TestValidation($fields);

if($validation->isValid) {
echo "There were no errors";
}

} else { // form has not been submitted.

$username = "";
$zipcode = "";
}
?>
<?php require_once("../database_info/templates/header.php"); ?>
<?php if (isset($message)) echo output_message($message); ?>
<?php require_once("../database_info/templates/test_template.php"); ?>
<?php require_once("../database_info/templates/footer.php"); ?>


This is the form template-test_template.php


<form method="post" action="<?php $action ?>">
<fieldset>
<legend>Sign Up</legend>
<label for="username">Username:</label>
<input type="text" name="username" value="<?php if (!empty($username)) echo htmlentities($username); ?>" /><br />
<?php if(isset($validation->errors["username"])) { echo "<p>".$validation->errors["username"]."</p>"; }?>
<label for="zipcode">Zipcode:</label>
<input type="text" name="zipcode" value="<?php if (!empty($zipcode)) echo htmlentities($zipcode); ?>" /><br />
<?php if(isset($validation->errors["zipcode"])) { echo "<p>".$validation->errors["zipcode"]."</p>"; }?>
</fieldset>
<input type="submit" value="Sign Up" name="submit" />
</form>

BluePanther
01-17-2012, 08:30 AM
if(!$this->isValid) {
return $this->errors;
}

That snippet in the constructor is useless, I would just get rid of it. Like I said in my previous post, constructors shouldn't return values and in fact, won't do anything with that.

Other than that, it looks fine. Having to create a method for a new rule is a good thing, as it allows you to define what the rule actually is. I mean, you could have some methods that return bool's for string length or w/e, but what you've got is fine.

Chris Hick
01-17-2012, 08:45 PM
Thanks for all the inputs, I have finally come up with a solution to my problem. I believe this was is much more efficient and flexible. It has basic form validation and allows for one to write custom methods to suit their needs.

Validation Class


<?php
// this is a helper class Form Validation

class TestValidation {

// public properties
public $errors = array();

// private properties

public $isValid = true;

function __construct($fields) {
$this->getRules($fields);
}

private function isEmpty ($field) {
if (!isset($_POST[$field]) || empty($_POST[$field])) {
$this->errors[$field] ="* ". ucfirst($field) . " cannot be empty.";
$this->isValid = false;
}
}
private function getRules($fields) {
foreach ($fields as $field=>$rules) {
if(array_key_exists($field, $_POST)){
foreach ($rules as $rule) {
if(method_exists($this,$rule)) {
if(empty($this->errors[$field])){
$this->$rule($field);
}
}else {
$this->errors["method"] = "This rule does not exist";
}
}
} else {
$this->errors["exist"] = "This field does not exist";
}
}
}
private function checkUsername($username) {
$username_regex = '/[^a-zA-Z0-9_-]/';
if ((strlen($_POST[$username]) < 6)|| (strlen($_POST[$username]) > 32)) {
$this->errors["username"] = "* Your username must be greater than 6 characters and less than 32 characters.";
$this->isValid = false;
} else if (preg_match($username_regex, $_POST[$username])) {
$this->errors["username"] = "* Your username can only consist of lowercase and uppercase letters, any numbers between 0-9, _, or - .";
$this->isValid = false;
} else {
$users = User::find_by_username($_POST[$username]);
if (!empty($users)) {
$this->errors["username"] = "* The username you selected has already been taken. Please select another.";
$this->isValid = false;
}
}
}
private function checkZipcode($zipcode) {
$zipcode_regex = '/[0-9]{5}/';
if(!preg_match($zipcode_regex, $_POST[$zipcode])) {
$this->errors["zipcode"] = "* Your zipcode is not valid.";
$this->isValid = false;
}
}

private function checkEmail($email){
$email_regex = '/^[a-zA-Z0-9][a-zA-Z0-9\._\-&!?=#]*@/';
if(!preg_match($email_regex, $_POST[$email])) {
// email is invalid because the LocalName is bad.
$this->errors["email"] = "* Your email is not valid.";
$this->isValid = false;
} else {
// strip out everything but hte domain for the email
$domain = preg_replace($email_regex, '', $_POST[$email]);
//Now check if $domain is registered
if(!checkdnsrr($domain)) {
$this->errors["email"] = "* Your email is not valid.";
$this->isValid = false;
}
}

}

private function checkPassword($password) {
if((strlen($_POST[$password]) < 6)) {
$this->errors["password"] = "* Your password must be longer than six character.";
} else if (!preg_match('/[a-z]/', $_POST[$password]) || !preg_match('/[A-Z]/', $_POST[$password]) || !preg_match('/[0-9]/', $_POST[$password])) {
$this->errors["password"] = "* Your password must have one lowercase letter, one uppercase letter, and one number between 0-9.";
}
}

private function checkBirthday($birthday) {
list($month, $day, $year) = preg_split("/[\s-\/]+/", $_POST[$birthday]);
if (!checkdate($month, $day, $year)) {
$this->errors["birthday"] = "* This is not a valid date. Please choose your correct birthday.";
$this->isValid = false;
}
}

private function checkErrors() {
if(!empty($this->errors)) {
$this->isValid = false;
}
}


}


?>


This is how you use the class for those who read this later:


<?php

$fields = array(
'username' =>
array("isEmpty", "checkUsername"),
'password' =>
array("isEmpty", "checkPassword"),
'email' =>
array("isEmpty", "checkEmail")
);

$validation = new TestValidation($fields);
?>

Let me know if any of you disagree or think something should be added.



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum