Notes on refactoring ZF2's "InputFilter"

1-Notes.md

A few notes from my personal gripes about what InputFilter is and does.

1. InputFilter is a terrible name as the class mixes sanitation and validation. Rename to something far more appropriate (e.g. InputManager?).
2. Data should only be processed, not stored - remove setData()

SoC Article

For example, if both raw and sanitized data are required:

$sanitizedData    = $this->inputManager->sanitize($data->toArray());
$validationResult = $this->inputManager->validate($sanitizedData);

if ($validationResult->isValid()) {
    // hooray!
} else {
    $messages = $validationResult->getMessages();
}

Also, if all we need is the result:

$validationResult = $this->inputManager->sanitizeAndValidate($data->toArray());

if ($validationResult->isValid()) {
    // hooray!
} else {
    $messages = $validationResult->getMessages();
}

CollectionInput.php

<?php

namespace Zend\Input;

use SplPriorityQueue;

class CollectionInput implements InputInterface, CollectionInterface
{
    /**
     * @var SplPriorityQueue
     */
    private $inputs;
    
    private $name;
    
    public function __construct($aName, array $anInputList = [])
    {
        $this->name   = (string) $aName;
        $this->inputs = new SplPriorityQueue();
        $this->insertAll($anInputList);
    }
    
    public function sanitize($aValue)
    {
        foreach ($this->inputs as $input) {
            $name = $input->getName();
            if (isset($aValue[$name])) {
                $aValue[$name] = $input->sanitize($aValue[$name]);
            }
        }
        return $aValue;
    }
    
    public function validate($aValue)
    {
        $results = [];
        foreach ($this->inputs as $input) {
            $name = $input->getName();
            if (isset($aValue[$name])) {
                $results[$name] = $input->validate($aValue[$name]);
            }
        }
        return $results;
    }
    
    public function sanitizeAndValidate($aValue)
    {
        return $this->validate($this->sanitize($aValue));
    }
    
    public function insert(InputInterface $anInput, $aPriority = 0)
    {
        $this->inputs->insert($anInput, $aPriority);
    }
    
    public function insertAll($anInputList)
    {
        foreach ($anInputList as $input) {
            $this->insert($input);
        }
    }
    
    // ...
    
    public function getIterator()
    {
        return $this->inputs;
    }
}

CollectionInterface.php

<?php

namespace Zend\Input;

interface CollectionInterface extends \InteratorAggregate
{
    public function insert(InputInterface $anInput);
    
    public function insertAll($inputs);
    
    public function remove(InputInterface $anInput);
    
    public function removeAll($inputs);
    
    public function contains(InputInterface $anInput);
    
    public function retrieve($anInputName);
}

Input.php

<?php

namespace Zend\Input;

use Zend\Sanitizer\SanitizerInterface;
use Zend\Validator\ValidatorInterface;

/**
 * An implementation of the {@link Zend\Input\InputInterface}
 *
 * @package Zend\Input
 */
class Input implements InputInterface
{
    /**
     * @var string
     */
    protected $name;
    
    /**
     * @var SanitizerInterface
     */
    protected $sanitizer;
    
    /**
     * @var ValidatorInterface
     */
    protected $validator;
    
    
    /**
     * @param string              $aName
     * @param SanitizerInterface  $aSanitizer
     * @param ValidatorInterface  $aValidator
     */
    public function __construct($aName, SanitizerInterface $aSanitizer, ValidatorInterface $aValidator)
    {
        $this->name      = (string) $aName;
        $this->sanitizer = $aSanitizer;
        $this->validator = $aValidator;
    }
    
    /**
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }
    
    /**
     * @param  mixed $aValue
     * @return mixed
     */
    public function sanitize($aValue)
    {
        return $this->sanitizer->sanitize($aValue);
    }
    
    /**
     * @param  mixed $aValue
     * @return \Zend\Validator\ResultInterface
     */
    public function validate($aValue)
    {
        return $this->validator->validate($aValue);
    }
    
    /**
     * @param mixed $aValue
     * @return \Zend\Validator\ResultInterface
     */
    public function sanitizeAndValidate($aValue)
    {
        return $this->validate($this->sanitize($aValue));
    }
}

