Opened 4 years ago

Closed 3 years ago

Last modified 21 months ago

#19774 closed Cleanup/optimization (fixed)

contentypes generic module has core functionality plus admins-specific one

Reported by: Ramiro Morales Owned by: Ramiro Morales
Component: contrib.contenttypes Version: master
Severity: Normal Keywords: dependency generic contenttypes
Cc: loic@…, Simon Charette 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 (last modified by Ramiro Morales)

(Copied and adapted from https://code.djangoproject.com/ticket/16368#comment:16)

Consider a project with a foo and a bar apps listed on INSTALLED_APPS:

  • foo uses generic foreign keys, with the following models.py:
    from django.db import models
    from django.contrib.contenttypes.models import ContentType
    from django.contrib.contenttypes import generic
    
    class TaggedItem(models.Model):
        tag = models.SlugField()
        content_type = models.ForeignKey(ContentType)
        object_id = models.PositiveIntegerField()
        content_object = generic.GenericForeignKey()
    
  • bar has a Site model:
    from django.db import models
    
    class Site(models.Model):
        name = models.CharField(...
        # ...
    
  • Neither the admin nor sites Django apps are being used.

This causes the user's Site model to be overridden and masked by Django sites framework's one.

This is because the django/contrib/contenttypes/generic.py module contain both the definitions of the model-related generic stuff (GenericForeignKey, etc.) AND the admin app-related specialized inlines (GenericInlineModelAdmin, GenericStackedInline, GenericTabularInline.)

Maybe it's time we move the latter ones from django.contrib.contenttypes.generic to, say, a new generic_admin (or admin_tools?) on django.contrib.contenttypes?. Of course this would need a deprecation process.

Change History (15)

comment:1 Changed 4 years ago by wim@…

Keywords: dependency generic contenttypes added
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:2 Changed 4 years ago by Ramiro Morales

Description: modified (diff)
Owner: changed from nobody to Ramiro Morales
Status: newassigned

comment:3 Changed 3 years ago by loic84

Cc: loic@… added

comment:4 Changed 3 years ago by Simon Charette

As discussed on IRC with ramiro and loic splitting the generic module into fields and admin and deprecating the former should fix the issue.

I couldn't reproduce the Site cloaking with you described example against 1.6 and master. The only way I managed to reproduce is by having a sites app, just like it's described in #16368.

comment:5 Changed 3 years ago by Simon Charette

Cc: Simon Charette added

comment:6 Changed 3 years ago by Simon Charette

Has patch: set
Needs documentation: set
Patch needs improvement: set

@ramiro, I created a PR missing release notes and documentation adjustment. I'll try to work on it in the new few days but if you want to pick it up from here feel free to do it.

Last edited 3 years ago by Simon Charette (previous) (diff)

comment:7 Changed 3 years ago by Simon Charette

Patch needs improvement: unset

I adjusted the documentation and added a release note. The PR should be ready for review.

comment:8 Changed 3 years ago by loic84

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

Looks good to me, I'm pretty happy with the forms.py, fields.py, admin.py split up.

comment:9 Changed 3 years ago by Carl Meyer

Looks good to me; good idea to remove everything from that module and just deprecate the module, that's a lot simpler than the previous approach.

I won't claim to have reviewed all the minor changes to docs and tests in detail, but I assume if you've run the tests and they are warning-free that that was all done correctly. Looks good to me!

comment:10 Changed 3 years ago by Carl Meyer

(I did make a couple comments on wording of docs and error messages.)

comment:11 Changed 3 years ago by Ramiro Morales

Simon,

I had some unfinished work in that direction and was a bit uncomfortable about still having the formset definition in the new fields module. Good call on creating forms.py

Please go ahead, my review didn't find anything.

comment:12 Changed 3 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 10e3faf191d8f230dde8534d1c8fad8c8717816e:

Fixed #19774 -- Deprecated the contenttypes.generic module.

It contained models, forms and admin objects causing undesirable
import side effects. Refs #16368.

Thanks to Ramiro, Carl and Loïc for the review.

comment:13 Changed 21 months ago by Tim Graham <timograham@…>

In 3b89d2d540a96ffd33c294b576147ff800b16632:

Removed contrib.contenttypes.generic per deprecation timeline; refs #19774.

comment:14 Changed 21 months ago by Tim Graham <timograham@…>

In 737cd4ff3decfd3e709885d219e70d8395305965:

[1.8.x] Clarified contrib.contenttypes.generic deprecation; refs #19774.

comment:15 Changed 21 months ago by Tim Graham <timograham@…>

In 4df91d05e8ffca15f58d09d9fd0f0ae16891565b:

[1.7.x] Clarified contrib.contenttypes.generic deprecation; refs #19774.

Backport of 737cd4ff3decfd3e709885d219e70d8395305965 from stable/1.7.x

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