zendframework / zend-inputfilter

InputFilter component from Zend Framework
BSD 3-Clause "New" or "Revised" License
64 stars 50 forks source link

Fix loop validation input context is mutable #24

Closed Maks3w closed 9 years ago

Maks3w commented 9 years ago

Given two input fields the context used in each one will be different. https://github.com/zendframework/zend-inputfilter/pull/24/files#diff-2cd0c1ba642dc3e6de9ca4e2af0f5e71R290

This means inputs need to be order sensitive

$inputs = ['A', 'B'];
$data = [ /* empty */ ];

$inputA->isValid($context = ['A' => null]);
$inputB->isValid($context = ['A' => null, 'B' => null]);
// Inverse order will change $context too.
$inputs = ['B', 'A'];
$data = [ /* empty */ ];

$inputB->isValid($context = ['B' => null]);
$inputA->isValid($context = ['B' => null, 'A' => null]);

This PR provides fix and enhance this feature by building a default input context using the default input values when no custom context is provided.

Fix #16

Supersede and close #30

Maks3w commented 9 years ago

Note tests are not failling just the coverage due the different weights given to each line after removal.

svycka commented 9 years ago

looks good also have to note that this adds something new

$inputs = ['A', 'B'];
$data = ['C' => 'something'];

In context now we have:

['A' => null, 'B' => null, 'C' => 'something']

Before 'C' => 'something' would not be added to context. But I think this is not a bug but good feature. Maybe add test to keep it :)

Maks3w commented 9 years ago

Yep I opt for use default input raw value for build the context (usually null, sometimes mixed). Test it's placed for this enhancement.

weierophinney commented 9 years ago

As I noted on #25, I'm a bit worried that changing test assumptions is indicative of a BC break; also, like #25, the logic looks reasonable. I'll do a more thorough review early next week.

weierophinney commented 9 years ago

@Maks3w Can you rebase this, please? When I merged, there was a conflict, and my attempts to resolve the conflict have left me with failing tests.

Maks3w commented 9 years ago

Since #25 optional fields is not call what was expected. I've to rewrite the test

Maks3w commented 9 years ago

Done. Now includes changes from #30