makes sense as validation?

[cluelessPHP]

Makes sense as a validation class?

<?php 
class Validation{
	private $_fails = [];
	
public function validator($input, $validation = []){
	
	foreach($input as $inputs => $input_value){
		if(key_exists($inputs, $validation)){
			foreach($validation[$inputs] as $tests => $test_value){
				
				if(is_int($tests)){
					$tests = $test_value;
					
					switch($tests){
						case 'required':
						if(empty($input_value) && $test_value)
						{
							$this->addError($inputs,ucwords($inputs). ' required');
						}
						break;
						
							case 'minLength':
						if(empty($input_value) > $test_value)
						{
							$this->addError($inputs, ucwords($inputs). ' should be minimum '.$rule_value. ' characters');
						}
						break;
						
						case 'maxLength':
						if(empty($input_value) > $test_value)
						{
							$this->addError($inputs, ucwords($inputs). ' Cannot be more than '.$rule_value. ' characters');
						}
					}
						
				}
			}
			
		}
	}
	
}

 private function addError($inputs, $errors){
    $this->_fails[$inputs][] = $errors;
}


public function error(){
    if(empty($this->_fails)) return false;
    return $this->_fails;
}
}

[pbismad]

cluelessPHP foreach($input as $inputs => $input_value){
if(key_exists($inputs, $validation)){

If the above is blindly looping over all the input data, then testing if there's a validation rule, you need to reverse the logic, and loop over an expected list of inputs and test/validate the corresponding input. Hackers/bots can submit 1000's of inputs. You don't want to loop over all of them as this uses resources and will trigger errors, which is what hackers want.

cluelessPHP if(is_int($tests)){

is_int() doesn't do what you think. It tests if the variable is of type integer. By definition, all external data, $_GET, $_POST, $_COOKIE are string variables, regardless of what is in them, so, using is_int() won't ever be true for external data.

I don't see anywhere that you are trimming the values before validating them. This is the one 'alteration' to input data that you should do.

[NogDog]

A switch() inside an if() inside a foreach() inside an if() inside a foreach() is a definite "code smell". Therefore, if I understood exactly what you were trying to do with all those conditions etc., I'd want to refactor it, maybe adding a few methods to compartmentalize things?

I suspect you want strlen() instead of empty() for the minLength and maxLength cases (plus probably a > for that latter)?

Why do you do $tests = $test_value;? You do not appear to use $tests after that.

[cluelessPHP]

NogDog

A switch() inside an if() inside a foreach() inside an if() inside a foreach() is a definite "code smell". Therefore, if I understood exactly what you were trying to do with all those conditions etc., I'd want to refactor it, maybe adding a few methods to compartmentalize things?

Think I got myself tied in knows, I still have no idea what's expected of a junior developer tbh. I've asked this question in various places over time and never actually gotten an answer, kind of frustrating

[cluelessPHP]

pbismad

Ah thanks, wasn't sure about that

I don't see anywhere that you are trimming the values before validating them. This is the one 'alteration' to input data that you should do.

I thought I could do something like

$post = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);		

[NogDog]

Not saying this is exactly how I'd do it, but trying to keep your general idea and just breaking things up into more atomic pieces, maybe:

<?php

class FormValidation {
    protected $errors = [];
    public function validateFields(Array $fields, Array $rules) {
        $this->errors = [];
        foreach($fields as $field => $value) {
            if(isset($rules[$field])) {
                foreach($rules[$field] as $rule => $param) {
                    $this->validate($field, $value, $rule, $param);
                }
            }
        }
        return empty($this->errors);
    }
    public function errors() {
        return $this->errors;
    }
    protected function validate($name, $input, $validationType, $validationValue=null) {
        if(!$this->$validationType($input, $validationValue)) {
            $this->errors[$name][$validationType] = $validationValue;
        }
    }
    protected function required($input, $value=null) {
        return !empty(trim($input));
    }
    protected function minLength($input, $length) {
        return $this->length($input) >= $length;
    }
    protected function maxLength($input, $length) {
        return $this->length($input) <= $length;
    }
    protected function length($input) {
        return strlen(trim($input));
    }
}

// TEST
$validationRules = [
    'first_name' => [
        'required' => true,
        'minLength' => 2,
        'maxLength' => 40
    ],
    'last_name' => [
        'required' => true,
        'minLength' => 2,
        'maxLength' => 40
    ],
    'middle_name' => [
        'required' => false,
        'maxLength' => 40
    ]
];

$validator = new FormValidation();

$testData = [
    'first_name' => 'Nog',
    'last_name' => 'Dog',
    'middle_name' => 'X',
];

echo "Should pass:\n";
$test = $validator->validateFields($testData, $validationRules);
var_dump($test);
var_export($validator->errors());

$testData = [
    'first_name' => '   ',
    'last_name' => 'Dog',
    'middle_name' => 'Xdkdkdkdkdkdkdkdkdkdkdkdddddddasdfasdfasdfasdfasdfasdfasdfadfsasdfasdfasdf',
];

echo "Should fail:\n";
$test = $validator->validateFields($testData, $validationRules);
var_dump($test);
var_export($validator->errors());

Output:

$ php validator.php
Should pass:
bool(true)
array (
)
Should fail:
bool(false)
array (
  'first_name' =>
  array (
    'required' => true,
    'minLength' => 2,
  ),
  'middle_name' =>
  array (
    'maxLength' => 40,
  ),
)

[cluelessPHP]

NogDog Oh I see that makes more sense