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();
}

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.