Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9751 closed (fixed)

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

Reported by: lamby Owned by: gsong
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 lamby 7 years ago.
Rebasing patch against HEAD
06-fix-project_name-location-when-settings-is-a-module.patch (1.2 KB) - added by lamby 7 years ago.
0001-Fix-project_name-location-when-settings-is-a-module.patch
9751-core_management_init-r10599.diff (1.6 KB) - added by gsong 6 years ago.
9751-with-tests.diff (7.5 KB) - added by ericholscher 6 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 7 years ago by lamby

Rebasing patch against HEAD

Changed 7 years ago by lamby

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

comment:1 Changed 7 years ago by lamby

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Rebasing patch against HEAD.

comment:2 Changed 7 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by russellm

  • Component changed from Uncategorized to django-admin.py

comment:4 Changed 6 years ago by alexr

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 6 years ago by alexr

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 6 years ago by gsong

comment:6 Changed 6 years ago by gsong

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 6 years ago by gsong

  • Owner changed from nobody to gsong
  • Status changed from new to assigned

comment:8 Changed 6 years ago by ericholscher

  • Needs tests set

Trying to write tests for this.

Changed 6 years ago by ericholscher

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 6 years ago by ericholscher

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 6 years ago by Alex

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 years ago by gsong

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 6 years ago by Alex

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

comment:13 Changed 6 years ago by anonymous

  • Cc robillard.etienne@… added

comment:14 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 6 years ago by Leo

This fix broke our configuration - documented in #11147.

comment:16 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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