PHP Validation Class

I'm looking for advice and ways on making it better.

Code

```
errorText = $errorText;
}

public function rule($value, $name, $inputRules)
{

$inputRules = explode('|', $inputRules);
foreach ($inputRules as $inputRule) {

if (!strlen($value) && $inputRule != 'required') {
break;
}

if (preg_match('/\[(.*?)\]/', $inputRule, $match)) {

$rule = explode('[', $inputRule);
$rule = $rule[0];
$param = $match[1];
if (!method_exists($this, $rule)) {
throw new Exception("Method {$rule} does not exist");
exit;
}

$call = array($value, $param);
}
else {
if (!method_exists($this, $inputRule)) {
throw new Exception("Method {$inputRule} does not exist");
exit;
}
$rule = $inputRule;
$call = array($value);
}

$response = call_user_func_array(array($this, $rule), $call);

if ($response) {
$error = $this->errorText[$rule];
$replace = array(
':name' => $name,
':param' => (isset($param)) ? $param : NULL
);
$response = str_replace(array_keys($replace), array_values($replace), $error);

$this->_errors[] = $response;
}

}

}

public function exec()
{
return (empty($this->_errors)) ? false : $this->_errors;
}

/*
* Rule functions
*/

public function min_length($value, $param)
{
return (strlen($value) $param);
}

public function email($value)
{
return !filter_var($value, FILTER_VALIDATE_EMAIL);
}

public function required($value)
{
return (strlen($value) !== 0);
}

public functio

Technical Issues

public function __construct()
{
    require_once 'errors.php';
    $this->errorText = $errorText;
}

Will not do what you think it will do.

$val1 = new Validate();
$val2 = new Validate();

The second instance will not include 'errors.php', and thus $errorText will not be defined.

You should probably consider generalizing your error message handling since you might need to have different messages or internationalization.

In other words, your class should probably either return error codes (class constants), or your class should have some kind of facility for requesting a different error message at run time.

If you do stick with your current approach though, consider structuring it something like:

public function __construct()
{
    $this->errorText = $this->getErrorText();
}

private function getErrorText()
{
    include 'errors.php';
}

Your errors.php would then look like:

 'Some message'
    'somethingElse' => 'some other message',
    ...
);

This would of course mean that the file is included on every object instantiation though. A different approach could be to store the messages in a static variable the first time they're needed.

Also, while I'm talking about this, errors.php could be a problematic path. You should probably use something like dirname(__FILE__) . '/errors.php'; so the path is more resistant to outside factors.

if (!method_exists($this, $rule)) {
    throw new Exception("Method {$rule} does not exist");
    exit;
}

There's two issues here:

• exit will never be reached

• exit shouldn't here

When you throw an exception, you bail out of the current scope up until you hit a catch block. No code under the throw will be executed.

function f() {
    throw new Exception();
    //No code here is ever excecuted
}

function f() {
    if (...) {
        throw new Exception();
        //Code here is never excecuted
    }
    //Code here will be executed only if the throw above is not reached
}

function f() {
    try {
        if () {
            throw new Exception();
            //Code here is never exceuted
        }
        //Code here is executed if the throw is not reached
    } catch (Exception $e) { }
    //Code here is always executed
}

It gets a bit more complicated than that, but those examples should illustrate the basic behavior.

exit shouldn't be in there anyway though. Would you actually want script execution to completely stop? Typically exit should only be used if there is no way to continue the program execution without compromising security or stability.

Imagine if instead of throwing exceptions, you had only the exits there. This means that no error message would be displayed. Nothing would be logged. The code would just bail.

Imagine if someone was using your class based only on the API. They would have no idea why the code exited.

Typically low level flow control like exiting or redirecting to a different page, so on, should be handled in dedicated "controller" code and not in classes that have responsibilities unrelated to that.

Throwing an exception is the right thing to do here. It allows the code that called rule to decide how to handle it. rule should only responsible for saying "Hey, ummm, something wen't horribly wrong," not deciding how to handle it. (In fact, an uncaught exception is basically an exit with debugging information included :p. You do have to be careful with uncaught exceptions though as you need to make sure they don't get displayed to end clients. Some exceptions, like PDOException, may contain sensitive data like database credentials.)

Design Issues

Your class does way too much. This is just using a class as a collection of functions. Each validation can be seen as a "responsibility" and thus your class completely violates the single responsibility principle.

Imagine that in the future you want to add a new validation. How can you do this? Well, you have two options, both of which are unpleasant:

-
Edit the source code. What if many people are using your class though, or you're using it across 10 applications? Suddenly you have a lot of version management going on. Since you control the class, you could just commit it all to git which would alleviate the synchronization, but what about end-consumers of your code? This isn't a viable option for them.

-
Extend the class. This is also a bad option. You would have to change all of the object types in your code to be a MyValidation instead of Validation. Depending on how widespread this was, this could be a huge chore.

Or, you have a third option. Scrap your current design and abstract each validation into its own class. You could have a very simple interface:

```
interface Validator
{

/**
* Validates the given $data against whatever rule(s) the implementation of this
* interface enforces. Returns an array of error messages, or an empty array
* if

StackExchange Code Review Q#18814, answer score: 8