Opened 9 years ago
Last modified 13 days ago
#27157 assigned Bug
AdminForm may crash if model_admin argument is None
| Reported by: | tpazderka | Owned by: | Sanghyuk Jeong |
|---|---|---|---|
| Component: | contrib.admin | Version: | 1.10 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
When using AdminForm without the keyword argument model_admin which defaults to None, we get AttributeError by calling get_empty_value_display without checking for the model_admin type.
This seems to be also true for AdminReadonlyField class.
Change History (6)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
This is just a stub as our class is not based on BaseFormset but rather reimplements its methods...
I am not sure if this is sufficient. If not, I will try to write more complete example.
class ContactFormset(BaseFormset): @property def forms(self): """ Iterates through forms and returns them wrapped by `AdminForm`. """ for form, datatype in zip(self._forms, self.get_types()): yield AdminForm(form, (self.get_fieldset(datatype), ), {}, self.get_readonly_fields(datatype))
The last line has to be replaced by:
yield AdminForm(form, (self.get_fieldset(datatype), ), {}, self.get_readonly_fields(datatype), model_admin=ModelAdmin(MojeidUser, AdminSite))
in order to work in django 1.9 and later.
comment:3 by , 9 years ago
| Summary: | AdminForm model_admin cannot be None → AdminForm may crash if model_admin argument is None |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
I guess, for example, we need to account for model_admin=None in a place like AdminReadonlyField. It's not so clear to me why model_admin is an optional argument (see bcd9482a2019158f4580c24cd50ee8bfae9b2739), perhaps for backwards compatibility. A better resolution might be a deprecation path toward making those arguments required unless there's argument against it. In your case, if you using that formset for the admin, then a possibility would be to declare the formset in ModelAdmin.get_changelist_formset() so you can access to the ModelAdmin class (self).
comment:4 by , 9 months ago
model_admin can't be required because it is set to None in django/contrib/auth/admin.py for use on the change password form: https://github.com/django/django/blob/90429625a85f1f77dfea200c91bd2dabab57974f/django/contrib/auth/admin.py#L207. However, that form doesn't use any readonly fields, so it doesn't exhibit this issue.
I realize this is not intended to be a public API but it would be nice to be able to use this for rendering properly stylized custom pages in the admin site.
comment:5 by , 13 days ago
I'd like to work on this ticket.
As Tim suggested in comment:3, I'm taking the deprecation path approach rather than simply adding None guards.
Changes:
AdminForm.__init__andAdminReadonlyField.__init__now emitRemovedInDjango70Warningwhenmodel_admin=None, with a fallback toAdminSite.empty_value_display(default"-") so existing code doesn't crash immediately.AdminReadonlyField.get_admin_urlgracefully handlesmodel_admin=Noneby passingcurrent_app=Nonetoreverse().- Fixed
django/contrib/auth/admin.py(the change password form) to passmodel_admin=selfinstead of relying on theNonedefault, as noted in comment:4 by Shawn Zivontsis.
This sets up a clean path toward making model_admin a required argument in Django 7.0.
PR: https://github.com/django/django/pull/20689
comment:6 by , 13 days ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
Can you give some example code that causes this error?
AdminFormisn't a public API, and I'm not sure if you're calling it directly or triggering the problem some other way.