5
\$\begingroup\$

I've made a (service) class to handle all form validation of my application. It contains a set of frequently used validation callables extracted into private methods that I use to build each is[FormName]Valid(); method. It would probably end up containing validation logic for about 5 or 6 forms of which it contains 2 already. So the class could become relatively big; roughly about 230 lines.

It works completely fine and was the best solution I could think of. But since this would pretty much be considered a god object, at some point I would want to adapt to a better pattern. I've got some clues that my class might be somewhat extraordinary, because I haven't seen anyone else do it like this before.

Are there any better / more efficient solutions?

namespace Model\Application;

use Library\Validator;
use Model\Infrastructure\Mapper\UserMapper;

class FormValidationService
{
    private $validator;
    private $userMapper;
    private $errors;

    public function __construct(Validator $validator, UserMapper $userMapper)
    {
        $this->validator  = $validator;
        $this->userMapper = $userMapper;
    }

    public function isLoginValid($email, $password)
    {
        $this->validator->reset();

        $this->validator->isValid('email', [
            $this->notEmpty($email),
            $this->email($email),
        ]);

        $this->validator->isValid('password', [
            $this->notEmpty($password),
            $this->minChars($password, 6)
        ]);

        return !$this->errors['Login'] = $this->validator->findErrors();
    }

    public function isSignUpValid($firstName, $lastName, $email, $confirmationEmail, $password, $terms)
    {
        $this->validator->reset();

        $this->validator->isValid('firstName', [
            $this->notEmpty($firstName),
            $this->maxChars($firstName, 35)
        ]);

        $this->validator->isValid('lastName', [
            $this->notEmpty($lastName),
            $this->maxChars($lastName, 35)
        ]);

        $this->validator->isValid('email', [
            $this->notEmpty($email),
            $this->email($email),
            $this->uniqueUserEmail($email)
        ]);

        $this->validator->isValid('confirmationEmail', [
            $this->notEmpty($confirmationEmail),
            $this->email($confirmationEmail),
            $this->match($confirmationEmail, $email)
        ]);

        $this->validator->isValid('password', [
            $this->notEmpty($password),
            $this->minChars($password, 6)
        ]);

        $this->validator->isValid('terms', [
            $this->notEmpty($terms)
        ]);

        return !$this->errors['SignUp'] = $this->validator->findErrors();
    }

    public function findErrorsOf($formName)
    {
        return isset($this->errors[$formName]) ? $this->errors[$formName] : [];
    }

    private function notEmpty($input)
    {
        return (function () use ($input) {
            return !empty($input) ?: 'NotEmpty';
        });
    }

    private function minChars($input, $min)
    {
        return (function () use ($input, $min) {
            return mb_strlen($input, mb_detect_encoding($input)) >= $min ?: 'MinChars';
        });
    }

    private function maxChars($input, $max)
    {
        return (function () use ($input, $max) {
            return mb_strlen($input, mb_detect_encoding($input)) <= $max ?: 'MaxChars';
        });
    }

    private function email($input)
    {
        return (function () use ($input) {
            $sanitized = filter_var($input, FILTER_SANITIZE_EMAIL);
            $isValid   = filter_var($sanitized, FILTER_VALIDATE_EMAIL);
            return $input === $sanitized && $isValid ?: 'Email';                    
        });
    }

    private function match($input1, $input2)
    {
        return (function () use ($input1, $input2) {
            return $input1 === $input2 ?: 'Match';
        });
    }

