simple entity validator - critique

[Destramic]

last year a made a validion script that works simular to this but using annotation (like symphony).  it was very long winded and messy, but although it worked perfectly i was never happy with how complicated it was.

well i had a brain wave last night on how to simplify a validator without any annotation bs!

 

this took me minutes to write, and would be nice to have an overall critique from you guru's.

validator:

<?php

class Validator
{
    private $entities       = array();
    private $error_messages = array();
    
    public function __construct(array $entities)
    {
        foreach($entities as $entity)
        {
            $this->check_entity($entity);
        }

        $this->entities = $entities;
    }
    
    private function check_entity($entity)
    {
        if (!is_object($entity))
        {
            throw new Exception('Validator: Entity must be an object.');
        }
        else if (!is_subclass_of($entity, 'Entity'))
        {
            throw new Exception('Validator: Parent entity has not been implimented.');
        }
        
        return true;
    }
    
    public function validate(array $data)
    {
        foreach ($this->entities as $entity)
        {
            foreach ($data as $property => $value)
            {
                if (property_exists($entity, $property))
                {
                    $entity->{$property} = $value;
                }
                
                if (method_exists($entity, $property))
                {
                    call_user_func_array(array($entity, $property), array('value' => $value));
                }
            }
            
             $this->error_messages = array_merge($this->error_messages, $entity->get_error_messages());
        }
    }
    
    public function get_error_messages()
    {
        return $this->error_messages;
    }   
    
    public function is_valid()
    {
        return empty($this->error_messages);
    }
}

 

login entity

<?php

include_once 'entity.class.php';
    
class Login extends Entity
{
    public $username;
    public $password;
    
    public function username()
    {
        if (empty($this->username))
        {
            $this->add_error_message('Username: username is empty');
        }
    }
    
    public function password()
    {
        if (empty($this->password))
        {
            $this->add_error_message('Password: password is empty');
        }
    }
}

 

abstract entity

<?php

abstract class Entity 
{
    private $error_messages = array();
    
    public function add_error_message(string $message)
    {
        $this->error_messages[] = $message;
    }
    
    public function get_error_messages()
    {
        return $this->error_messages;
    }
}

 

test it all out!

<?php

include_once 'validator.class.php';
include_once 'entity/login.class.php';

$validator = new Validator(array(new Login));

$post = array(
    'username' => 'john.doe',   
    'password' => null
);

$validator->validate($post);

if (!$validator->is_valid())
{
    print_r($validator->get_error_messages());
}

 

result:

Array ( [0] => Password: password is empty ) 

 

the script will work with multiple entities too.

 

thank you guys!

[requinix]

The nice thing about annotations is that they don't require you to write code.

If what you have there works better for you then use it.

However it doesn't feel right to me to put invalid values into the object. Yeah, it's not valid, but you still filled in the properties. Don't do that. If I were writing this approach, I would have the methods return the correct values to use. "if method_exists then property=return value from method(); else if property_exists then property=value without validation". The methods can return null or empty strings or whatever you want in case of failure.

[Destramic]

I feel with annotation also your force to learn a new syntax...I could also use traits for certian fields that are use widley throughout my site,  which is cool.

I concur with what your saying, and I'd also be able to get rid of the abstract too. I'll implement what you've said and try to build upon that.

Thank you for your great input once again.

 

[ignace]

Entities should not have error messages, they should throw exception's on invalid input. Error messages are to indicate to the user that what he input is invalid.

Why exceptions? Because your model must be a source of truth, it enforces the business rules.

The application (forms, buttons, ui) lies on top of your model. So for the application to communicate effectively with the model it must ensure all input is valid BEFORE the model is called.

The exception's are usually called like this in the model:

<?php

public function setBirthDate(DateTimeInterface $dt) {
  $this->assertGreaterThanOrEqualTo18($dt);
  $this->assertLessThan100($dt);
  
  $this->birthDate = $dt;
}

