Code

Opened 4 months ago

Last modified 5 days ago

#21680 new Cleanup/optimization

Stop supporting models in non-installed apps

Reported by: aaugustin Owned by:
Component: Core (Other) Version: master
Severity: Normal Keywords: app-loading 1.9
Cc: mmitar@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The app registry is explicitly designed to support models outside installed applications.

This doesn't seem particularly useful and makes the code more complicated than necessary.

However, the consequences on backwards compatibility are hairy. If users simply add the missing apps to their INSTALLED_APPS settings, their project could start loading the wrong templates, static files, etc.

This change shouldn't be made lightly.

Attachments (0)

Change History (18)

comment:1 Changed 4 months ago by mitar

  • Cc mmitar@… added

comment:2 Changed 4 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 4 months ago by aaugustin

The 1.7 release notes already warn against this.

comment:4 Changed 4 months ago by aaugustin

This could wreak havoc in Django's own tests, as some create arbitrary model classes.

A good middle ground could be to require that each model:

  • is in an installed application, or
  • has an explicit app label.

"Being in an installed application" means that the application has been registered in the app registry before the model class gets created. This is related to #21719.

comment:5 Changed 4 months ago by aaugustin

I'm going to implement a deprecation path for this.

comment:7 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In 3326a412ccde9f72e22a070a0b4d922048ed2286:

Deprecated importing a model before loading its application.

Refs #21719, #21680.

comment:8 Changed 3 months ago by aaugustin

  • Keywords 1.9 added

This ticket cannot move forward until Django 1.9.

comment:9 Changed 3 months ago by loic84

While discussing #19774 on IRC it appeared that django.contrib.sites.models.get_current_site() which is imported in various places in Django would end up being an issue to anyone who doesn't have django.contrib.sites installed.

If we want to be ready for Django 1.9, we probably should start a deprecation period in 1.7 to move get_current_site() to the package __init__.py.

comment:10 Changed 3 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Severity changed from Normal to Release blocker
  • Status changed from new to assigned

Good point.

comment:11 Changed 3 months ago by aaugustin

  • Has patch set

comment:12 Changed 3 months ago by carljm

The PR for contrib.sites looks good.

It occurs to me that the ideal resolution for this ticket would be one where you could safely import models anytime, but they just wouldn't be available in the app registry or register themselves with the ORM (e.g. attach related managers to other models) until/unless their app were installed. (The general principle being that errors on import, or having to be careful what you import when, is an unfortunate smell due to reliance on import side effects.) I presume you considered this and decided it just wasn't feasible to implement? (I haven't looked at the relevant code in detail recently enough to know.)

comment:13 Changed 3 months ago by aaugustin

No, I didn't consider that. I don't know how complicated it would be. In general I would like to see if we can get away with the strict approach and avoid adding complexity to the app registry.

comment:14 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

In 9ffab9cee1a5bd1a2f6c326ae970d92526f9a304:

Moved RequestSite and get_current_site.

Following the app-loading refactor, these objects must live outside of
django.contrib.sites.models because they must be available without
importing the django.contrib.sites.models module when
django.contrib.sites isn't installed.

Refs #21680. Thanks Carl and Loic for reporting this issue.

comment:15 Changed 3 months ago by aaugustin

  • Has patch unset
  • Severity changed from Release blocker to Normal

This is still open, but postponed until the deprecation completes.

comment:16 Changed 3 months ago by aaugustin

  • Owner aaugustin deleted
  • Status changed from assigned to new

comment:17 Changed 8 weeks ago by aaugustin

#22182 was a duplicate (with a long explanation).

comment:18 Changed 5 days ago by mlavin

I've created a related ticket which notes that this deprecation exposes an issue with the defaults defined in global settings. See #22477.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.