Add class OptionalInputFilter

[Erikvv]

This inputfilter allows you to validate data where a (nested) array is optional, but if it is present it should validate the (nested) data like the standard inputfilter would

This is currently not possible.

This is analog to a Zend\InputFilter\Input with the option ->setRequired(false)

Example use case:

$valid = [
    'car' => [
    'brand' => 'Volkswagen',
    'model' => 'Golf',
    ]
];

$valid = [
    'car' => null
];

$invalid = [
    'car' => [
        'brand' => 'Volkswagen',
    ],
];

I found it works best when OptionalInputFilter::getValues() returns null so no hydrator can iterate over the result.

[Erikvv]

Would you prefer ternaries or explicit if-else?

[froschdesign]

@Erikvv

This inputfilter allows you to validate data where a (nested) array is optional…

Okay, now you can test if an array is optional. But what is with the opposite, an array is required?

Is the new OptionalInputFilter really needed or can we improve the CollectionInputFilter?

[Erikvv]

@froschdesign

Okay, now you can test if an array is optional. But what is with the opposite, an array is required?

By array I mean hashmap in this case. Being required is the behaviour of the current standard InputFilter. So you would use that. Does that answer your question?

Is the new OptionalInputFilter really needed or can we improve the CollectionInputFilter?

They are different things. The nesting structure of the data is different:

A CollectionInputFilter can validate an array of cars like so:

$data = [
    'cars' => [
        [
            'brand' => 'Volkswagen'
            'model' => 'Passat'
        ], [
            'brand' => 'Volvo'
            'model' => 'Grand Luxe'
        ]
        (...)
    ]
]

An OptionalInputfilter (or a normal InputFilter for that matter) can validate a car like so:

$data = [
    'car' => [
        'brand' => 'Volkswagen'
        'model' => 'Passat'
    ]
]

I view this as the structure of this library:

Class	Analog in the type system
Input	Scalar or null *
ArrayInput	Array of scalars **
InputFilter	Object
OptionalInputFilter	Object or null
CollectionInputFilter	Array of objects ***

* if null is allowed depends if required is set to true or false ** This class needs some work to be useable. For example the CollectionInputFilter you can give an InputFilter to which it uses to process the members, the ArrayInput you should be able to give it an Input to process the members but that's not how it is currently implemented *** You could do CollectionInputFilter->setInputFilter(new OptionalInputFilter) if you want to validate an array of Object|null

OptionalInputFilter fills a gap which is currently not serviced.

After this change I believe in the abstract all the possible data can be validated without the user writing his own Input or InputFilter classes (obviously under the condition that the data is a DAG with a 'static' structure and no interdependence between fields)

If it is a priority to preserve symmetry between Input and InputFilter you would either (1) add ->setRequired to InputFilter or (2) remove ->setRequired from Input and also create a class OptionalInput

I think (2) would be better but this would be a breaking change. For 3.0 perhaps? (You would still be able to implement the current dynamic behavior on top of it by using inheritance or composition).

IMHO (1) is less desirable. The focus for this library and zendframework in general is not to add more state and flags to existing classes. We have seen in the past that it leads to large, inflexible classes and a bigger chance of edge cases.

You would end up with these 6 main classes:

Input Filter	Analog in the type system
Input	Scalar
OptionalInput	Scalar or null
ArrayInput	Array of scalars
InputFilter	Object
OptionalInputFilter	Object or null
CollectionInputFilter	Array of objects

[Erikvv]

I just noticed I still need to add as an alias to InputFilterManager

[froschdesign]

@Erikvv Thanks for your further explanation! 👍

By array I mean hashmap in this case.

Good to know, because I still fight with name and the usage of your InputFilter.

Your InputFilter makes some return values of getValues optional. Without it must look like this:

public function testNestedInpuFilterWithoutData()
{
    $childInputFilter = new InputFilter();
    $childInputFilter->add((new Input('brand'))->setRequired(false));
    $childInputFilter->add((new Input('model'))->setRequired(false));

    $inputFilter = (new InputFilter())->add($childInputFilter, 'car');

    $data = [
        'car' => [], // needs an array or Traversable
    ];
    $inputFilter->setData($data);

    $this->assertTrue($inputFilter->isValid());
    $this->assertEquals(
        [
            'car' => [
                'brand' => null,
                'model' => null,
            ],
        ],
        $inputFilter->getValues()
    );
}

The keys "brand" and "model" are present in the output of getValues.

Your OptionalInputFilter will change this behaviour. Right?

[Erikvv]

You are correct. This behavior will change. getValues will return null.

Initially I returned an empty array, to keep the interface the same. But I ran into the issue that it is then valid to pass it to a hydrator and it will fail later.

[froschdesign]

getValues will return null.

…and also no fallback values!

[froschdesign]

@weierophinney This InputFilter produces a complete different output for the getValues method and doesn't respect the fallback values of the included inputs. If we fine with this, then we need an explicit documentation for this difference!

[svycka]

looks like good feature, but I don't like one thing

// copy-paste from tests
$data = [];
$inputFilter->setData($data);
$this->assertTrue($inputFilter->isValid());
$this->assertEquals(['car' => null,], $inputFilter->getValues()); // why we need 'car' in values?

I understand this is the same as ->setRequired(false) as an optional value I would like it to be optional not a null

[froschdesign]

@svycka

as an optional value I would like it to be optional

