fix(ngModel): don’t clear the model when an external validator failed

[tbosch]

Calling ctrl.$setValidity() with a an error key that
does not belong to a validator in ctrl.$validator should
not result in setting the model to undefined on the next
input change. This bug was introduced in 1.3.0-beta.12.

Closes #8357
Fixes #8080

[jeffbcross]

s/changes/change

[jeffbcross]

Added comments, otherwise LGTM.

[Narretz]

As I said, I don't like it. It introduces a distinction between validity set by $validators, and validity set externally. I think this is something that is gonna be difficult to explain to users. Let's not try to recreate the old behavior simply because it was there.

[Narretz]

We should at least wait and try to get some input from the original posters. But from a consistency and understandability POV this makes the API worse.

[tbosch]

@Narretz Without this change using ctrl.$setValidity without ngOptions.allowInvalid will never make sense / always lead to the problem described in #8080 (even if we kept an internal $parsedModelValue or something else). The only "clean" way to solve this would be to make ngOptions.allowInvalid the default behavior always.

[Narretz]

Without this change using ctrl.$setValidity without ngOptions.allowInvalid will never make sense

Yeah, that makes sense.
FWIW, I think we're now preventing using $setValidity to augment the default validation process, i.e. when you set validity based on your conditions, and expect it to work with normal validation. I don't know if people will expect it to work this way, though.

[Narretz]

Aaaand, there's still no easy way to get the parsed model value, when (a) your model is invalid (but not because of $setValidity), and (b) you don't use allowInvalid - for example in a form submit handler. That's it, I think.

[tbosch]

@Narretz I don't understand that usecase: you want to get the invalid value, but don't want to use the allowInvalid. Could you explain it a little but more when this is important?

[Narretz]

It is important when you want only valid values on the scope, but still want to do some postprocesing in your form handling. (so you don't use allowInvalid).

You submit a form, and find that a value is invalid.
Now you want to send the whole form to your legacy backend for further validation, but you can't get the $modelValue. Or your errors are somehow recoverable - $modelValue still undefined.
Unfortunately, you have also made some complex parse actions, and now you must extract the parser run logic from input core, put them in a directive and run them again*

My problem is that it is strange that we simply throw away the parsed model value when it is invalid, and force you to use allowInvalid to get it. Restoring the validation behavior of $setValidity was basically a workaround for one specific use case. For me the API is incomplete without the parsed model value, or an easy way to get it.

*Running formatters manually has been requested actually, so $runParsers would be one way to make this easier.

[tbosch]

So why would you not use allowInvalid and in you code just check whether the value that you got from ng-model is valid by checking form.myInput.$error?

[Narretz]

allowInvalid sets the model on the scope, and this might be undesired if it is invalid. For example, you display your form inputs in another way, but you only want that for valid values. Or your model is watched on the scope.

[tbosch]

For reading ctrl.$modelValue you need a directive.
Using the example you gave before:

Now you want to send the whole form to your legacy backend for further validation...

For that you actually would want to have the invalid value on the scope and not implement this logic in a directive...
So I am still not convinced. But I really like the discussion!

[shahata]

You don't really need a directive... It is available at scope.form.input.$modelValue. Or did I miss something?

[tbosch]

Right! As the whole controller lives there... Never used it that way though...
Ok @Narretz, could you create a new PR that changes $modelValue to always be the parsed model value (just like your last PR that I closed, but including allowInvalid)?

[tbosch]

Or @shahata

[shahata]

@tbosch it makes more sense to me that the invalid model value will be stored on ctrl.$invalidModelValue or something and not ctrl.$modelValue. It is also not accurate to call it parsed model value, since it will store the value also in case scope changes programatically. WDYT?

[Narretz]

Hey guys, I like where this is going! ;)
@shahata You are right, parsedModelValue doesn't quite reflect when it is set programatically. The idea is to have a property on the modelController that always reflects the parsed or set value of the model, without encoding any sort of validity info. When you call it $invalidModelValue, that suggests you would set it to undefined when $modelValue is valid, (or at least the name suggests it), but this is exactly what we don't want - there should be an easy way to get the value without having to check $invalidModelValue || $modelValue, manually call the parsers etc. Another idea for the name could be $rawModelValue.
Also, can you elaborate why it doesn't make sense to store it on $modelValue? It's true that the current semantic is to have $modelValue reflect the scope value, but that's not really something that is set in stone. Setting it to undefined when invalid is basically convention because of the old parsers-for-validation behavior, so this is a mute point, too.

[shahata]

I'm fine with the semantics of $rawModelValue, not sure about the name though, but we can sort that out later. The reason I don't like the idea of using $modelValue is that I assume some directives will still want the value that is actually assigned to the model. (which is also what the name implies)