Opened 9 years ago
Last modified 8 months ago
#25594 new New feature
Difficult to customize model field default_validators and have them used on both model and form fields
Reported by: | Marcin Nowak | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
There is incosistency between model's URLField and modelform's URLField validation.
Form's URLField does not use model's field validation.
Form and model fields have declared class properties with own validators, whose defaults to validators.URLValidator()
. But when you customize validator for model field, the form is still using own (and bad in this context) instance. As a result validation differs for form and model. And this is an incosistency between modelforms and models.
Change History (11)
comment:1 by , 9 years ago
Component: | Forms → Database layer (models, ORM) |
---|---|
Summary: | ModelForm`s URLField does not use model`s URLField validator → Difficult to customize model field default_validators and have them used on both model and form fields |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → New feature |
Version: | 1.8 → master |
comment:2 by , 9 years ago
Thinking out loud, too: provide a NO_DEFAULT_VALIDATORS
constant/class, when added to the validators
list prevent using the field's default validators.
comment:3 by , 8 years ago
From looking at the code, I suspect the existing reasoning would be as follows:
- Model fields have default validators, and devs can append to this list
- Form fields have default validators, and devs can append to this list
- When performing a clean on a ModelForm, it will invoke Model validation
So, if you reconfigure your model field to be more restrictive, this isn't a problem as it will catch the issue.
However, if you make it more permissive, this will cause problems in your form.
comment:4 by , 8 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
I created a pull request with my suggestions: PR.
It's not completely ready mainly because more tests should be added. But I'd like to hear opinions on the design before going too far. It has some backwards compatible issues (one test needed changes). I will ask for comments on the mailing list.
comment:5 by , 7 years ago
My ticket was marked as a duplicate of this, I'm not sure I agree, but I'll re-post here:
Example URL: "myscheme://server/group-name/item-name"
Problem 1: models.URLField
needs to be subclassed to change allowed schemes
Problem 2: forms.URLField
needs to be subclassed to change allowed schemes
Problem 3: validators.URLValidator
needs to be subclassed to change regex
to allow netloc
that is a hostname without a domain ad TLD
At the very least, making these changes should be documented better. The message accompanying ValidationError gives no hint as to the source issue with a URL.
comment:6 by , 5 years ago
From a design perspective, wouldn't it be clearer to have a new arguments for every forms.Field
and models.Field
that disables the default validators?
Something like disable_default_validators=False
.
When instantiating a field, one can set it to True
, and this will disable all default_validators
, and if it is set on a model field, will be propagated to the form field.
This way, the developer is in full control of the validators, both for the model field and the form field. Less magic, and more clear what is going on.
comment:8 by , 21 months ago
Fixing this ticket would also fix #25594 which is only applies to propagating MinValueValidator
and MaxValueValidator
validators.
comment:11 by , 8 months ago
Cc: | added |
---|
This problem exists for more than just
URLField
so I'm going to retitle the ticket to reflect that. Using the use case from #25593 of customizing aURLValidator
's accepted schemes, my idea would be to allow something like:URLField(validators=[URLValidator(schemes=[])])
and have that validator be used in place of the default validator on both the model and form field. Thinking out loud: would it be acceptable to replace any
default_validators
with values fromvalidators
if the value fromvalidators
is a subclass of a value indefault_validators
? I guess for something likeRegexValidator
you may want to have multiple regular expressions so we probably need a different way to say "use this validator to replace the default validators". Maybe adefault_validators
argument to model fields? This change seems tricky and likely needs a discussion on the DevelopersMailingList to nail down the API details, but I agree it'd be a worthwhile improvement.