Opened 10 years ago

Closed 9 years ago

#3922 closed (fixed)

formfield_callback in form_for_model also changes refering model

Reported by: Jure Cuhalev <gandalf@…> Owned by: Philippe Raoult
Component: Forms Version: master
Severity: Keywords: form_for_model
Cc: gandalf@…, cgrady@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Using formfield_callback with newforms form.form_for_model will also change the model you are refering to.

def CategoryCallback(f, **kwargs):
    if not "cat_" in f.name:
      f.name = 'cat_'+f.name
    return f.formfield(**kwargs)

>>> w = Writer(name='Bob Woodward')
>>> Article(headline='Test article', pub_date=datetime.date(1988, 1, 4), writer=w, article='Hello.')
<Article: Test article>

>>> ArticleForm = form_for_model(Article, formfield_callback=CategoryCallback)
>>> f = ArticleForm(auto_id=False)
>>> print f
<tr><th>Headline:</th><td><input type="text" name="cat_headline" maxlength="50" /></td></tr>
<tr><th>Pub date:</th><td><input type="text" name="cat_pub_date" /></td></tr>
<tr><th>Writer:</th><td><select name="cat_writer">
<option value="" selected="selected">---------</option>
</select></td></tr>
<tr><th>Article:</th><td><textarea name="cat_article"></textarea></td></tr>
<tr><th>Categories:</th><td><select multiple="multiple" name="cat_categories">
</select><br /> Hold down "Control", or "Command" on a Mac, to select more than one.</td></tr>
>>> w = Writer(name='Bob Woodward')

but then doing this one (same one as in the beginning):

>>> Article(headline='Test article', pub_date=datetime.date(1988, 1, 4), writer=w, article='Hello.')

fails with:

    Traceback (most recent call last):
      File "/Users/gandalf/django/django_src/django/test/doctest.py", line 1243, in __run
        compileflags, 1) in test.globs
      File "<doctest modeltests.model_forms_callback.models.__test__.API_TESTS[16]>", line 1, in ?
        Article(headline='Test article', pub_date=datetime.date(1988, 1, 4), writer=w, article='Hello.')
      File "/Users/gandalf/django/django_src/django/db/models/base.py", line 166, in __init__
        raise TypeError, "'%s' is an invalid keyword argument for this function" % kwargs.keys()[0]
    TypeError: 'writer' is an invalid keyword argument for this function

I have attached more comprehensive test derived from modeltests/model_form. I belive that form_for_model with callback should not affect the actual model. It is also not clear if by doing my approach .save() should still work normally or is up to the developer to rename fields back to the way they were originally defined.

Attachments (2)

models.py (3.4 KB) - added by Jure Cuhalev <gandalf@…> 10 years ago.
Test for described formfield_callback problem
3922.patch (704 bytes) - added by Philippe Raoult 9 years ago.
patch to newforms.txt to mention this problem

Download all attachments as: .zip

Change History (7)

Changed 10 years ago by Jure Cuhalev <gandalf@…>

Attachment: models.py added

Test for described formfield_callback problem

comment:1 Changed 10 years ago by Collin Grady <cgrady@…>

Cc: cgrady@… added

Doesn't the prefix argument to a Form do the same thing, but without the side-effects? :)

Editing the model fields to produce changes in the form output seems a bit backwards - perhaps a custom base form with init overrided would be better than trying to do certain modifications in the callback?

comment:2 Changed 10 years ago by Jure Cuhalev <gandalf@…>

Yes, it probably would. But using it in this "wrong" way still shouldn't break the model in a running project.

comment:3 Changed 9 years ago by Philippe Raoult

There are no object/class permissions in python, there is no way to "fix" this. The documentation should mention that the argument passed to the callback is the actual class field and should not be treated as a temporary variable you can modify.

Changed 9 years ago by Philippe Raoult

Attachment: 3922.patch added

patch to newforms.txt to mention this problem

comment:4 Changed 9 years ago by Philippe Raoult

Has patch: set
Keywords: form_for_model added; for_for_model removed
Owner: changed from nobody to Philippe Raoult
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:5 Changed 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [6148]) Fixed #3922 -- Added a warning that fields passed to form_for_model callback function should not be modified. It's bad for your health. Thanks, Philippe Raoult.

Note: See TracTickets for help on using tickets.
Back to Top