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

Zend-Form: Allow Input Filter Preference Over Element Defaults #3218

Closed
wants to merge 1 commit into from
Closed

Zend-Form: Allow Input Filter Preference Over Element Defaults #3218

wants to merge 1 commit into from

Conversation

mwillbanks
Copy link
Contributor

Purpose:
When building out forms you may want to give preference to the InputFilter set on the form while maintaining defaults that you have not overwritten.

The Change
A new property and 2 methods in Zend\Form\Form which set preferFormInputFilter variable
When checking for input filters; this property is utilized to simply continue if we find one otherwise it does what it did before.
The change is BC friendly.

Simple Example

  1. Email element has defaults
  2. Eliminate the validation portion
  3. Utilize NotEmpty validator
  4. Set a custom message.
  5. Only NotEmpty validator is now set with custom error message (this wouldn't work previously)
    NOTE
  6. This drops off all filters and any other validators, basically this overrides element defaults completely.
$form->setPreferFormInputFilter(true);
$form->setInputFilter($inputFilterFactory->createInputFilter(array(
    'email' => array(
        'name' => 'email',
        'required' => true,
        'validators' => array(
             array(
                 'name' => 'NotEmpty',
                 'options' => array(
                     'messages' => array(
                          'notEmpty' => 'My new message for not empty',
                     ),
                 ),
             ),
         ),
    ),
));

…er over defaults from elements while still allowing element defaults if nothing was defined in the form filter
@bakura10
Copy link
Contributor

Maybe the naming could be "shouldUseFormInputFilter" to be more consistent with other elements, or "setPreferFormInputFilter" as setter and "preferFormInputFilter" and getter (as we use setShouldBlaBla and shouldBlaBla when using verbs).

However, I'm not a big fan of this feature. Basically, what you're doing is simply get rid of everything of the element (validators, filters)... Why not just doing that :

// Add your element without using specialized type
$this->add(array(
    // No type, default to Element
    'name' => 'email',
    'attributes' => array(
        'type' => 'email'
    )
));

// And add your own validators and filters to the input filter

I find it cleaner. If you basically drop of everything that is interesting when using a specialized element like Email, why not just not using it ?

@ghost ghost assigned weierophinney Dec 13, 2012
@mwillbanks
Copy link
Contributor Author

@bakura10 the actual handing I do use bits and pieces of; was really just showing how you can get into that condition. Take for an example that you want all of the validation but then want to customize the error messages from the above filter; you could grab the validators off of the email element itself and then subsequently modify the array to add in the messages portion.

@mwillbanks
Copy link
Contributor Author

Changing it to should makes sense but I do believe prefer makes more sense in this case. shouldUseFormInputFilter to me sounds like that it will use the form input filter or it wont (meaning if i was to set it and that value was false just by looking at the api i would think that it would not use the form input filter at all); whereas prefer is basically stating that i want to utilize it over other items.

@cgmartin
Copy link
Contributor

Hi @mwillbanks , asking this to try and clarify/understand the current behavior better...

In your example, if you omit the: $form->setPreferFormInputFilter(true); what happens with the email Input that you specified? Will your custom Input specification be overwritten by the email element's getInputSpecification()?

@mwillbanks
Copy link
Contributor Author

@cgmartin correct, the custom input specification will be overwritten by the email elements getInputSpecification.

@cgmartin
Copy link
Contributor

@mwillbanks thanks for confirming... Could this be a bug or is this the intended behavior? I would find it more intuitive that $form->setPreferFormInputFilter(true); be the default behavior - that, if the inputFilter already has a specified Input, the defaults be skipped... or at the minimum, apply the defaults first and then merge my specification over the top of it.

There's also the useInputFilterDefaults setting that could be turned off, but that doesn't really help in a mixed case where you want the unspecified inputs to use defaults.

If the overwriting of the custom input specification is not the intended behavior (a bug), lets simplify and remove the getter/setter and just do the $inputFilter->has($name) test to continue/skip. If this behavior was intended, I think what you've got here is a good solution to avoid BC.

I need to go back through my code and make sure I don't have bugs from this. I assumed my custom input filter specifications were not being merged over by the defaults.

@cgmartin
Copy link
Contributor

@mwillbanks I'm less concerned now of there being a bug in the behavior...

I went back through my project's forms and they seemed to be correctly applying the default input specifications first and then merging my custom specification over the top of it. I am not using any custom NotEmpty validators, though.

From the code, it looks like custom specifications are merged over the top of the default specification. But, the custom validators are appended to the default spec validators...making your custom NotEmpty validator last in the chain:

https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/BaseInputFilter.php#L67
https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Input.php#L259
https://github.com/zendframework/zf2/blob/master/library/Zend/Validator/ValidatorChain.php#L194

This behavior seems intuitive to me for most cases... but it definitely poses a problem for custom NotEmpty validators like in your case, or even when replacing other validators in the default input specification. The NotEmpty validators also need to come first in the chain:

https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Input.php#L318

An alternate workaround could be to add the defaults and then modify the validators afterwards:

$form->add(new \Zend\Form\Element\Email('email'));
$inputFilter = $form->getInputFilter();  // triggers adding the default input specs
$inputFilter->get('email')
    ->setRequired(true)
    ->getValidatorChain()->prependByName(
        'notEmpty', array('messages' => array(
            'notEmpty' => 'My new message for not empty',
     ))
);

I think the PR would still be handy to have, so you could do things all up front.

@mwillbanks
Copy link
Contributor Author

@cgmartin

Yeah the whole idea is to skip the amount of manual code within the form to do things like that. Custom messaging is a huge piece of most websites. Re-assigning them through the input filter or the form specifically is not as easy to leverage since you might be writing the input filter for multiple purposes and in that case it can be a bit frustrating since then you have to manually modify say the form or overwrite certain behaviors by providing a base class.

@weierophinney
Copy link
Member

@cgmartin and @bakura10 -- are you okay with this? It seems reasonable so far, and it looks like most issues are addressed -- but it seems like @cgmartin may still have some lingering reservations. Update me. :)

@bakura10
Copy link
Contributor

I'm ok with it now that I've understood it.

@cgmartin
Copy link
Contributor

@weierophinney no lingering reservations. Good to go. 👍

@weierophinney
Copy link
Member

Merged to develop.

weierophinney added a commit that referenced this pull request Dec 19, 2012
- whitespace after operators
- use identity operator when testing against null
- array/argument indentation
kbabioch pushed a commit to kbabioch/zf2 that referenced this pull request Dec 19, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants