Deprecate django.contrib.admin.helpers.InlineAdminForm.original_content_type_id

[ILYA]

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 greping 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.

[Omer Katz]

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.

[ILYA]

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)

[Tim Graham]

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.

[ILYA]

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 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?

[Tim Graham]

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.

[ILYA]

I've made initial pull request:
​https://github.com/django/django/pull/3201

[Loic Bistuer]

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.

[Tim Graham]

Loic, are you suggesting that we should modify get_for_model() so it doesn't create ContentTypes for through models? If so, can we create a separate ticket and keep this one focused on deprecating original_content_type_id?

[ILYA]

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.

[ILYA]

I've updated the patch and created new ticket about get_for_model: #23687.

[Tim Graham <timograham@…>]

In 2d75515a4c4fe726e26ebb1a2d2acbc9d047cba6:

Fixed #23444 -- Deprecated django.contrib.admin.helpers.InlineAdminForm.original_content_type_id

[Tim Graham <timograham@…>]

In 7140d4a:

Refs #23444 -- Removed InlineAdminForm.original_content_type_id per deprecation timeline.