powmedia / backbone-forms

Form framework for BackboneJS with nested forms, editable lists and validation
MIT License
2.17k stars 415 forks source link

form.commit({validate:false}) still validates individual fields #540

Open hcharley opened 7 years ago

hcharley commented 7 years ago

When I call form.commit({validate: false}) I am receiving errors in response.

var MyModel = Backbone.Model.extend({
  schema: {
    headline: { type: "Text", validators: [ "required" ] },
    slug: { type: "Text", validators: [ "required" ] }
  }
});
var model_instance = new MyModel();
var form = new BackboneForm({
  model: model_instance,
  fields: [ "slug", "headline" ]
}).render();

$('body').append(form.el);
var result = form.commit({ validate: false });
console.log("number of errors: ", _.keys(result).length);

screen shot 2017-02-07 at 2 37 27 pm

It looks like individual fields are being validated with in form.validate() BEFORE checking if skipModelValidate is set.

I'm going to file a PR shortly to check skipModelValidate before the individual fields are validated.

This wasn't intentional, right? I'm not missing something important?

philfreo commented 7 years ago

From the README:

There are 2 levels of validation: schema validators and the regular built-in Backbone model validation. Backbone Forms will run both when form.validate() is called. Calling form.commit() will run schema level validation by default, and can also run model validation if { validate: true } is passed.

In other words, I believe the validate flag is specifically designed to mirror / pass through to Backbone's set (whose option is described on http://backbonejs.org/#Model-validate). So yes, I think it was intentional this way and it would definitely a backwards incompatible change for validate: false to suddenly also ignore schema-level validation.

If this behavior is really needed, perhaps introducing another option (e.g. schemaValidate) would be better, though I'd question why you bother with form/schema validation if you want to disable it.

hcharley commented 7 years ago

@philfreo Thanks for breaking it down for me. I thought there was an intent that I wasn't picking up on.

My usecase is this:

I want to show users a rendered preview of their form values as they input data. Like a WYSIWG. (e.g. insert a headline in a form input, have it show up in a "preview pane" inside an <h1> tag).

I have a slug field that I definitely want to validate on form submission, but I don't care about whether or not it's valid before then. The slug doesn't show up in the preview pane.

So if the slug fails validation, I still want to be able to commit other values, like headline, to the model so that they get rendered on the preview. I don't want one invalid field to break my entire preview pane.

On form submit, I will definitely validate and prevent submission if there are invalid fields.

philfreo commented 7 years ago

One idea is to move slug validation to the Backbone model then. Or have a selective backbone-forms validator that only gets triggered when a certain criteria (final submission) is true.

hcharley commented 7 years ago

I think I see what you're getting at. It's an anti-pattern to commit dirty data to the model, and in a sense defeats the purpose of having validation schemas in the first place. Perhaps, I am the only exception who would like to commit dirty data to my model.

However, I'm not a big fan of moving validation to the Backbone model's validate, as I lose out on backbone-form's schema pattern, which I really prefer, and ultimately on final submission, I want to use.

Also, moving to a custom validator would be alright, but in practice, I'll probably end up writing custom validators for all my fields and then would have to lay some more code on to determine the state of a commit (final submission vs. preview render). So I'd either have to abstract Backbone-Form's validators or rewrite them.

I'm going to submit a PR to see if there are thoughts on adding an option to form.commit().

0xgeert commented 7 years ago

How about having a different form + schema (in which you define which fields you want to validate. e.g.: all except slug) for the preview pane? Preview pane and normal form are backed by the same model.

On Wed, Feb 8, 2017 at 1:46 AM, Charley Bodkin notifications@github.com wrote:

I think I see what you're getting at. It's an anti-pattern to commit dirty data to the model, and in a sense defeats the purpose of having validation schemas in the first place. Perhaps, I am the only exception who would like to commit dirty data to my model.

However, I'm not a big fan of moving validation to the Backbone model's validate, as I lose out on backbone-form's schema pattern, which I really prefer, and ultimately on final submission, I want to use.

Also, moving to a custom validator would be alright, but in practice, I'll probably end up writing custom validators for all my fields and then would have to lay some more code on to determine the state of a commit (final submission vs. preview render). So I'd either have to abstract Backbone-Form's validators or rewrite them.

I'm going to submit a PR to see if there are thoughts on adding an option to form.commit().

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/powmedia/backbone-forms/issues/540#issuecomment-278193400, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYAK2TAs-bvImthj1q39_Is1Xei1zl3ks5raRBkgaJpZM4L6I4s .

0xgeert commented 7 years ago

Don't write unvalidated data to the model. That's indeed an anti-pattern. You never know which other view attached to the model expects a valid state and would totally crash. Don't break that interface/contract.

On Wed, Feb 8, 2017 at 10:21 AM, Geert-Jan Brits (Clearskyabove) < geertjan@clearskyabove.com> wrote:

How about having a different form + schema (in which you define which fields you want to validate. e.g.: all except slug) for the preview pane? Preview pane and normal form are backed by the same model.

On Wed, Feb 8, 2017 at 1:46 AM, Charley Bodkin notifications@github.com wrote:

I think I see what you're getting at. It's an anti-pattern to commit dirty data to the model, and in a sense defeats the purpose of having validation schemas in the first place. Perhaps, I am the only exception who would like to commit dirty data to my model.

However, I'm not a big fan of moving validation to the Backbone model's validate, as I lose out on backbone-form's schema pattern, which I really prefer, and ultimately on final submission, I want to use.

Also, moving to a custom validator would be alright, but in practice, I'll probably end up writing custom validators for all my fields and then would have to lay some more code on to determine the state of a commit (final submission vs. preview render). So I'd either have to abstract Backbone-Form's validators or rewrite them.

I'm going to submit a PR to see if there are thoughts on adding an option to form.commit().

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/powmedia/backbone-forms/issues/540#issuecomment-278193400, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYAK2TAs-bvImthj1q39_Is1Xei1zl3ks5raRBkgaJpZM4L6I4s .