InputInterface.php

<?php

namespace Zend\Input;

interface InputInterface
{
    public function getName();
    
    public function sanitize($aValue);
    
    public function validate($aValue);
    
    public function sanitizeAndValidate($aValue);
}

Manager.php

<?php

namespace Zend\Input;

/**
 * An implementation of the {@link Zend\Input\ManagerInterface}
 *
 * @package Zend\Input
 */
class Manager implements ManagerInterface
{
    /**
     * @param ContainerInterface $anInputList
     * @param array|\Traversable $aValidationGroup
     */
    public function __construct(CollectionInterface $anInputList, $aValidationGroup)
    {
        foreach ($anInputList as $name => $input) {
            if (!in_array($name, $aValidationGroup)) {
                $anInputList->remove($input);
            }
        }
        $this->insertAll($anInputList);
    }
    
    /**
     * @param  InputInterface[] $anInputList
     * @param  string[]         $aValidationGroup
     * @return self
     */
    public static function withInputs($anInputList, $aValidationGroup)
    {
        return new static(new Collection($anInputList), $aValidationGroup);
    }
    
    /**
     * @param  array
     * @return array
     */
    public function sanitize(array $data)
    {
        return $this->inputs->sanitize($data);
    }
    
    /**
     * @param  array $data
     * @return \Zend\Validator\ResultInterface[]
     */
    public function validate(array $data)
    {
        return $this->inputs->validate($data);
    }
    
    /**
     * @param  array
     * @return \Zend\Validator\ResultInterface[]
     */
    public function sanitizeAndValidate(array $data)
    {
        return $this->inputs->sanitizeAndValidate($data);
    }
}

ManagerInterface.php

<?php

namespace Zend\Input;

interface ManagerInterface
{
    public function sanitize(array $data);
    
    public function validate(array $data);
    
    public function sanitizeAndValidate(array $data);
}

Validator\ResultInterface.php

<?php

namespace Zend\Validator;

interface ResultInterface extends \Serializable, \JsonSerializable
{
    /**
     * Was the validation successful?
     * 
     * @return bool
     */
    public function isValid();
    
    /**
     * List any messages
     * 
     * @return string[]
     */
    public function getMessages();
}

Hmm. I may agree on the naming but InputFilter seems the best for me now (it's not because it does validation also that it should be named InputValidatorFilter or something like this).

$inputManager     = $serviceManager->get('My\InputManager');
$filteredData     = $inputManager->filter($data);
$validationResult = $inputManager->validate($filteredData);

// or, if the filtered data isn't needed
$validationResult = $inputManager->filterAndValidate($data);

if ($validationResult->isValid()) {
    // hooray!
} else {
    $messages = $validationResult->getMessages();
}

Currently, the same result is achieved like this:

$inputFilter->setData($data);

if ($inputFilter->isValid()) { // No filtering here
   $data = $inputFilter->getValues(); // Filtering here
}

Furthemore:

// or, if the filtered data isn't needed
$validationResult = $inputManager->filterAndValidate($data);

if ($validationResult->isValid()) {
    // hooray!
}

If I understand this correctly, you will filter data even if the result is not valid. This is useless and will consume time and memory for nothing.

I find your abstraction a bit more complicated to use. People may think that isValid does filtering but it does not. I don't think we need to introduce explicit "validate" and "filter". Validation is implicit when doing isValid, and filtering is implicit when doing getValues.

I don't understand your FilterCollectionInterface and ValidatorCollectionInterface. To me this is really similar to ValidatorChain and FilterChain. This would mean that your ValidatorCollection would consume a ValidatorChain ? Pretty complex for few benefits imho.

Your input interface seems definitely wrong to me. An InputFilter consumes validator chain and filter chain, I don't really get the point of an Input filter interface extending a ValidatorCollection or FilterCollection.

To be honest Input filter is to my opinion one of the best ZF 2 component. It's simple to use, powerful and do it right with abstraction, without being to complicated to use. My rewrite is mostly clean the code to remove edge cases support and use much more efficient data structures.

@bakura10

http://vimeo.com/12643301

http://www.martinfowler.com/eaaCatalog/separatedInterface.html

@bakura10

I've removed the Collections so that InputInterface is completely decoupled from both Zend\Filter and Zend\Validator. That's not to say that those components shouldn't be used. But, we should be free to filter and validate with any potential implementation. This greatly simplifies how a combined sanitation and validation component is built.

How is an input filtered? Maybe it's a FilterChain, but maybe it isn't. Don't paint the dev into a corner and force them to use something they may not want to.

Implicit design is bad. Always be explicit and don't leave the implementation open to assumptions, or rely on comments to document behavior.

What are filter, validate and filterAndValidate suppose to return? To my understanding of your examples, filter returns an array of values, filterAndValidate and validate returns a ValidationResult ?

How is an input filtered? Maybe it's a FilterChain, but maybe it isn't. Don't paint the dev into a corner and force them to use something they may not want to.

I'm not sure to understand this. How do you want to filter? A Filter chain does not force you anything. A Filter Chain is just a container for any filters. It can either be built-in filters, custom filters, callback…

I completely agree with you about implicit design, but it still has to be simple to use for the end-user. Introducing too much abstraction is bad for framework's adoption. I mean, can you show me a concrete use-case of something you'd like to do but cannot do with the current implementation ?

@bakura10

You are correct, filter returns filtered data, and validate and filterAndValidate return a ValidationResultInterface.

I want the developer to be free to choose how data is filtered and how data is validated, including using external libraries. If Zend\Filter\FilterChain or Zend\Validator\ValidatorChain are defined in the interface, it limits the implementation to only those classes.

I can't use this component without also using Zend\Filter and Zend\Validator. In order to make it easier to adopt ZF, every component should be decoupled to eliminate dependencies. ZF components should still be suggested, or recommended, obviously.

I may not agree on this one though. I agree that dependencies must be reduced, but input filter is validation and filtering, it is assumed to use Zend\Filter and Zend\Validator.

The problem is that if we remove this assumption, a lot of type hinting is lost (well, you won't be able to type hint ValidatorInterface or FilterInterface now, as you don't assume that either Zend\Filter nor Zend\Validator is used). This is to me even worse.

There will always be, at some point, some dependencies between components. This lead to better quality code. Otherwise, you remove all dependencies. For instance, will you remove Zend\Validator dependency from Zend\I18n, just because validators defined in Zend\I18n implements Zend\Validator\ValidatorInterface ?

Yes, I would remove Zend\Validator from Zend\I18n. Using the two components together should require a bridge pattern. Doing so aids reuse. Dependencies reduce code quality as they force a particular implementation. One of the strengths of ZF is it's intended flexibility. If we wanted something opinionated, why not just use RoR?

Yes yes, all of this is nice. I know. But please, show me with interfaces how you deal with validators here? And I want to keep the type hinting to ValidatorInterface somewhere (of course).

I've been thinking about that. While InputFilter is a terrible name, perhaps it's best for a backwards-compatible bridge component. Maybe move it over to ZF-Common? Also, the InputManager name is starting to lose favor with me as a manager in ZF is still more of a registry with the get($foo) method. Obviously, I'm very particular about names! :)

<?php

namespace Zend\Filter;

interface FilterChainAwareInterface
{
    public function setFilterChain(FilterChain $filterChain);

    public function getFilterChain();
}

// ...

namespace Zend\Validator;

interface ValidatorChainAwareInterface
{
    public function setValidatorChain(ValidatorChain $validatorChain);

    public function getValidatorChain();
}

// ...

namespace ZfcInput;

use Zend\InputManager\InputInterface as ManagedInputInterface;
use Zend\Filter\FilterChainAwareInterface;
use Zend\Validator\ValidatorChainAwareInterface;

interface InputInterface extends
    ManagedInputInterface,
    FilterChainAwareInterface,
    ValidatorChainAwareInterface
{
    // ...
}

Yes, I'm very on names too. Concerning moving it to ZF-Commons, no way, Input filter is one of the most basic component and is used by various other components. This is to me a vital component, should be in core.

For InputManager, I want to homogeneize eveyrhting to using the "PluginManager" suffix. So HydratorPluginManager, InputFilterPluginManager, ControllerPluginManager… Currently the naming is not coherent (for instance the plugin manager for controllers is called ControllerLoaderManager iirc).

I'm not sure to completely grasp what you are looking for. This makes it much more complicated. If I understand correctly, you'd like the core to have only interfaces and having another library that will be aware of validation and filter ? I'm against it. Really, while modules are a great features, they SHOULD NOT be used at the expense of simplicity. If someone need to install 20 modules just to have basic functionality like input filtering, that's a no-go. Please consider that 90% of users of ZF 2 are not power-users, yet they are the ones that make ZF2 live and be a success (or not).

Going too convoluted is something really I don't want.

Anyway, I think we should speak on the PR please, I'm pretty sure most people won't get there.

Do you understand the concept of Separation of Concerns? That's the key objective here. Bridge components would be better organized outside of the core library. A Zfc version would be no different than the current version in terms of requirements. It's not complex: If you need a pre-built input module, just use the Zfc version; if you need more flexibility, use the individual components as necessary and roll your own. But, if you need flexibility, you should NOT be forced to load "20 modules" (a gross exaggeration) just for one component.

    "require": {
        "php": ">=5.3.3",
        "zendframework/zend-filter": "self.version",
        "zendframework/zend-validator": "self.version",
        "zendframework/zend-stdlib": "self.version"
    },
    "suggest": {
        "zendframework/zend-servicemanager": "To support plugin manager support"
    },

would become

    "require": {
        "php": ">=5.3.3",
        "zendframework/zend-stdlib": "self.version"
    },
    "suggest": {
        "zendframework/zend-servicemanager": "To support plugin manager support",
        "zendframework/zend-filter": "For value filtering and sanitation",
        "zendframework/zend-validator": "For value validation and errors"
    },

Just some quick notes:

1. I'd get rid of the traits while prototyping. Traits are useful only once you're having lots of duplicate code, and in this case we won't
2. I'd still keep an Input being a Validator + Filter (both optional)
3. An InputFilter should be renamed to InputCollection, and should simply act as an Input on any array|Traversable. That naming also clears up its role.
4. The fact that an Input may be required or not could probably land in the InputCollection, since the input itself doesn't deal with cases where no value at all is passed in.
5. I'd leave validation groups out while prototyping, they really are operations on the collection.
6. +1 for the ValidationResult. That can encapsulate a lot of logic that is currently mixed up with inputs/validators/filters and may as well handle translations or such. It should probably be a FilterResult though, since it doesn't just include validation results, but also original and filtered data. It also allows persistence of validation results.
7. +1 on making Input stateless
8. filterAndValidate is a code smell (just a note: there's an And in your method name, nuff said). Instead, some way of deciding whether filtering or validation goes first should be defined. That's an implementation detail of the filter.

I've already renamed the InputFilter to InputCollection (you can check it here: zendframework/zendframework#4772 (comment)).

Regarding Input stateless, my question is: how can you retrieve the raw value and filtered value if you never store it?

In my refactor, it uses RecursiveIteratorIterator: https://github.com/bakura10/zf2/blob/902416598924a1749096d2026e6e8c02e01c3c4e/library/Zend/InputFilter/InputCollection.php#L317-L336

What we get is only Input, not input collection, therefore the logic of required/not required is handled by the input, not input collection.

Regarding ValidationResult, I'll try to integrate it.

The ValidationResult thing is really not easy at all, because of the recursiveiteratoriterator thing….

@texdc: what happen in case of InputCollection embedded another InputCollection, regarding the ValidationResult ? The problem is that with this new implementation using RecursiveIteratorIterator, what I get are only Input (only leaves). It makes the validation result stuff extremely complicated, and I would need to revert to the old way (ZF2) which is at least 10 times slower.

EDIT : what I don't understand too is that from your code, the "Input" class returns too a ValidationResult. So what a InputCollection / InputFilter would return ? An array of validation result? And what about an input collection containing input collection containing input ? Would it returns nested validation results too? I can't get it.