I have written Validation and Form classes in PHP.Validation class allow you to define the rules of each field and a error message. Form class allow you to get a error message and also allow you to get a value of POST via $_SESSION. I stored in $_SESSION because if validation did not pass it will need to redirect back to main Form page and repopulate all the fields using value() method. What do you think of Validation and Form classes - what can be improved?Validation Classclass Validation{    protected  $fields = array();    private $errors = array();    public function setRules($field, $errorMessage, $rules = array())    {        if (count($_POST) == 0) {            throw new Exception(The array of post parameters is empty);        }        if ($field == '') {            throw new Exception(field parameter is empty);        }        if (!is_array($rules) || count($rules) == 0) {            throw new Exception(The array of rules parameter is empty);        }        $errorMessage = ($errorMessage == '') ? $field : $errorMessage;        $this->fields[] = array(            'name' => $field,            'errorMessage' => $errorMessage,            'rules' => $rules,        );    }    public function validate()    {        if (count($_POST) == 0) {            throw new Exception(The array of post parameters is empty);        }        if (count($this->fields) == 0) {            throw new Exception(Validation rules is not set);        }        foreach ($this->fields as $field) {            $fieldName = $field['name'];            if (isset($_POST[$fieldName])) {                foreach ($field['rules'] as $rule) {                    $param = false;                    if ($arr = explode('=', $rule)) {                        if (isset($arr[0]) && isset($arr[1])) {                            $param = $arr[1];                            $rule = $arr[0];                        }                    }                    $output = $this->$rule($_POST[$fieldName], $param);                    if ((in_array('required', $field['rules']) && $output == FALSE) || (!in_array('required', $field['rules']) && $output == FALSE && $_POST[$fieldName] != '')) {                        if (!isset($this->errors[$fieldName])) {                            $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']);                        }                    }                }            }        }        return (count($this->errors) == 0) ? true : false;    }    public function getError($field)    {        if (isset($this->errors[$field]) && $this->errors[$field] != '') {            return $this->errors[$field]['errorMessage'];        }        return false;    }    private function validEmail($value)    {        return filter_var($value, FILTER_VALIDATE_EMAIL);    }    private function required($value)    {        return ($value != '') ? true : false;    }    private function numeric($value)    {        return is_numeric($value) ? true : false;    }    private function exactLength($value, $param)    {        return (strlen($value) == $param) ? true : false;    }}Form Classclass Form{    private $validation;    function __construct($validation) {        $this->validation = $validation;        $_SESSION['posts'] = $_POST;    }    public function error($field)    {       return $this->validation->getError($field);    }    public function value($field) {        if (isset($_SESSION['posts'][$field]))            return $_SESSION['posts'][$field];    }}Usage Testing:// FAKE POST TEST$_POST['name'] = 'Pau';$_POST['email'] = 'hello@google';$val = new Validation();$form = new Form($val);$val->setRules('name', 'Please enter your full name', array('exactLength=6','required'));$val->setRules('email', 'Please enter your email', array('validEmail'));if ($val->validate()) {    echo Good! - Validation Passed;} else {    echo Bad! - Validation Not Passed;    echo $form->error(name);}
Validation and Form classes
php;design patterns;classes;php5;validation
Well, first off, I think using client side cookies, instead of session cookies, might be a little better here. It puts less strain on your server and keeps unnecessary transfers from occurring. But the difference might be trivial, depending on the type of traffic you are expecting. Corbin touched on a few good points, +1 to him, below are some that I found.$fields should only be protected if it is being shared, either by a parent or child class. Since the Validation class isn't extending anything I think its safe to say that no parent classes are using it. Its a little harder to tell if it has any children that might use it. I don't see any in the code you posted, but that doesn't mean its not being done somewhere else. Just know that protected means a property is to be shared but not available outside of a parent/child relationship, otherwise it should be public or private, depending on how you are planning on using it. Though typically public properties are avoided in favor of setters/getters.As Corbin has said, you should pass in the POST array instead of explicitly calling for it in your class. For instance, say you parsed a JSON file and wanted to verify it before using it, this class would be the perfect candidate, but because you have limited it to using only POST values you will either have to create a new class just for JSON, or you will have to modify this one to use both POST and JSON, and the cycle will continue the more things you add. Better to just generalize it now. This will also mean that verifying that the POST array was set will become unnecessary, at least here.Also, an easier way to determine if an array is empty is to use the empty() function. This holds true for anywhere you are comparing the size of an array to zero.if( ! is_array( $rules ) || empty( $rules ) ) {Since it appears obvious that you only want an array to be passed as your ruleset, perhaps a better way of passing the $rules parameter would be to use type hinting in addition to a default value. This would throw an error automatically if some other form of variable was used, meaning you don't have to explicitly check it anymore, except for empty of course, you can just assume its an array and work from there.public function setRules( $field, $errorMessage, Array $rules = array() ) {If you are like me and are unlucky enough not to be able to use short ternary yet, then the following ternary statement is fine, but, if your PHP version is >= 5.3, then know that you can change it. Also, an empty string translates to a FALSE state, so explicitly checking it should be unnecessary; and those parenthesis are also unnecessary.$errorMessage = ($errorMessage == '') ? $field : $errorMessage;//The same thing in 5.3$errorMessage = $errorMessage ?: $field;Perhaps a better way to parse your rules would be to use array functions rather than explicitly declaring each array element. This will make your nested if statements completely unnecessary. Additionally, try not to define variables in your statements. It can make debugging hard, besides, the only way this function would return false is if you passed it an empty string. If you use array_filter() in your setRules() method before setting this value to the fields array then all such elements will automatically be removed from your array.$rules = array_filter( $rules );if( empty( $array ) { /* throw error */ }//complete setRules() and start validate()$ruleParams = explode( '=', $rule );$rule = array_pop( $ruleParams );$param = array_shift( $ruleParams );Be careful of variable-functions, or in this case variable-methods. Typically most people will glare at you and call you dirty names for doing this. They are very hard to debug for. I understand what you are trying to do, but I'm almost tempted to tell you to do it manually.The following if statement is working too hard. First, it is just too long. You can abstract some of those statements to make this a bit easier to read.if ((in_array('required', $field['rules']) && $output == FALSE) || (!in_array('required', $field['rules']) && $output == FALSE && $_POST[$fieldName] != '')) {//compared to$noOutput = $output == FALSE;$required = in_array( 'required', $field[ 'rules' ] );if( ( $required && $noOutput ) || ( ! $required && $noOutput && $_POST[ $fieldName ] ) ) {Additionally, the above statement is redundant. At least in as far as you are checking if a field exists in your array on every iteration. Do it once outside of the loop and use that instead. But what is this even doing? You are saying that if one parameter is required, they all are? Did you instead mean to check if $rule == 'required'? This just seems a bit odd.$required = in_array( 'required', $field[ 'rules' ] );foreach( $this->fields AS $field ) {    //etc...    if( ( $required && $noOutput ) || ( ! $required && $noOutput && $_POST[ $fieldName ] ) ) {Alright the next statement I sort of get, you don't want to overwrite any existing messages, but why not instead just log them all using a multidimensional array?if (!isset($this->errors[$fieldName])) {    $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']);}//compared to$this->errors[ $fieldsName ] [] = array( 'errorMessage' => $field[ 'errorMessage' ] );No need to explicitly define a TRUE/FALSE state for the return value. Just return the count, or empty() check, either one would result in the same thing, though I would use the latter as it does not require the added negation.return ! count($this->errors);//orreturn empty( $this->errors );The following statement will always be true, at least partially. Each element of the $errors property is an array, so it will never be equal to an empty string. Maybe you meant to use an empty() check here, but even that should be impossible. The $errors property is private and I don't see any method unsetting values from this array, therefore, if the $field index is set, the value should be as well. Just remove the second check.if (isset($this->errors[$field]) && $this->errors[$field] != '') {//compared toif( isset( $this->errors[ $field ] ) ) {Then the above could be rewritten so that you only have one return statement. Though this ternary statement is getting a bit long and complicated, so this might be unwise.return isset( $this->errors[ $field ] ) ? $this->errors[ $field ] [ 'errorMessage' ] : FALSE;Please, always use braces on your statements. PHP inherently requires them, otherwise you wouldn't have to add them if your statements extend more than one line. This would be different if that wasn't the case, but it is and it can cause issues. Besides, this is inconsistent with the rest of your code.if (isset($_SESSION['posts'][$field]))    return $_SESSION['posts'][$field];//use the following insteadif( isset( $_SESSION[ 'posts' ] [ $field ] ) ) {    return $_SESSION[ 'posts' ] [ $field ];}Correct me if I'm wrong, but I'm not sure your usage tests works. You pass your validation class to the form class, but because nothing has been done to it yet it will not reflect in the form class. This means that calling for the errors from the form class is pointless. You should just wait to inject the validation class until you add everything you need to first.