Add element input filters before form input filters #5479
Conversation
Input filters returned by the element getInputSpecification() are added to the chains before the form input filters returned by getInputFilterSpecification(). Also, it creates the replace() method in BaseInputFilter, that allows the filters to be replaced without having to remove them first. As the filters must be merged, preferFormInputFilter must be set to false on the forms.
|
Thanks @saltoledo can you add; above here; https://github.com/zendframework/zf2/blob/master/library/Zend/Form/Form.php#L646 And check all tests still pass. |
|
@davidwindell, testWillUseFormInputFilterOverrideOverInputSpecificationFromElement() fails. |
|
@saltoledo can you change the order of the line below in that test and see if that fixes it? I don't think anyone will have an issue with that. You can then remove the call to setPreferFormInputFilter(false) in my test making everything work out. |
| * Replace a named input | ||
| * | ||
| * @param string $name | ||
| * @return InputFilterInterface |
There was a problem hiding this comment.
I just copied the comments from another method, the other methods in this class are using "@return InputFilterInterface". Anyway, this DocBlock was incomplete, I made it a bit more complete.
|
@davidwindell, these changes make everything work out, but I think that the preferFormInputFilter flag shouldn't be changed to false without user knowledge. From 2.2.4 its default value is true, and changing it on setInputFilter() call could lead to an unsteady behaviour. As in the test that was breaking, if the user sets preferFormInputFilter to true before setting the inputFilter, or even if this user relies on the default setting as being true, he expects that the provided inputFilter will override the default input specs of the form elements. On the other hand, you want to merge element specs with the input filter that you are providing, so you are expecting the behaviour of preferFormInputFilter set to false. In our application we have many custom form elements that implement InputProviderInterface, so since 2.2.4 update we simply extended the Form class and set preferFormInputFilter to false. |
|
for the docblock should be see #4436 (comment) |
|
@samsonasik, thanks for pointing out. I'll update the other docbloks in the committed files, is it ok? |
|
@saltoledo why not set the default value of preferforminputfilter to null, then we can check if a user has set it explicitly or not. I think that anyone who directly calls 'setInputFilter' is going to expect their input filter to take priority over the forms. |
|
@saltoledo you should only update the related changes to your PR, no need to update other to ease review. |
|
@davidwindell I completely agree with you, the default value of preferFormInputFilter should be false, it is more intuitive. Would it not be better to open a new PR for it? See weierophinney@ce07861. @samsonasik thanks for your time, I'll update it right now. |
|
@saltoledo I agree too in our context however I can't see them wanting to change that again. Here's what I'd do;
That should resolve all the issues and mean no changes to that test. |
|
@davidwindell I think it's cleaner to use a $hasChangedPreferFormInputFilter and set it to true if setPreferFormInputFilter method is called. Then just check for it in setInputFilter and set preferFormInputFilter accordingly. This way testPreferFormInputFilterFlagIsEnabledByDefault won't break. But I'm kinda concerned about whenever someone calls setInputFilter he expects that element specs will be overridden if he isn't explicitly set preferFormInputFilter to its default value. Since 2.2.3, the forms in my application are breaking on every ZF release. Sometimes a change like this may cause more BC breaks to other people. Maybe the solution is turning preferFormInputFilter default to false again, i.e., if I use an email element, I want to use its defaults properties, including validators, so I expect that they validate the input before my validators gets into action. If I don't want to use this element property, I disable it setting preferFormInputFilter to true and use only the validators set for my form. The same with the select and other elements implementing InputProviderInterface. What do you think about it? |
|
@saltoledo You'll have to ask @weierophinney about that as there is a lot of history in the 2.2.3/2.2.4(hotfix) releases about it. As I've said before I'm surprised this issue wasn't seen earlier as it completely broke all our forms as it did for you. Thanks for finding a solution. p.s. I personally prefer the null approach (it's used elsewhere in the framework) over the hasChanged flag - can you get either approach implemented soon so we can push for a merge. |
Set Form::$preferFormInputFilter to false if user don't explicitly set its value and call Form::setInputFilter()
|
@davidwindell I've sent the changes. I'd implemented the hasChangedPreferFormInputFilter approach, as changing preferFormInputFilter to null would break testPreferFormInputFilterFlagIsEnabledByDefault(). |
|
@weierophinney your thoughts? |
|
|
||
| if ($inputFilter->has($name)) { | ||
| $input->merge($inputFilter->get($name)); | ||
| $inputFilter->replace($input, $name); |
There was a problem hiding this comment.
The problem with this approach is that replace is not in the interface. If the input filter implementation used does not have that method, we end up getting an error.
Two options:
- Duck-type:
if (method_exists($inputFilter, 'replace')) - Create a
ReplaceableInputInterfacewith thereplacemethod, and have theBaseInputFilterimplement it. Then do aninstanceofcheck:if ($inputFilter instanceof ReplaceableInputInterface).
The second is cleaner from an OOP perspective, and will ensure we include the functionality directly in the base interface in v3.
|
@saltoledo can you implement the ReplaceableInputInterface so that we can get this merged asap? |
|
@weierophinney I did as described in the second option. @davidwindell sorry for the delay, I have had a lot of work this week! |
|
Can someone merge this please, it's been 2 months now without further feedback. Ping @Maks3w @weierophinney |
|
|
||
| namespace Zend\InputFilter; | ||
|
|
||
| interface ReplaceableInputInterface |
There was a problem hiding this comment.
Please a docblock describing the purpose of the interface
Add element input filters before form input filters
- Incorporates feedback from @Maks3w
Forward port #5479 Conflicts: tests/ZendTest/Form/FormTest.php
Close #309 See zendframework/zendframework#5479 Compatibilizes with zendframework/zf2:~2.2.4, which uses `prefer_form_input_filter` by default
…rm-input-filter-fix Add element input filters before form input filters
- Incorporates feedback from @Maks3w
Forward port zendframework/zendframework#5479 Conflicts: tests/ZendTest/Form/FormTest.php
When preferFormInputFilter is set to false, input filters returned by the elements implementing InputProviderInterface are added to inputFilter before the form input filters. This way the inputs are validated as expected, e.g.: using email element, the email is validated before an user specified RecordExists runs.