Because you may have these business rules. You can take it one step further:

<?php

class BirthDate {
  private $date;

  public function __construct(DateTimeInterface $date) {
    $this->assertGreaterThanOrEqualTo18($date);
    $this->assertLessThan100($date);
    
    $this->date = $date;
  }
}

public function setBirthDate(BirthDate $bd) {
  $this->birthDate = $bd;
}

Now every time you use a birthdate you will apply the business rules.

[Destramic]

interesting, ok so the user has inputted a date and your checking if the date is >= 18 or  <100 if so then set date and if not it'll throw an exception.

shouldnt a validation error message go back to the user in the form process? or should i just rely on client side validation for that.

and if they pass client side validation then exceptions will be thrown if invalid.

 

i hope you can elaborate on this please.

 

thank you

[ignace]

Yes, your form and any other client-side code should have validation that checks that all business rules (being 18+) have been enforced.

Validation and exceptions are not the same thing. Validations are done before you pass it down. While exceptions are thrown after it has been passed down and is invalid.

Because your model sits at the center of your application and is guarded with exceptions it becomes impossible to use them with invalid data. Which makes your life as a developer much easier.

And anything you add on top adds it's own validation and has it's own method of communicating error's back to the user.

[Destramic]

im trying to digest everything your saying here...so...

ok validating a form and returning error messages for a user is great...but for instance, if form data is passed to my business model after being validated would i need to validate the data again in themodel, but this time returning exceptions?

 

 

 

[Destramic]

here is what ive come up with:

 

<?php

class Validator
{
    private $entities;
    private $fields;

    private $error_messages = array();

    public function __construct($entities, array $fields = array())
    {
        if (!is_array($entities))
        {
            $entities = array($entities);
        }

        foreach ($entities as $entity)
        {
            $this->check_entity($entity);
        }

        $this->entities = $entities;
        $this->fields   = $fields;
    }

    private function check_entity($entity)
    {
        if (!is_object($entity))
        {
            throw new Exception('Validator: Entity must be an object.');
        }

        return true;
    }

    public function validate(array $data, $strict = false)
    {
        foreach ($this->entities as $entity)
        {
            foreach ($data as $property => $value)
            {
                if (!empty($this->fields) && !in_array($property, $this->fields))
                {
                    throw new Exception(sprintf('Validator: Unknown field %s', $property));
                }

                if (!method_exists($entity, $property))
                {
                    throw new Exception(sprintf('Validator: Unknown method %s', $property));
                }

                try
                {
                    $result = $entity->$property($value);
                }
                catch (Exception $exception)
                {
                    if ($strict)
                    {
                        throw new Exception($exception->getMessage());
                    }
                    else
                    {
                        $error_message = ucwords($property) . ': ' . $exception->getMessage();

                        $this->error_messages[$property] = $error_message;
                    }
                }
            }
        }
    }

    public function get_error_messages()
    {
        return $this->error_messages;
    }

    public function is_valid()
    {
        return empty($this->error_messages);
    }
}

class Assert
{
    public function __call(string $constraint, $arguments)
    {
        $class = $constraint;

        if (!class_exists($class))
        {
            throw new Exception(sprintf('Assert: Constraint %s doesn\'t exist.', $constraint));
        }
        else if (!method_exists($class, 'assert'))
        {
            throw new Exception(sprintf('Assert: Method assert doesn\'t exist for %s.', $constraint));
        }

        return call_user_func_array(array(new $class, 'assert'), $arguments);
    }
}

class Not_Blank
{
    public function assert($value)
    {
        if (empty($value))
        {
            throw new Exception('Value is empty');
        }

        return true;
    }
}

class Login extends Assert
{
    public function username($username)
    {
        $this->not_blank($username);
    }

    public function password($password)
    {
        $this->not_blank($password);
    }
}

 

form validation:  i added fields to be inputted to make validation stricter, becasue if post data is manipulated and there is no username or post in the data then the validator would return true.

$validator = new Validator(array(new Login));

$post = array(
    'username' => null,
    'password' => null
);

// fields added to be validated
$validator->validate($post, array(
    'username',
    'password'
));

if (!$validator->is_valid())
{
    print_r($validator->get_error_messages());
}

 

business model data validation...this is where im able to put validation to strict and it will return exceptions instead of getting friendly error messages

$validator = new Validator(array(new Login));

// true = exceptions will be returned
$validator->validate($data, true);

 

if you could tell me what you think and please elaborate on how form validation and business model validation should work

 

thank you

[ignace]

On 8/25/2018 at 7:50 PM, Destramic said:

im trying to digest everything your saying here...so...

ok validating a form and returning error messages for a user is great...but for instance, if form data is passed to my business model after being validated would i need to validate the data again in themodel, but this time returning exceptions?

Okay, let's try something simpler. Say you have this code:

<?php

declare(strict_types=1);

// ..

public function setAge(int $age) {
  $this->age = $age;
}

Passing a string to this method will throw an exception. The form processing code should validate that the age is indeed a number before passing it to your setAge() method. You can't start accepting "Dave", TRUE, ['foo']['bar'] = 'bat' as age. What I said above is basically the same thing. You protect your model from invalid data.

If the business you are writing this software for has other rules, like must be over the age of 18. Then you will need to add additional checks, that his rule is not broken. And this is where you use exceptions. To halt the code from polluting your model.

So to answer your question: yes, you must validate the data both in your model and in your form. The reasoning here is that your data might not always come from a form. But perhaps some data is input from a 3rd party API. Or from the CLI. Wherever the data comes from, you can be certain your model will always be valid. If you are too lazy to write out all the exceptions, follow these rules:

1. Use this library: https://github.com/webmozart/assert
2. Write custom value objects like the example in my previous post: BirthDate. Which you can use over and over again. Need another example?

<?php

class EmailAddress {
  private $emailAddress;
  
  public function __construct(string $emailAddress) {
    if (!filter_var($emailAddress, FILTER_VALIDATE_EMAIL)) {
      throw new InvalidArgumentException(sprintf('The e-mail address "%s" is invalid.', $emailAddress));
    }
    
    $this->emailAddress = $emailAddress;
  }
  
  public function getMailTo(string $subject, string $body): string {
    return sprintf('mailto:%s?%s', $this->emailAddress, http_build_query([
      'subject' => $subject,
      'body' => $body,
    ]));
  }
  
  public function getEmailAddress(): string {
    return $this->emailAddress;
  }
}

To answer your 2nd question:

Don't use exceptions for validation. The code should not be halted at that point, as you want to report back ALL errors at once to the user instead of 1-by-1.And use a format that is most easy to work with like an array to store the validation errors.

Also create dedicated validators for your entities. As you might need to validate your entity in more than 1 place. It can still extend from a base generic validator:

<?php

class LoginValidator extends Validator {
  public function validate(array $data) {
    return parent::validate($data, [
      'username',
      'password',
    ]);
  }
}

 

[maxxd]

21 hours ago, ignace said:

So to answer your question: yes, you must validate the data both in your model and in your form. The reasoning here is that your data might not always come from a form. But perhaps some data is input from a 3rd party API. Or from the CLI. Wherever the data comes from, you can be certain your model will always be valid.

To add to the point that @ignace is making here, it's also very simple to load a page containing a form, open the inspector for your browser and change any value for any form field. Client-side validation may not catch this, but the server-side processing (model) code validation will.

[Destramic]

I've been doing things wrong for years...I've had one model class per controller with multiple methods. Ie. Get tasks, get task etc

I really do like the structure I've been shown here and I'm excited to make the changes.  Also I will check that link out ignace.

Thank you all for your help and patience as It means a lot to me to know the correct way of doing things.

 

?