Opened 10 years ago

Closed 9 years ago

#21680 closed Cleanup/optimization (fixed)

Stop supporting models in non-installed apps

Reported by: Aymeric Augustin Owned by: Aymeric Augustin <aymeric.augustin@…>
Component: Core (Other) Version: dev
Severity: Normal Keywords: app-loading 1.9
Cc: mmitar@… Triage Stage: Accepted
Has patch: yes 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.

Change History (25)

comment:1 by Mitar, 10 years ago

Cc: mmitar@… added

comment:2 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Aymeric Augustin, 10 years ago

The 1.7 release notes already warn against this.

comment:4 by Aymeric Augustin, 10 years ago

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 by Aymeric Augustin, 10 years ago

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

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In 3326a412ccde9f72e22a070a0b4d922048ed2286:

Deprecated importing a model before loading its application.

Refs #21719, #21680.

comment:8 by Aymeric Augustin, 10 years ago

Keywords: 1.9 added

This ticket cannot move forward until Django 1.9.

comment:9 by loic84, 10 years ago

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 by Aymeric Augustin, 10 years ago

Owner: changed from nobody to Aymeric Augustin
Severity: NormalRelease blocker
Status: newassigned

Good point.

comment:11 by Aymeric Augustin, 10 years ago

Has patch: set

comment:12 by Carl Meyer, 10 years ago

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 by Aymeric Augustin, 10 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

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 by Aymeric Augustin, 10 years ago

Has patch: unset
Severity: Release blockerNormal

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

comment:16 by Aymeric Augustin, 10 years ago

Owner: Aymeric Augustin removed
Status: assignednew

comment:17 by Aymeric Augustin, 10 years ago

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

comment:18 by Mark Lavin, 10 years ago

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

comment:19 by Tim Graham <timograham@…>, 9 years ago

In 4548a282a1c64bb2ed414da3d08617d869cd3740:

Removed contrib.sites.models.RequestSite/get_current_site() aliases.

Per deprecation timeline; refs #21680.

comment:20 by Tim Graham, 9 years ago

Aymeric will look at this ticket and determine if there is there more to it than promoting the existing warning to an exception.

comment:21 by Tim Graham, 9 years ago

Also need to verify the documentation for Options.app_label in docs/ref/models/options.txt is accurate after this change.

comment:22 by Aymeric Augustin, 9 years ago

I filed #24312 so as not to lose the idea from comment 12.

comment:23 by Aymeric Augustin, 9 years ago

Patch needs improvement: set

Four tests in the test_runner app are failing.

comment:24 by Aymeric Augustin, 9 years ago

Has patch: set
Patch needs improvement: unset

comment:25 by Aymeric Augustin <aymeric.augustin@…>, 9 years ago

Owner: set to Aymeric Augustin <aymeric.augustin@…>
Resolution: fixed
Status: newclosed

In 1b8af4cfa023161924a45fb572399d2f3071bf5b:

Disallowed importing concrete models without an application.

Removed fragile algorithm to find which application a model belongs to.

Fixed #21680, #21719. Refs #21794.

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