    private function uniqueUserEmail($input)
    {
        return (function () use ($input) {
            return !$this->userMapper->findByEmail($input) ? true : 'Unique';
        });
    }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Are you sure it's a form?

Everytime I see this 'form-validation' code I shrug. A form shouldn't be validated. Maybe on client side for better UX. But not on serverside.

Now before you burn with those torches and send me to hell, let me explain:

A form is client-side input mechanism that helps the user to interact with your application. Instead of having ot manually perform POST/PUT/DELETE requests, we give him some nice UX sugar. For instance a html <form> element.

The form is however not the only way to interact with the application. If it is a GET form we can easily perform that in the browser with [email protected]. l33t hack0rs will even be able to do the same for POST/PUT/... requests.

So yes we need validation. But not form validation.

Validate where you should

Most application these days have some sort of Router that dispatches a Request to a Controller. For instance a Authentication@logMeIn Controller/method. The controller will try and authenticate a user given the credentials provided.

If no credentials are send with the request, then the controller has no idea what to do. Otherwise he does: * Ask the Authentication service if the credentials are correct * authenticate the user + return success || return error

All validation is done by the Authentication service. That Authentication service will have an instance of a Validator that it will use. Something like this:

function areTheCredentialsCorrect($username, $password)
{
    if ( !$this->validator->isEmail($username) ) {
        throw new InvalidArgumentException('I need an email!');
    }

    $user = $this->repository->getUser('email','=',$username);

    return password_verify($user->password, $password);
}

The authentication service validates or the username is of the correct format. Here we need an email. We could even add exra logic sugar for 2 cases. If $username is an email -> get user by email otherwise get it by 'oldskool_username'.

Same goes for creation of a user. Our repository that handles the creation of users will check or the passed in arguments are correct:

function createUser($name, $email, $password)
{
    if ( !$this->validator->isAlpha($name) ) {
        //we only allow alpha-characters. We don't like the O'lastname guys
        throw new InvalidArgumentException('No scottisch people allowed!');
    }

    if ( $this->valdiator->isEmail($email) ) {
        throw new InvalidArgumentException('I need an email!');
    }

    if ( !$this->validator->isPassword($password) ) {
        throw new InvalidArgumentException('Strong passwords only please');
    }

    $this->database->create($name, $email, $password);
}

This not only gives us cleaner code. It also decouples the logic used in your html form (name stuff) from the validator. A validator shouldn't care how your forms are named/build. It should only care about validating data that is inserted into it. Doesn't matter from where it comes.

So be gone with your FormValidator. Simply let the code that handles the data itself perform validation. this way you have less security bugs of the kind where you forgot to validate one little input that then is directly printed into your sql. Don't give bobby tables a chance ;)

EDIT: Different meanings of a Form are used here. Having methods like isSignUpValid and isLoginValid means you will eventually have a method for every possible action with input. If your app get's bigger you will have tons of methods. and everytime you add a form you will need a different method, copy-pasting most of the inner workings.

As Elias correctly points out, once the data is put into your Models the valiation should have happened. But not in some kind of all knowing FormValidationService.

And yes, you will probably have some generic testing:

  • is the specified REQUEST_METHOD valid?
  • can I answer in the correct ACCEPT language (text/html, JSON, ...)
  • SRP
  • spam blocking
  • ...

Let the more specific validation checks happen in the controller:

  • is the email unique
  • is the password valid
  • is the firstName of an accepted length
\$\endgroup\$
8
  • 1
    \$\begingroup\$ Couldn't disagree more with you on "forms don't need validation". Yes, I read your reasons, but a form, server-side, represents the cluster of data that the user sent. This cluster of data needs to be treated as suspicious, and needs to be validated as what it is: a series of strings that should match certain criteria. Most applications indeed have a dispatcher and/or router. They often have a form component, that allows you to do something like: $form->processRequest($request); and $form->isValid(). That's when and where you validate \$\endgroup\$ Sep 2, 2014 at 14:17
  • 1
    \$\begingroup\$ By the time the form data has been put into data models, you should've made sure that: all of the required fields were filled in, fields representing dates contain valid formats, emails are indeed emails and if applicable: password & repeat password values do indeed match. If you don't treat the data as a form, you might make a mistake, and forget to cross check the data for internal mismatches, or get the DB connection involved too soon, which you really shouldn't do... I do agree that not all validation belongs in the form, but the form decides over its own validity \$\endgroup\$ Sep 2, 2014 at 14:20
  • 1
    \$\begingroup\$ I aggree with you if with 'form' you mean data that is sent to the client. But an all knowing FormValidator that knows about every data that can be sent to the controller. That is bad. And that is what I was trying to point out. A $form->isValid(); however is something that I hav enever encountered before. Creating a validator, addins constraints. and then passing in a ParameterBag (call it Form), yes that's okay. \$\endgroup\$
    – Pinoniq
    Sep 2, 2014 at 14:25
  • \$\begingroup\$ Symfony2 forms have this, as did the old (in many ways hellish) Zend_Form instances of old. The point is that, like I said: not all validation (ie: if a user exists and such) belong in the form. What should be the forms job is to make sure all required fields were set. If not, they can simply add an error to themselves, return false on the isValid call, and be sent straight to the view. All of the users' initial input will be there, along side with the error messages, showing them what fields they missed, or what data was wrong. That's the point of form validation \$\endgroup\$ Sep 2, 2014 at 14:28
  • 2
    \$\begingroup\$ This is somewhat how initially had it first. Why should RegistrationService->registerUser() and similar scenarios have any knowledge about a checkbox that's checked or not, or if 2 email fields match? Imo, you should only pass through what it needs. I figured I was violating SRP so I separated it. Not to mention I dislike using exceptions for non-exceptional errors, but you knew that already. :P \$\endgroup\$ Sep 2, 2014 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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