Code

Opened 5 years ago

Closed 4 years ago

#10966 closed (wontfix)

Management module imports project_module, considered evil

Reported by: patrys Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords:
Cc: gonz Triage Stage: Unreviewed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

I think Django's core.management.setup_environ should stop trying to import the project dir as if it was a module.

Currently lines 337-341 read:

# Import the project module. We add the parent directory to PYTHONPATH to
# avoid some of the path errors new users can have.
sys.path.append(os.path.join(project_directory, os.pardir))
project_module = import_module(project_name)
sys.path.pop()

There is no explanation as to why it's useful to treat the project as a module. Also it assumes that:

  • there is no and will never be a python module using the same name in any of the current sys.path directories;
  • the directory name is a valid python module

I see no value in treating a mere container dir as a module. If there is one, it could be clarified in the comment, currently it just states what the code does while not really explaining why.

I actually have projects where the dir only contains settings.py and an empty __init__.py just to keep Django from crashing. If you don't import apps using your project name as a prefix, the __init__.py file is useless and you should be free to use any directory name you want. If you do use the project prefix, Python will already happily parse the top-level __init__.py for you and I see no point in importing it earlier in core.management.

Attachments (2)

project_module_optional.patch (1.4 KB) - added by chad@… 5 years ago.
Patch to make importing of top-level project conditional on presence of init.py
project_module_optional.2.patch (1.4 KB) - added by chad@… 5 years ago.
Patch to make importing of top-level project conditional on presence of init.py (w/o set_trace call :-) )

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by chad@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.0 to SVN

I agree. I would like to use a Django project directory as the top-level directory for a website on the filesystem, but on the other hand I like to use the domain name of a website as its directory name. Domain names are not valid Python identifiers, so I can't have it both ways without hacking Django.

This behavior is also present on trunk.

Changed 5 years ago by chad@…

Patch to make importing of top-level project conditional on presence of init.py

comment:2 Changed 5 years ago by anonymous

  • Has patch set
  • Needs documentation set
  • Needs tests set

Changed 5 years ago by chad@…

Patch to make importing of top-level project conditional on presence of init.py (w/o set_trace call :-) )

comment:4 Changed 5 years ago by patrys

Another problem caused by treating project as a python module is that omitting the "project." prefix seems to work at first but leads unsuspecting users to traps like executing code twice. Example follows:

Assume that the user has "project.app" in INSTALLED_APPS but in some other file uses:

from app import models

If project/app/models.py contains:

print 'this will be executed twice'

...you will of course see two messages being printed as for python project.app.views and app.views are two different modules.

Now imagine instead of printing stuff you had signal.foo.connect() in there - this can lead to serious data corruption due to all signals being fired twice.

comment:5 Changed 5 years ago by patrys

chad@…:

You do not need to explicitly import the project as a module. If your settings.py contains project prefix for at least one module (URLS_CONF, one of the INSTALLED_APPS, etc.), python will already parse the top-level init.py for you. As "old" projects have the project prefix everywhere by default, dropping the explicit import is a backward-compatible change. It's just a matter of documenting the new behavior.

comment:6 Changed 5 years ago by chad@…

I'm open to that. Where would the documentation need to be changed?

comment:7 Changed 5 years ago by gonz

  • Cc gonz added

comment:8 Changed 5 years ago by chad@…

patrys,

Actually, simply removing the __import__ call wouldn't be backwards compatible, because setup_environ has to add the parent directory to sys.path in order for __import__ to work. It then removes this path immediately.

The options seem to be:

  1. Add the path and leave it there, but don't __import__.
  2. Don't add the path and don't __import__.
  3. Add/__import__/remove, but conditionally (per patch).

#1: You end up with at least a wart (an extraneous path on sys.path), and at most a bug (if siblings directories of the project are accidentally imported).

#2: This breaks the current behavior, forcing users to take the extra step of manually adding their project's parent directory to PYTHONPATH before using their project as a module. I think this is a no-go.

#3: I think this is the safest way to go. It exactly preserves current behavior, unless the user takes an explicit step to disable that behavior.

chad

comment:9 Changed 5 years ago by chad@…

BTW, until ever this is fixed, you can work around it by monkey-patching setup_environ:

from django.core import management


def setup_environ(settings_mod, original_settings_path=None):
    pass # copy/paste and customize

management.setup_environ = setup_environ

If you do that then execute_manager will call your customized setup_environ instead of the stock Django one.

comment:10 Changed 4 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

Not sure I see the benefit. django-admin.py creates the project directory as a module. The proposed patch adds a bunch of complication to setup_environ, and the only real gain I can see is that it means you can use 'website.com' as the directory in which you keep your project code. Closing wontfix - please open a discussion on django-dev if you disagree.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.