2
\$\begingroup\$

I started improving my validator library even more, and I think I'm at the last final element (domain object / entity validations) before I can gladly say that my Validator library is completely done and it supports all of it.

Here you can see that my validator can already deal with regular variable inputs or non-entity inputs (certain forms for example).

I started figuring out how I could extend my validator to create domain object validators and here is my attempt.

Could anybody review it and see if you find any room for improvements and give constructive feedback?

User

class User
{
    public $name;
    public $email;
    public $age;
}

UserValidator

class UserValidator extends Validator
{
    public function __construct()
    {
        $rules['name']  = [new MaxChars(20), new Alpha(true)];
        $rules['email'] = [new Email()];
        $rules['age']   = [new Numeric(), new Between(13, 17)];

        $this->setRule('name',  $rules['name']);
        $this->setRule('email', $rules['email']);
        $this->setRule('age',   $rules['age']);
    }

    public function validate(User $user)
    {
        $this->errors = [];

        parent::validate('name',  $user->name);
        parent::validate('email', $user->email);
        parent::validate('age',   $user->age);

        return empty($this->errors);
    }
}

Validator

class Validator
{
    protected $rules  = [];
    protected $errors = [];

    public function getErrors($rulename = null)
    {
        if ($rulename) {
            return isset($this->errors[$rulename]) ? $this->errors[$rulename] : [];
        }

        return $this->errors;
    }

    public function setRule($name, Array $rules, $required = true)
    {
        $this->rules[$name]['rules'] = $rules;

        switch (true) {
            case ($required === true):
                $this->rules[$name]['required'] = 'Must not be null or empty.';
                break;
            case ($required === false):
                $this->rules[$name]['required'] = false;
                break;
            default:
                $this->rules[$name]['required'] = $required;
                break;
        }
    }

    public function validate($rulename, $input)
    {
        if ($this->rules[$rulename]['required']) {
            if (empty($input)) {
                $this->errors[$rulename][] = $this->rules[$rulename]['required'];
            }

            foreach ($this->rules[$rulename]['rules'] as $rule) {
                if (!$rule->check($input)) {
                    $this->errors[$rulename][] = $rule->getError();
                }
            }
        } elseif (!empty($input)) {
            foreach ($this->rules[$rulename]['rules'] as $rule) {
                if (!$rule->check($input)) {
                    $this->errors[$rulename][] = $rule->getError();
                }
            }
        }

        return isset($this->errors[$rulename]);
    }
}

Example

$user        = new User();
$user->name  = 'John Doe';
$user->email = 'john@doe.com';
$user->age   = 13;

$uv = new UserValidator();

$uv->validate($user)); //true
$uv->getErrors());     //empty array
\$\endgroup\$
  • \$\begingroup\$ I think it's a bit of an overkill to have whole objects for Alpha or Numeric, don't you? \$\endgroup\$ – Madara Uchiha Jun 24 '14 at 14:19
  • \$\begingroup\$ Whats that [ ] around the instance? [new something] \$\endgroup\$ – Abandoned Account Jun 24 '14 at 14:43
  • \$\begingroup\$ @HassanAlthaf Shorthand for arrays. [] equals array(). \$\endgroup\$ – Kid Diamond Jun 24 '14 at 14:55
  • \$\begingroup\$ So what happens when he does that? \$\endgroup\$ – Abandoned Account Jun 24 '14 at 14:57
  • 1
    \$\begingroup\$ @KidDiamond In my validators, I just pass in callables to the validator class. Not everything is an object. This isn't Java. \$\endgroup\$ – Madara Uchiha Jun 24 '14 at 15:37
2
\$\begingroup\$

Coding-wise it looks pretty good. Consistent naming convention and code style, no apparent security vulnerabilities (assuming your Rule classes do what I expect them to do).

Let's talk about approach and scale.

Scale

It doesn't really make sense for you to write a validator for each Domain Object you create! What if you have 50 Domain Objects, will you write an extra 50 classes just for validation?

Approach

As a result of the above, your UserValidator class is very highly tightly coupled with the User object, it knows about the structure of the User object, making it useless for reuse (Unless, of course, you find another project with the exact same User object).

A better approach?

For starters, you should ask yourself what are you validating?

Let's differentiate between two possible "validations":

  1. Business rule validation - "User chose this city and this country, does it make sense?" or "User said he was born in 2012, does it make sense?"
  2. Data-integrity validation - "User already exists in the database" or "Username should contain between 3-20 characters".

Business rule validation belongs in the Domain Object itself. (i.e. the User object directly). While your data-integrity checks should be done by the RDBM.

More often than not, the RDBM (which is usually MySQL when talking about PHP) is not capable of doing all of that (which is why moving away to a better RDBM is encouraged), in which case those things also need to happen at the Domain Object level.

