Opened 11 years ago
Closed 10 years ago
#21680 closed Cleanup/optimization (fixed)
Stop supporting models in non-installed apps
Reported by: | Aymeric Augustin | Owned by: | |
---|---|---|---|
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 , 11 years ago
Cc: | added |
---|
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 11 years ago
comment:4 by , 11 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:9 by , 11 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 , 11 years ago
Owner: | changed from | to
---|---|
Severity: | Normal → Release blocker |
Status: | new → assigned |
Good point.
comment:11 by , 11 years ago
Has patch: | set |
---|
PR for contrib.sites: https://github.com/django/django/pull/2214
comment:12 by , 11 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 , 11 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:15 by , 11 years ago
Has patch: | unset |
---|---|
Severity: | Release blocker → Normal |
This is still open, but postponed until the deprecation completes.
comment:16 by , 11 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:18 by , 11 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:20 by , 10 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 , 10 years ago
Also need to verify the documentation for Options.app_label
in docs/ref/models/options.txt
is accurate after this change.
comment:23 by , 10 years ago
Patch needs improvement: | set |
---|
Four tests in the test_runner
app are failing.
comment:24 by , 10 years ago
Has patch: | set |
---|---|
Patch needs improvement: | unset |
comment:25 by , 10 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
The 1.7 release notes already warn against this.