Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#9751 closed (fixed)

project_directory calculated incorrectly when "settings" is a directory (breaks 'startapp')

Reported by: Chris Lamb Owned by: George Song
Component: Core (Management commands) Version: 1.0
Severity: Keywords: startapp, settings, module
Cc: robillard.etienne@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When a Django project's settings is contained in directory-style module instead of the usual "settings.py" file-based module, project_directory (as returned from setup_environ) is calculated incorrectly as "settings", which results in--at least--'startapp' creating new apps inside the settings directory.

Whilst the use of a settings directory is non-standard, it helps when splitting larger or more complicated configurations, such as when settings change depending on the hostname, etc. Indeed, this would be completely transparent to Django if it wasn't parsing the __file__ attribute.

To reproduce:

 % django-admin.py startproject myproject
 % cd myproject
 % mkdir settings
 % mv settings.py settings/__init__.py
 % ./manage.py startapp myapp
 % tree
 |-- __init__.py
 |-- manage.py
 |-- settings
 |   |-- __init__.py
 |   `-- myapp                              # <----
 |       |-- __init__.py
 |       |-- models.py
 |       `-- views.py
 `-- urls.py

Patch attached.

Attachments (4)

0001-Fix-project_name-location-when-settings-is-a-module.patch (1.3 KB) - added by Chris Lamb 8 years ago.
Rebasing patch against HEAD
06-fix-project_name-location-when-settings-is-a-module.patch (1.2 KB) - added by Chris Lamb 8 years ago.
0001-Fix-project_name-location-when-settings-is-a-module.patch
9751-core_management_init-r10599.diff (1.6 KB) - added by George Song 8 years ago.
9751-with-tests.diff (7.5 KB) - added by Eric Holscher 8 years ago.
Basic patch, removed the sys.modules import, and it seems to work without all that split() stuff, but if there's a reason that was there, feel free to add it back.

Download all attachments as: .zip

Change History (20)

Changed 8 years ago by Chris Lamb

Rebasing patch against HEAD

Changed 8 years ago by Chris Lamb

0001-Fix-project_name-location-when-settings-is-a-module.patch

comment:1 Changed 8 years ago by Chris Lamb

Rebasing patch against HEAD.

comment:2 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:3 Changed 8 years ago by Russell Keith-Magee

Component: Uncategorizeddjango-admin.py

comment:4 Changed 8 years ago by Alex Robbins

06-fix-project_name-location-when-settings-is-a-module.patch does fix the bug. It also breaks 37 of the admin_scripts regression tests. Somehow it causes the spawned admin commands to die with a "TypeError: relative imports require the 'package' argument" traceback.

comment:5 Changed 8 years ago by Alex Robbins

Looks like the conditional the patch added (if settings_name == "init") is being excecuted during the tests, even though I don't think they are using a settings directory. Not exactly sure why that is.

Changed 8 years ago by George Song

comment:6 Changed 8 years ago by George Song

My patch fixes a few problems:

  1. get_commands() was importing project settings each time it's called. That's not necessary.
  2. get_commands() was actually passing the project package instead of settings module/package to setup_environ().
  3. In setup_environ(), check to see if settings module is a package or module by checking to see if its file contains init.py or not. Not sure if this works for Jython as I don't know how Jython filenames work.

comment:7 Changed 8 years ago by George Song

Owner: changed from nobody to George Song
Status: newassigned

comment:8 Changed 8 years ago by Eric Holscher

Needs tests: set

Trying to write tests for this.

Changed 8 years ago by Eric Holscher

Attachment: 9751-with-tests.diff added

Basic patch, removed the sys.modules import, and it seems to work without all that split() stuff, but if there's a reason that was there, feel free to add it back.

comment:9 Changed 8 years ago by Eric Holscher

All tests pass, and the regression test (first at the top of that patch) fails on current trunk and passes with the patch.

comment:10 Changed 8 years ago by Alex Gaynor

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:11 Changed 8 years ago by George Song

The reason for using sys.modules lookup instead of import_module() was to prevent the setting module from unnecessarily being imported again, since it would have to have been imported by this point.

comment:12 Changed 8 years ago by Alex Gaynor

import doesn't redo an imoprt if it's already in sys.modules.

comment:13 Changed 8 years ago by anonymous

Cc: robillard.etienne@… added

comment:14 Changed 8 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [10751]) Fixed #9751: admin scripts now calculate the project directory correctly when the settings module is a directory with an init.py. Thanks to Eric Holscher.

comment:15 Changed 8 years ago by Leo Shklovskii

This fix broke our configuration - documented in #11147.

comment:16 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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