TL;DR

It's the job of the domain object to validate itself. It's the job of the storage manager (the database) to make sure the data is correct from the constraints point of view.

The validator class ends up (usually) as a utility class, which is nice to have, but unneeded for an actual project.

\$\endgroup\$
  • \$\begingroup\$ I'm a little bit confused now. So I could use my validation library just for forms in my controller. And have setters in my domain object that do the business rule validation, and since I'm using MySQL data integrity validation would happen there too. But what if I have a field in a form that's supposed to check if the value isn't already in the database? The form submit should not validate and return with an error stating that it already exists. But since the validation that checks for that will be in my domain object, how would I link it up with my form validation in my controller? \$\endgroup\$ – Kid Diamond Jun 26 '14 at 10:25
  • \$\begingroup\$ The database has the ability to induce UNIQUE constraints. You just need to catch the exception thrown by PDO. \$\endgroup\$ – Madara Uchiha Jun 26 '14 at 10:32
  • \$\begingroup\$ Okay, so the scenario would be: Form validation in my controller. After form inputs have passed validation, the values will be set to my domain object User, this object will be sent to my UserDAO object to try and persist it. If let's say the email already exists, catch the PDO exception and return it to my form validator, re-display the form and output the error that the input must be unique. That's right? \$\endgroup\$ – Kid Diamond Jun 26 '14 at 10:40
  • \$\begingroup\$ let's continue this discussion in chat \$\endgroup\$ – Madara Uchiha Jun 26 '14 at 10:45
3
\$\begingroup\$

I disagree with @Madara Uchiha on a several points.

  1. If you couple business rule validations to the domain object, you cannot apply different business rules to the same domain object.

    Never assume you will only have one set of business rules. By creating validators outside the domain object, you give yourself the flexibility of creating whole different swaths of business rules without touching the domain layer.

    Basically, by the time you go to save a domain object to a database or other repository it should be completely valid from a business rule AND storage perspective. This allows you to communicate problems to the user in a manor that allows them to correct their mistake. If you let a SQL error propagate up the call stack and result in a 500 Internal Server Error, you as a programmer have not done your due diligence when validating data.

    Now I could see a case for separating the storage validations from the business rules. Maybe you start out persisting your domain objects in a database, but later you move to a RESTfull web service. The web service may have different basic data validations, which you can then swap out for a different validator while keeping your existing business rule validations.

  2. I disagree that this solution doesn't scale. Quite the opposite actually. It scales very well. Mashing your validations into the domain object is what doesn't scale. Changing the storage mechanism or needing to apply different categories of business rules gets complicated when your domain objects validate themselves. Domain objects should be mere boxes containing data. The decision of whether that data is correct is wholly out of scope for the domain object, in my opinion.

  3. The storage manager should have absolutely no validations. It should accept a domain object, attempt to persist it and throw an exception if anything goes wrong.

Really, all of my objections are related to this tenet of software engineering, and especially object oriented programming: Do one thing, and do it well.

Putting validations into your domain objects means they are serving a dual purpose: Hold data from a storage medium AND ensuring it is valid. Do one thing and do it well. The domain object should hold data. The validation layer should ensure things are valid.

Putting validations into your storage manager means it is serving a dual purpose in the same manor. You should be able to swap out one layer for another with minimal refactoring. If you have to copy and paste of rewrite lots of code, then your application needs additional layers with looser coupling.

\$\endgroup\$
  • \$\begingroup\$ Besides your disagreement, what you think about my code? \$\endgroup\$ – Kid Diamond Jun 26 '14 at 17:33
  • \$\begingroup\$ I think it is a good approach, though I am a tad biased since I submitted an answer to your previous code review. :) \$\endgroup\$ – Greg Burghardt Jun 26 '14 at 17:46
  • \$\begingroup\$ True, but I was just wondering since I left out your rule builder idea in my version. Other then that, I'm hearing so many different approaches when it comes to domain object validation. I guess at the end of the day I'll have to look at everything side by side and decide myself which way attracts me the most. \$\endgroup\$ – Kid Diamond Jun 26 '14 at 17:50
  • 1
    \$\begingroup\$ The lack of a rule builder is not a show stopper. And yes, you'll have to weigh the different approaches and go with the one that fits your needs. There are many ways to solve this problem. \$\endgroup\$ – Greg Burghardt Jun 26 '14 at 18:09
  • \$\begingroup\$ You shouldn't return a 500 error to the user when the database can't save his invalid data, you should catch the exception, and give a useful error back. Also, when would you need a different set of business rules? \$\endgroup\$ – Madara Uchiha Jun 29 '14 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.