[Django] #25594: ModelForm`s URLField does not use model`s URLField validator
Django
----------------------------+--------------------
Reporter: marcinn | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: marcinn | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* component: Forms => Database layer (models, ORM)
* needs_tests: => 0
* version: 1.8 => master
* needs_docs: => 0
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
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
a `URLValidator`'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 from `validators` if the
value from `validators` is a subclass of a value in `default_validators`?
I guess for something like `RegexValidator` 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 a `default_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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:1>
Django
both model and form fields
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Thinking out loud, too: provide a `NO_DEFAULT_VALIDATORS` constant/class,
when added to the `validators` list prevent using the field's default
validators.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:2>
Django
both model and form fields
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by funkybob):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:3>
Django
both model and form fields
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
I created a pull request with my suggestions:
[https://github.com/django/django/pull/7161 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:4>
Django
both model and form fields
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Arthur Pemberton):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:5>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Federico Jaramillo Martínez):
From a design perspective, wouldn't it be clearer to have a new arguments
for every `FormField` and `ModelField` 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:6>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Federico Jaramillo Martínez):
If my proposed solution sounds good, I can work on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:7>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak):
Fixing this ticket would also fix #25594 which is only applies to
propagating `MinValueValidator` and `MaxValueValidator` validators.
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:8>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Claude Paroz):
You meant to mention #26834 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:9>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Mariusz Felisiak):
Replying to [comment:9 Claude Paroz]:
> You meant to mention #26834 ?
Yes!
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:10>
Django
both model and form fields
-------------------------------------+-------------------------------------
Reporter: Marcin Nowak | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:11>
Django
both model and form fields
-------------------------------------+-------------------------------------
| Gama
Type: New feature | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Clifford Gama):
* owner: nobody => Clifford Gama
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25594#comment:12>