Re: [Django] #25887: Clarify support for ForeignKey and form fields other than ModelChoiceField
Django
-------------------------------------+-------------------------------------
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ModelForm, | Triage Stage:
ForeignKey | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by mgersh:
Old description:
> I'm maintaining a project which has a similar situation to #23795: we
> have an ajax autocomplete form field for ForeignKeys, which in our case
> inherits from `forms.IntegerField`. We also have a custom model field
> which subclasses ForeignKey in order to override `formfield()` and
> specify our custom form field as the default.
>
> When we updated from Django 1.6 to 1.8, ModelForms for all models using
> this field started erroring on import because of the `limit_choices_to`
> keyword argument getting inserted by `RelatedField.formfield()`, not
> being handled by our form field (which has no support for
> `limit_choices_to` because we haven't needed it), and eventually being
> passed to the `Field` constructor through some `super()` calls.
>
> Here is a simple example of the error:
> {{{
> class CustomForeignKey(models.ForeignKey):
> def formfield(self, **kwargs):
> defaults = {'form_class': IntegerField} # or a subclass of
> IntegerField
> defaults.update(kwargs)
> return super(CustomForeignKey, self).formfield(**defaults)
>
> class Target(models.Model):
> pass
>
> class NormalFKModel(models.Model):
> fk = models.ForeignKey(Target)
>
> class CustomFKModel(models.Model):
> fk = CustomForeignKey(Target)
>
> # This will work fine until I try to submit the form
> class NormalFKModelForm(forms.ModelForm):
> fk = IntegerField()
> class Meta:
> model = NormalFKModel
> fields = '__all__'
>
> # This will error on import on all Django versions
> class CustomFKModelForm(forms.ModelForm):
> class Meta:
> model = CustomFKModel
> fields = '__all__'
> }}}
>
> A similar error happens even on older Django, or when I add a workaround
> for `limit_choices_to` only; the code I had which worked on 1.6 already
> included some workarounds for a few other similar issues, which I hadn't
> been aware of.
>
> The [https://docs.djangoproject.com/en/dev/howto/custom-model-fields
> /#specifying-the-form-field-for-a-model-field documentation on overriding
> formfield()] doesn't mention this pitfall, and neither does the
> [https://docs.djangoproject.com/en/dev/ref/forms/fields/#creating-custom-
> fields form field documentation], or any other relevant documentation I
> could find.
>
> Ideally, both of the `ModelForm` examples above would display without
> erroring, and would work correctly if the `forms.IntegerField` is
> replaced by a subclass which normalizes to model instances rather than
> integers, especially considering that one of them already meets those
> conditions. Alternatively, I would also consider this fixed if some
> documentation were added on special requirements for custom form fields
> that will be used for ForeignKeys.
New description:
I'm maintaining a project which has a similar situation to #23795: we have
an ajax autocomplete form field for ForeignKeys, which in our case
inherits from `forms.IntegerField`. We also have a custom model field
which subclasses ForeignKey in order to override `formfield()` and specify
our custom form field as the default.
When we updated from Django 1.6 to 1.8, ModelForms for all models using
this field started erroring on import because of the `limit_choices_to`
keyword argument getting inserted by `RelatedField.formfield()`, not being
handled by our form field (which has no support for `limit_choices_to`
because we haven't needed it), and eventually being passed to the `Field`
constructor through some `super()` calls.
Here is a simple example of the error:
{{{
class CustomForeignKey(models.ForeignKey):
def formfield(self, **kwargs):
defaults = {'form_class': forms.IntegerField} # or a subclass of
forms.IntegerField
defaults.update(kwargs)
return super(CustomForeignKey, self).formfield(**defaults)
class Target(models.Model):
pass
class NormalFKModel(models.Model):
fk = models.ForeignKey(Target)
class CustomFKModel(models.Model):
fk = CustomForeignKey(Target)
# This will work fine until I try to submit the form
class NormalFKModelForm(forms.ModelForm):
fk = forms.IntegerField()
class Meta:
model = NormalFKModel
fields = '__all__'
# This will error on import on all Django versions
class CustomFKModelForm(forms.ModelForm):
class Meta:
model = CustomFKModel
fields = '__all__'
}}}
A similar error happens even on older Django, or when I add a workaround
for `limit_choices_to` only; the code I had which worked on 1.6 already
included some workarounds for a few other similar issues, which I hadn't
been aware of.
The [https://docs.djangoproject.com/en/dev/howto/custom-model-fields
/#specifying-the-form-field-for-a-model-field documentation on overriding
formfield()] doesn't mention this pitfall, and neither does the
[https://docs.djangoproject.com/en/dev/ref/forms/fields/#creating-custom-
fields form field documentation], or any other relevant documentation I
could find.
Ideally, both of the `ModelForm` examples above would display without
erroring, and would work correctly if the `forms.IntegerField` is replaced
by a subclass which normalizes to model instances rather than integers,
especially considering that one of them already meets those conditions.
Alternatively, I would also consider this fixed if some documentation were
added on special requirements for custom form fields that will be used for
ForeignKeys.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
class Target(models.Model):
pass
--
Ticket URL: <https://code.djangoproject.com/ticket/25887>
Django
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: ModelForm, | Triage Stage:
ForeignKey | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by benjaminjkraft):
* cc: benjaminjkraft@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:1>
Django
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: ModelForm, | Triage Stage:
ForeignKey | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Is there a reason you need a custom form field instead of simply a custom
widget?
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:3>
Django
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: ModelForm, | Triage Stage:
ForeignKey | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mgersh):
Yes, we have some custom logic in the form field's `__init__()` and
`clean()` as well. In any case, this error happens in almost the same
situation as in #23795, but with the addition of a custom model field
which exists only to set the custom form field as the default, and if that
use case is supported, it makes sense for this one to be as well. The
issue with `limit_choices_to` was a regression since 1.6, and the
documentation currently has no indication that what we're trying to do
might not be supported.
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:4>
Django
Reporter: mgersh | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: ModelForm, | Triage Stage:
ForeignKey | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I haven't fully investigated but I'm not sure Django can do anything about
it other than document that your `Field` must pop off any unexpected
`kwargs`. Do you have a proposal?
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:5>
Django
Reporter: mgersh | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: ModelForm, | Triage Stage: Accepted
ForeignKey |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Bug => Cleanup/optimization
* component: Forms => Documentation
* stage: Unreviewed => Accepted
Comment:
Accepting as a documentation issue pending further investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:6>
Django
Reporter: Miriam Gershenson | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: dev
Keywords: ModelForm, | Triage Stage: Accepted
ForeignKey |
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25887#comment:7>