Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ValidationScope: User unfriendly implementation #111

Open
juniwalk opened this issue Feb 17, 2016 · 24 comments
Open

ValidationScope: User unfriendly implementation #111

juniwalk opened this issue Feb 17, 2016 · 24 comments

Comments

@juniwalk
Copy link

Hello,
I'd like to ask, could we come up with some way to rework disabling validation scope for selected fields?

Currently if you have 17 fields and one of them is AJAXified <select>, you have to do this:

$form->addSubmit('submit')->setValidationScope([
        $form['name'], $form['email'], $form['password'], $form['phone'], $form['ic'],
        $manage['isBanned'], $manage['isApproved'], $account, $address,
]);

Best way would be to disable this per-field.

@dg
Copy link
Member

dg commented Feb 17, 2016

What about condition?

$form->addSelect('country', 'Country:', $countries);
$form->addSubmit('submit', 'Send');

$form['country']
    ->addConditionOn($form['submit'], $form::SUBMITTED)
        ->setRequired('Choose country');

@juniwalk
Copy link
Author

How does this help me when I have those countries loaded using AJAX and have $select->checkAllowedValues = FALSE;? I need to disable scope validation for that select for the submit to work.

Sorry if I haven't made myself clear about that.

@dg
Copy link
Member

dg commented Feb 19, 2016

Why you need to disable scope validation for select?

@juniwalk
Copy link
Author

Because I am stupid, $select->setPrompt() is what I needed, not to disable validation scope. But I still think that disabling the scope per field would be better.

@juniwalk
Copy link
Author

@dg I am closing this as it is probably unnecessary to have it here.

@juniwalk
Copy link
Author

juniwalk commented Mar 2, 2016

@dg I have to disable validation scope when I use $select->setRequired();, $select->setPrompt() is not applicable in this case.

The problem is when I submit form with AJAX data, the value of select is outside $data variable, I have to get it using $form->getHttpData() which is okay but the form fails to submit because value of select is required and there is none in the time of checking.

I am back to my initial issue, disabling validation scope in this way is not friendly.

@juniwalk juniwalk reopened this Mar 2, 2016
@dg
Copy link
Member

dg commented Apr 1, 2016

Can you try to implement any solution?

@juniwalk
Copy link
Author

juniwalk commented Apr 1, 2016

@dg Well I have a few ideas

First idea

  • Add $validateScope property to BaseControl which will be bool.
  • Allow disabling scope per control using $control->disableValidationScope(); // just examples
  • Before the form is builded, iterate over each control and build validationScope and assign it to SubmitButton
  • But what if there is more than one SubmitButton control? Is that possible? That would cause problems.

Second idea

  • Add class Nette\Forms\ValidationScopeBuilder
  • Implement \Traversable or extend \ArrayObject
  • Pass form instance into it
  • Call $vsb->disableScopeFor('control');
  • Pass instance into $submit->setValidationScope($vsb);
  • This will get all current controls from the form, remove those disabled and foreach in setValidationScope will iterate over them.

Third idea

  • Each BaseControl can get it's parent Form instance
  • We could add disableValidationScopeFor('control') on the SubmitButton control
  • Add those control names into list and then just select all controls from form and leave out disabled ones

What do you think? Does any of those sound good?

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Jun 19, 2016

the form fails to submit because value of select is required and there is none in the time of checking.

Why not just set $select->checkAllowedValues = FALSE? Redesigning validation scope seems like overkill.


EDIT: Seems like we would need to modify ChoiceControl::getValue() to respect $checkAllowedValues as well. What do you think?


EDIT: No we can't do it, it would be a BC break.

@juniwalk
Copy link
Author

@JanTvrdik Yeah, it is particularly troubling issue this one. I don't mind working with validation scope, it's just not very intuitive to work with.

@JanTvrdik
Copy link
Contributor

I don't think that validation scopes should be used for this kind of issue at all.

Another random idea

$form->addSelect('country', 'Country', [$form->getHttpData('country') => '...'])
    ->setRequired();

@juniwalk
Copy link
Author

@JanTvrdik I don't think that validation scopes should be used for this kind of issue at all.

Really? Why?

With your another random idea, I see potential problem with setting default value for editing.

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Jun 20, 2016

@juniwalk Because if you don't want the value to be validated, you should not make the select box required. What's the point of setting validation rules if they should be ignored?

@juniwalk
Copy link
Author

@JanTvrdik I do want the value validated, and I need it to be required, but that does not play well with AJAX values feeded in to the select box ...

@JanTvrdik
Copy link
Contributor

I do want the value validated

Could you elaborate on what exactly and where do you validate?

@juniwalk
Copy link
Author

@JanTvrdik In this context I ment the requirement validation as i use this to suggest user names and I get back just int (user Id) which I then in the onSuccess (probably not properly done but that is how I use it).

@JanTvrdik
Copy link
Contributor

I'm sorry, I still don't understand it. How could you use the server-side validation when the select box is not validated at all because it is not included in the validation scope? What did I miss?

@juniwalk
Copy link
Author

@JanTvrdik Ignore it, we diverged from the problem here, validation is not the issue, getting the value is.

@juniwalk
Copy link
Author

@JanTvrdik Yes, and to disable the scope is absolute horrendous work to do (especially for large forms).

@dg
Copy link
Member

dg commented Jun 20, 2016

->setValidationScope(array_diff_key($form->getControls(), ['country' => TRUE])) is horrendous?

edit: $form->getControls() returns ArrayIterator, so iterator_to_array($form->getControls()).

@juniwalk
Copy link
Author

@dg Hey, now that looks much better, would you mind if I send PR to docs with this example to help others like me?

@dg
Copy link
Member

dg commented Jun 20, 2016

Of course I'll be happy. But try it please, I wrote from my head.

@juniwalk
Copy link
Author

juniwalk commented Jun 22, 2016

@dg So I tried that array_diff_key and it throws an error
PHP Warning: array_diff_key(): Argument #1 is not an array in ./app/forms/ProductForm.php:136

EDIT: I'd have to combine it with iterator_to_array(). Having $select->disableValidationScope('submit'); would be really lovely.

@juniwalk
Copy link
Author

Hi,
I revisited this old issue. Now when I am more skilled in Nette I basically never modify validationScope, I do my own validation in onValidate event.

I was modifying it before to keep the clientSide validation so user gets alerted before submitting that he did not select something required, however I can live without it.

I think we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants