Validator class for PHP

Question

I've made this validation class in PHP that would take inputs, on initialization sanitize them and validate them specific to the type. It works fine, but I would like to know if there's some bad practice involved and how I can improve it. I would also love to hear if there's anything that makes the code awful or hard to read. Does it follow good coding standards? If not, how can I improve it?

Class Validate {
    //Take the user input into the data put it to a class.
    private $data; //Function argument
    private $err; // boolean    
    public $empty; 
    private $errMsg;

    public function __construct($data) { //Not sure about this one. Have to check it out later and decide

        //perform some basic level validation like checking for emptiness.
        foreach ($data as $k =>$v) {
            $data[$k]= $v;
            if($this->checkEmpty($v)==1) {
                $this->empty=true;

            }
        }

    }

    private function checkEmpty($data) {
        if($data==""|| empty($data) || !isset($data)) {
    return 1;
        }
    }
    private function basicSanitize($data) { //Basic level input santization to be used by other functions only.

    $data=htmlspecialchars($data);
    $data=trim($data);
    $data=stripslashes($data);

    return $data;
    }

    public function sanitize($data) { // Sanitization at a massive level to sanitize a lot of inputs at one go.
        foreach ($data as $k =>$v) {
            $this->data[$k]= $this->basicSanitize($v);
            }

            return $this->data;
    }

    public function name($data) {

        if(strlen($data)<3) {

            $this->errMsg = "Name too short.";
        }

        else if(strlen($data)>100) {
            $this->errMsg = "Name too big.";
        } 


        if(isset($this->errMsg)) {
            return $this->errMsg;
        }

        else return true;
    }

    public function email($data) {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
             $this->errMsg = "Invalid email format"; 
        }

        if(isset($this->errMsg)) {
            return $this->errMsg;
        }

        else return true;
    }

}

And here is calling the class:

    $name="J";
    $email="mephone@2d.com";

    $input= array (
        'name'=>$name, 
        'email'=>$email
        );

    $val=new Validate($input);
    if($val->empty==false) {

        $clean=$val->sanitize($input);

        $nameErr = $val->name($clean['name']);
        if($nameErr)  {
        echo $nameErr;
        }

        else echo $clean['name'];

Should I be calling the error messages like that? What could be a more systematic and better way to do that?

Answer 1

Sanitize Functions

First of all, note that your class does two very different things:

• It checks if data is valid, returning either true or false (name, email).
• It changes the data so that it is valid (sanitize).

Combining both of these functionalities in the same class can get confusing quickly.

construct

Why does the constructor do sanitation? You already have basicSanitize, shouldn't that be responsible? And where is the result actually checked?

basicSanitize

The name alone already suggests that this isn't a great function. It's so generic, this function could be doing anything.

Let's look at what it is doing (adapted from my answer here, as these kinds of functions are quite often combined, even though they shouldn't be):

• trim: removes whitespace from beginning and end of string. This might be useful in some situations, and doesn't hurt in too many situations. It doesn't really affect security.
• stripslashes: as the name suggests, it removes slashes. This is only useful if magic quotes is on (which is the functionality that adds slashes), so you should check that first, as otherwise you may corrupt your data. If magic quotes is on (it shouldn't be), using stripslashes is recommended, as otherwise you will have corrupted data.
• htmlspecialchars again, this might corrupt your data (as in, you are not saving your original data in the database, but changed data). It is proper defense against XSS in most (but not all) situations (see here for exceptions, and use ENT_QUOTES), but it really should not happen before inserting data into the database, for two reasons: 1. corrupted data 2. This approach makes it easy to introduce XSS vulnerabilities. XSS is a vulnerability that is introduced when echoing data, so that is when it should be prevented. Otherwise, it will be hard to remember which data is secure and which is not.

To summarize: Your approach changes input data before it is inserted into the database, which could impact usability. This doesn't even add that much security, and is certainly not considered a best practice approach to security.

name

This functions doesn't really have anything to do with names. I'm glad that you are not too restrictive, but really, this is nothing more than a length check, so it might as well be called checkLength($data, $min, $max).

email

This is fine. FILTER_VALIDATE_EMAIL doesn't follow any email specs, but as it's PHPs inbuilt filter, it seems ok to use it.

Style / Coding Standards

I'm glad that you asked about this, as there are a couple of issues:

• Your indentation is off, making your code difficult to read.
• use curly brackets for all statements, even one-line statements.
• Your spacing is off.
• Your function level comments could be PHPDoc comments.

Use any IDE to fix most of these issues automatically.

Structure

Your class has state, but doesn't really need it, as your private fields are never used across functions. So they are just making your class unnecessarily complex.

You can just make your functions static. For example, your email function could just as well look like this:

public function email($data) {
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
         return "Invalid email format"; 
    }
    return true;
}

And your name function:

public function name($data) {
    if(strlen($data) < 3) {
        return "Name too short.";
    } else if(strlen($data) > 100) {
        return "Name too big.";
    } 
    return true;
}

API

Your class is very awkward to use. I imagine it works something like this:

$validate = new Validate($_GET); // this seems to make sense, but actually will not do anything
$validatedName = $validate->name($_GET['name']);
if (!$validatedName) {
    // handle $validatedName, which contains an error message
} else {
    // everything is fine
}

You'll notice that this doesn't actually work!

The reason for this is that strings as well as true evaluate to true with weak comparison. So instead, I'll need $validatedName !== true. And I will need to remember to use strict comparison each time, as I will otherwise have a - possibly quite serious - bug in my code.

The main problem is that you are sending error messages via the same mean as your return value, which just complicates things.

There are three different solutions that come to mind:

• get rid of the error message and just return true or false. For many filters this will make perfect sense (if the email filter failed, it's because the input isn't an email, no need to explain).
• store the error message in a field, which can later be accessed if needed.
• throw an exception containing the error message on failure.

Answer 2

First off, not sure why checkEmpty is returning an integer instead of a boolean. Second off, why are you setting $this->empty=true; if one of the possibly many inputs is empty?

Also this line is just setting the input data back to itself with $data[$k] = $v. You would want either $this->data[$k] = $v or just set the whole $this->data = $data and then loop through the class's data object.

foreach ($data as $k =>$v) {
    $data[$k]= $v;
    if($this->checkEmpty($v)==1) {
        $this->empty=true;

    }
}

When returning the error message, you do not need that else there since the return true is the only other thing that can happen.

Last but not least, in the checkEmpty method, I would check to see of the variable is set before doing anything else.

Answer 3

Really good answer from @tim so I won't re-hash those points other than to emphasize what he said about need for "state". I will say that I have no idea why you would ever want a concrete instantiation of this class. I would likely make a class of this sort have only static methods.

That way you can promote usage like this:

// for true false usage
if(Validator::validateEmail($email)) {
    // do something
}

/*
Or, if you want your validations to throw exceptions
(not common, but you might have some cases where you would never expect
a validation to fail and want to throw and exception)
*/
try {
    Validator::validateEmail($email);
    // maybe other validations
} catch (Exception $e) {
    // handle exception
}