This is the question:

1. optional for setData means also optional for getValues

or

2. optional for setData means included for getValues (current behaviour)

[svycka]

@froschdesign I would want that if not set in setData then also no value in getValues. The problem is that sometimes null is a value and this would just allow it together with values like 0, '', false maybe this is not a big problem in this case when we accept array or null but still.. When I get valeus from inputFilter I still have to check if this is not set null or this is provided null value. This would definitely hurt if we decide to introduce OptionalInput alongside with OptinalInputFilter.

Maybe if it is not possible here we can add some kind of option to InputFIlter to show values or not if not defined in setData() this would be really useful wen an api does not require all fields lts say when you update first name you have to send and last name. And yes you could add the additional validation like if second name is null do not update it but why if we could get only first name from inputFilter it would be perfect.

[froschdesign]

@svycka

OptionalInput

This makes no sense. You can not add to „Inputs“ with the same name. (The new input merges into the original.) You can already set the input as optional (required is false). See in my example test above.

The OptionalInputFilter from @Erikvv is good. Maybe an rework of the getValues would help to respect fallback values. If an input returns no content (null), then the getValues method of the OptionalInputFilter ignores the input.

[svycka]

@froschdesign maybe you did not understood me I will try to explain with example:

$inputFilter = new InputFilter();
$inputFilter->add(new Input('userId'));
$inputFilter->add(new OptionalInput('FirstName'));
$inputFilter->add(new OptionalInput('LastName'));

$optionalInputFilter = new OptionalInputFilter;
$optionalInputFilter->add(new Input('city'));
$optionalInputFilter->add(new Input('street'));

$inputFilter->add($optionalInputFilter, 'address');

$inputFilter->setData($data);
$inputFilter->isValid();
$values = $inputFilter->getValues();

// using this code  I would expect those values from inputFilter:
//with $data:
[
    'userId' => 1,
    'firstName' => 'given name'
];
// $values would be:
[
    'userId' => 1,
    'firstName' => 'given name'
];
// when current behavior returns $values like this:
[
    'userId' => 1,
    'firstName' => 'given name',
    'lastName' => null,
    'address' => null,
];

and I have a problem because I would like to use this directly with hydrator, something like this:

$user = new User(1, 'given name', 'family name', new Address('city', 'street'));
$hydrator = new Hydrator();
$user = $hydrator->hydrate($user, $values);

hydrator only hydrates what is in $values so current behavior resets address and lastName to null, but if those optional fields would be missing it would only update firstName so with current behavior I have to prepare $values before using with hydrator. something like:

$user = new User(1, 'given name', 'family name', new Address('city', 'street'));
$hydrator = new Hydrator();
$values = array_filter($values, function($k) {
    return $k !== null; // would still don't know what to do if API allows null as value(for example to remove address)
});
$user = $hydrator->hydrate($user, $values);

In some cases I did something like this:

$userData = $hydrator->extract($user);
$data = array_merge($userData, $data);
$inputFilter->setData($data);
//...

and if possible to have original $data and filtered $values I could go like this:

$user = new User(1, 'given name', 'family name', new Address('city', 'street'));
$hydrator = new Hydrator();
$values = array_intersect_key($values, $data);
$user = $hydrator->hydrate($user, $values);

to support updating only for few fields but that's a bit annoying :dagger: but those are just workarounds and i would like to avoid all this

[Erikvv]

@svyka can't it be done using validationgroups?

[Erikvv]

Using a FilterIterator bakura10 was working on maybe

https://github.com/bakura10/zf2/tree/d0cfc2e6643732b1e5e020f6154a6617e7e4d20d/library/Zend/InputFilter/ValidationGroup

[svycka]

@Erikvv no validation groups is different thing if I would set them it would be not possible validate anything else what is not in validation group okay maybe in some cases you could do something like:

$inputFitler->setValidationGroup(array_keys($data));
$inputFilter->setData($data);
$inputFilter->isValid();
$values = $inputFilter->getValues();

but that's not a good solution and leads only to bugs and other problems

[weierophinney]

It's not a fad.

If you're returning from within the then block, there is no need for an else; that becomes the primary execution flow.

Extra indentation has been proven to add cognitive overhead, so removing indentation aids in readability and comprehension.

On Jun 29, 2017 6:27 PM, "Erik van Velzen" notifications@github.com wrote:

@Erikvv commented on this pull request.

In src/OptionalInputFilter.php https://github.com/zendframework/zend-inputfilter/pull/135#discussion_r124935302 :

• • else it reports valid
• *
• • This is analog to a Zend\InputFilter\Input with the option ->setRequired(false)
• */ +class OptionalInputFilter extends InputFilter +{
• public function setData($data)
• {
• return parent::setData($data ?: []);
• }
• public function isValid($context = null)
• {
• if ($this->data) {
• return parent::isValid($context);
• } else {

I don't agree with the fad to always omit else when possible. I only do it for argument checking or short-circuiting.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-inputfilter/pull/135#discussion_r124935302, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlV4fufdv3px9Fs4bOm2c03WD_ax2Sks5sJDLPgaJpZM4MzE5x .

[Erikvv]

I've adjusted it

[weierophinney]

I've added a commit with documentation, and will now merge. Thanks, @Erikvv .

[Erikvv]

Thanks for picking up my slack

[weierophinney]

My pleasure - thanks for the new feature!