Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Refactoring and bugfixes for InputFilter component #7056

Closed
wants to merge 13 commits into from
Closed

Refactoring and bugfixes for InputFilter component #7056

wants to merge 13 commits into from

Conversation

hschletz
Copy link
Contributor

This is a refactor of the InputFilter component that replaces PR #6784. It is based on the same fixes, but addresses all the issues discussed there, plus some more.

Validation (including evaluation of emptiness-related options) has been moved entirely from BaseInputFilter::validateInputs() to the Input classes' isValid() method. This has several advantages:

  • Code is much simpler.
  • Input classes are more self-contained. Special semantics (like FileInput with its reversed order of filtering and validation) are handled by the Input classes themselves without BaseInputFilter having to know about it. This would facilitate extending functionality by enabling Input classes with their own semantics regarding filtering, validation and definition of "empty". In fact, BaseInputFilter only iterates over the attached Input objects and calls their isValid() method - the implementation and interpretation of options are up the Input class.
  • It's a requirement for fixing InputFilter: allow_empty should check filtered input #6668.

When checking for empty input, each Input class implements its own definition for empty values:

  • Input: NULL, '', array()
  • ArrayInput: same as Input, applied to each element
  • FileInput: Non-array values, file arrays with UPLOAD_ERR_NO_FILE

I added a lot of tests. Most tests simply enhance the existing test suite to ensure BC and will also pass with unpatched code. There are some exceptions:

  • FileInputTest::testIsEmpty*() test a newly introduced auxiliary method.
  • BaseInputFilterTest::testAllowEmptyTestsFilteredValueAndOverrulesValidatorChain() covers the scenario affected by InputFilter: allow_empty should check filtered input #6668.
  • InputTest::testValidatorInvokedIfValueIsEmptyAndAllowedAndContinue(), inherited by ArrayInputTest, revealed another bug that I fixed: ArrayInput injected the NotEmpty validator unconditionally, disregarding the continueIfEmpty() option. This bug could also be fixed separately.
  • InputTest::testValidatorSkippedIfValueIsEmptyAndAllowedAndNotContinue() and InputTest::testAllowEmptyOptionSet() cover new functionality of the Input classes that was previously handled by BaseInputFilter.

The last one could be interpreted as BC break. It only affects code that uses an Input class directly, either by calling its isValid() method or by implementing one. Code that calls Input classes indirectly through the InputFilter class (the standard use case) is not affected.

I tested the code against an application that contains explicit workarounds for #6668 without regressions.

@weierophinney
Copy link
Member

@Ocramius What is the exact BC break you're seeing here? Considering this adds tests but doesn't alter existing tests, and that all input filter and form tests continue to pass, I'm not sure if we can say there's breaking changes. The primary change is that the logic for validation is pushed from the input filter to the individual inputs, which might be a behavior change, but it also corrects for what could be some unexpected behavior in the current implementation.

Scheduling for 2.4; I'll remove that if you can illuminate the breakage you foresee, though.

@weierophinney weierophinney added this to the 2.4.0 milestone Feb 19, 2015
@hschletz
Copy link
Contributor Author

Code that uses InputFilter as documented should not see any behavioral changes except for the 2 fixed bugs. As you said, the change is in the implementation, and code that directly interacts with BaseInputFilter::validateInputs() or one of the Input classes and their isValid() method would need to be adapted. However, that code had not been designed to be extensible in the first place, and trying to extend it would be very hack-ish.

I think that regular code should not run into trouble, but the shifted responsibility should be mentioned in the release notes.

weierophinney added a commit that referenced this pull request Feb 25, 2015
Refactoring and bugfixes for InputFilter component

Conflicts:
	tests/ZendTest/InputFilter/BaseInputFilterTest.php
weierophinney added a commit that referenced this pull request Feb 25, 2015
@weierophinney
Copy link
Member

Merged to develop for release with 2.4.

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
…utfilter-refactor

Refactoring and bugfixes for InputFilter component

Conflicts:
	tests/ZendTest/InputFilter/BaseInputFilterTest.php
weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants