-
Notifications
You must be signed in to change notification settings - Fork 50
Required does not work, when array is empty #167
Conversation
The first problem is, that hasValue always returns true, when a value is set, even, if the value is an empty array. So the first validation for the required filter will be skipped. The second problem is, that the isRequired is called within the foreach loop. But if the value is an empty array, the loop will never start, so isRequired will never triggered and the method will return the defaultValue true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eweso We need test case for that change.
@webimpress Thank you for your feedback. I was on holiday until now. The point is, that I am really new to github and don't know, where the test files are and where I have to create one, to pass the checks. If someone could help me a little, we could fix that bug quickly. |
To pass test "continuous-integration/travis-ci/pr"
@eweso The repository contains information on contributing, including how to run tests: https://github.com/zendframework/zend-inputfilter/blob/master/docs/CONTRIBUTING.md If you need help writing tests, jump on the Slack and join the #contributors channel and start asking. |
@weierophinney Thank you for the information! I will create the test class within the next two weeks and will pull the request again. |
Okay, I still do not get, why the first test failed. There was already a test class in this repository. The basic behaviour hasn't changed. When I open the Details of coverage/coveralls it does not give me any information about missing test classes. It just informs me, that some lines of codes have changed in the base repository and that Module.php ist missing. So am I right, that the coverage/coveralls does not fail because of the test, it fails because some lines of code were changed in the base branch, to which I wanted to pull my request? If so, I think, I just have to update those files in my patch and the test will be successful? |
This is a different case. I fixed a bug in the ArrayInput. You are talking about the InpuFilter. When you have a Multi-Select for example and you use an ArrayInput with required in your InputFilter to validate the Multi-Select and there is no value selected, the ArrayInput returns at the moment true, because it does not handle the empty array as an empty value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've validated that the test fails without the change made, and the change makes sense.
Required does not work, when array is empty
- Renames `testRequiredWithoutFallbackAndValueIsEmptyArrayThenFail` to `testArrayInputMarkedRequiredWithoutAFallbackFailsValidationForEmptyArrays`. - Adds `testArrayInputMarkedRequiredWithoutAFallbackUsesProvidedErrorMessageOnFailureDueToEmptyArray` to validate that if an error message is set on the input, that value is used instead of the defaults.
Forward port #167 Conflicts: CHANGELOG.md
Thanks, @eweso! |
The first problem is, that hasValue always returns true, when a value is set, even, if the value is an empty array. So the first validation for the required filter will be skipped. The second problem is, that the isRequired is called within the foreach loop. But if the value is an empty array, the loop will never start, so isRequired will never triggered and the method will return the defaultValue true.