#23444 closed Cleanup/optimization (fixed)
Deprecate django.contrib.admin.helpers.InlineAdminForm.original_content_type_id
Reported by: | ILYA | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
First of all it must be said that at the moment django does not create ContentType objects for through
models.
Moreover, django suggests to remove such objects when trying to syncdb/migrate apps.
But if you try to use your through
model in inlines django will create ContentType object for it.
Consider the example. Every time you visit A
or B
admin pages ContentType is created.
# models.py class A(models.Model): items = models.ManyToManyField('B') class B(models.Model): pass # admin.py class Inline(admin.TabularInline): model = A.items.through class A_Admin(admin.ModelAdmin): exclude = ('items',) inlines = (Inline,) class B_Admin(admin.ModelAdmin): inlines = (Inline,)
The problem is here:
https://github.com/django/django/blob/stable/1.7.x/django/contrib/admin/helpers.py#L274
But the thing is that original_content_type_id
is not used anywhere in 1.7 anymore. I've checked it by grep
ing through all sources.
It was used in 1.6 and lower to show "View on site" links:
https://github.com/django/django/blob/stable/1.6.x/django/contrib/admin/templates/admin/edit_inline/tabular.html#L30
Now this part of template is refactored (while legacy view part remained):
https://github.com/django/django/blob/stable/1.7.x/django/contrib/admin/templates/admin/edit_inline/tabular.html#L30
It can be solved easily by removing several legacy lines.
If I didn't miss something I can make PR with that change.
Change History (16)
follow-up: 2 comment:1 by , 10 years ago
comment:2 by , 10 years ago
Replying to thedrow:
What if someone relied on this attribute being set? Many applications theme their admin app.
I suggest to wrap this in a property with an expedited deprecation process. That is, a DeprecationWarning will be issued.
Yes, that was on my mind too when I was submitting this issue but I've forgotten to mention it. Thanks!
So now we have this code:
if original is not None: # Since this module gets imported in the application's root package, # it cannot import models from other applications at the module level. from django.contrib.contenttypes.models import ContentType self.original_content_type_id = ContentType.objects.get_for_model(original).pk
...and we need to keep original_content_type_id
but prevent ContentType creating (and raise DeprecationWarning).
So smth like this may work (simplified):
if original is not None: from django.utils.functional import cached_property def original_content_type_id(self): import warnings from django.contrib.contenttypes.models import ContentType from django.utils.deprecation import RemovedInDjango18Warning warnings.warn("Use absolute_url attr instead.", RemovedInDjango18Warning) return ContentType.objects.get_for_model(original).pk self.__class__.original_content_type_id = cached_property(original_content_type_id)
follow-up: 4 comment:3 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 1.7 → master |
Deprecation sounds good, but we'd have start the process in 1.8 and I don't think there's any reason to accelerate it.
comment:4 by , 10 years ago
Replying to timgraham:
Deprecation sounds good, but we'd have start the process in 1.8 and I don't think there's any reason to accelerate it.
I'm ready to make PR but there 2 questions.
1
Should I extract original_content_type_id
into the separate InlineAdminForm
method?
The good part is that it will make code slightly more readable.
But what I don't like is that we'll get a bit of inconsistency: for those forms without original
original_content_type_id
property didn't even exist.
On the other side we still modify class that is not very good.
2
I didn't undestand about deprecation warning. Which one should I use - 1.8 or 1.9?
comment:5 by , 10 years ago
I don't think a method is really needed, but you could use an underscore method (_original_content_type_id
) and assign it to original_content_type_id
in __init__()
if you want.
The class to use is RemovedInDjango20Warning
, see the Deprecating a feature guide for details.
comment:6 by , 10 years ago
Has patch: | set |
---|
I've made initial pull request:
https://github.com/django/django/pull/3201
comment:7 by , 10 years ago
IMO InlineAdminForm
is the wrong place for a fix, can't we fix it at the get_for_model()
level?
I didn't check in detail but I think Model._meta.auto_created
can be used to this effect.
comment:8 by , 10 years ago
Loic, are you suggesting that we should modify get_for_model()
so it doesn't create ContentType
s for through
models? If so, can we create a separate ticket and keep this one focused on deprecating original_content_type_id
?
comment:9 by , 10 years ago
Moving it to get_for_model
might be logical. I've checked some details to be sure.
At the moment syncdb/migrate creates CT objects here:
https://github.com/django/django/blob/master/django%2Fcontrib%2Fcontenttypes%2Fmanagement.py
So it just calls app_config.get_models()
and then creates corresponding objects.
Those app CT objects that were not found are suggested to be deleted.
Moving on to get_models.__doc__
:
Returns a list of all installed models. By default, the following models aren't included: - auto-created models for many-to-many relations without an explicit intermediate table, - models created to satisfy deferred attribute queries, - models that have been swapped out.
So may be some other checks in addition to "being a through
model" should be added there.
And one more thing. I doesn't make much sense but some users could use their through
model in admin not only in inlines and therefore they might need its CT object. So that change can affect them in some way.
comment:10 by , 10 years ago
Patch needs improvement: | set |
---|
comment:11 by , 10 years ago
Patch needs improvement: | unset |
---|
I've updated the patch and created new ticket about get_for_model
: #23687.
comment:12 by , 10 years ago
Patch needs improvement: | set |
---|
comment:13 by , 10 years ago
Patch needs improvement: | unset |
---|
comment:14 by , 10 years ago
Summary: | Legacy code unnecessarily creates ContentType object for `through` model when using it in inlines → Deprecate django.contrib.admin.helpers.InlineAdminForm.original_content_type_id |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:15 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
What if someone relied on this attribute being set? Many applications theme their admin app.
I suggest to wrap this in a property with an expedited deprecation process. That is, a DeprecationWarning will be issued.