Code

Opened 7 years ago

Closed 6 years ago

#3815 closed (wontfix)

form_for_* methods should be able to use a Form superclass

Reported by: jon.i.austin@… Owned by: nobody
Component: Forms Version: master
Severity: Keywords: newforms
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

hi,

I'm not sure if this is a bug or not, but according to the newforms docs, one should be able to subclass an existing form class and add fields to it.
This works just fine if the ancestor form class is explicitly defined in the code.

However if one creates a new Form class using newforms.form_for_model(MyModelClass) and tries to subclass the resulting class it doesn't work.
i.e. using the Polls model from the tutorial:

from polls import models
from django.newforms import form_for_model
PollForm = form_for_model(models.Poll)

class PollFormWithTest(PollForm):
   ....:     test = charField()

testPollWithTest = PollFormWithTest()

print testPollWithTest.fields
{'question': <django.newforms.fields.CharField object at 0xb745b62c>, 'pub_date': <django.newforms.fields.DateTimeField object at 0xb745b58c>}

print testPollWithTest.test
<django.newforms.fields.CharField object at 0xb745b52c>

as you can see test is added as an base attribute to the instance, not as a field.

Is this perhaps because the PollForm is an instance of a class and not an actual class definition?
If so is there a way around this, and could it perhaps be added to the docs?

Also: if the above is the case the docstring for newforms.form_for_model should probably be changed:
Returns a Form class for the given Django model class.
to: s/a Form class/an instance of a Form class/

thanks!

jon

Attachments (3)

subclass_form.diff (1.3 KB) - added by SmileyChris 7 years ago.
subclass_form.2.diff (1.4 KB) - added by SmileyChris 7 years ago.
safer: makes sure base_fields is a dict instance
subclass_form.3.diff (1.6 KB) - added by SmileyChris 7 years ago.
another test

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Try form_for_model(models.Poll, form=Form) (BaseForm is the default superclass for these methods)

comment:2 Changed 7 years ago by jon austin <jon.i.austin@…>

ah, thanks much, I should have looked closer at the django code, where it explicitly states Form adds the fields attribute. lesson learned. :)

oh, and if anyone else reads this -- form_for_model does of course return a class not an instance ... as I would have seen had i studied the form_for_model code a bit...

comment:3 Changed 7 years ago by matt@…

  • Resolution invalid deleted
  • Status changed from closed to reopened

I might be missing something, but this no longer seems to work.

Using revision 5808, form_for_model(models.Poll, form=Form) seems to remove all the fields ordinarily generated by form_for_model. When you subclass it, additional fields are added correctly, but obviously this defeats the purpose of subclassing in the first place.

Details and an example are at [http://groups.google.com/group/django-users/browse_thread/thread/b1e0ba0406f51da/62adfe8729ead1fd?lnk=gst&q=subclassing+form_for_model&rnum=1#62adfe8729ead1fd
]

In the mean time, this seems to work:

ModelForm = forms.models.form_for_model(Model)

class ModelFormWithName(ModelForm):
    ModelForm.base_fields['name'] = forms.CharField(max_length=31,widget=forms.TextInput(), label='Name')

comment:4 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

It used to work back in the day... I guess the meta class magic changed between then and now. I just tried this after applying my patch in #5050 and it works, so I'll keep this open as a Design Decision for now.

Changed 7 years ago by SmileyChris

Changed 7 years ago by SmileyChris

safer: makes sure base_fields is a dict instance

Changed 7 years ago by SmileyChris

another test

comment:5 Changed 7 years ago by SmileyChris

  • Has patch set
  • Summary changed from subclassed newforms does not work with ancestor class created with form_for_model(ModelClass) to form_for_* methods should be able to use a Form superclass
  • Triage Stage changed from Design decision needed to Ready for checkin

This ticket has mutated to a different issue slightly, but I'll keep it open and just re-summarize it.

Using a Form (or subclass of Form) as a superclass should work fine. My patch is simple and adds this functionality, with no possible side effects that I can see

Promoting to checkin.

comment:6 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

Given the plan to deprecate/remove form_for_* based on Joseph's new work with ModelForm classes, I'm going to punt this for the time being. We can close or re-promote once a resolution has been reached on the other thing.

comment:7 Changed 7 years ago by brosner

ModelForm has been committed to trunk. I would think this ticket can be consider fixed?

comment:8 Changed 6 years ago by brosner

  • Resolution set to wontfix
  • Status changed from reopened to closed

Considering form_for_* are now replaced by ModelForm this is no longer needed. You can accomplish inheritance with ModelForm as of r7112.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.