Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#14130 closed (fixed)

Catching ImportError in manage.py considered dangerous

Reported by: Setok Owned by: nobody
Component: Core (Other) Version: 1.1
Severity: Keywords: manage settings import sprintnov13
Cc: lrekucki, bberes Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The basic implementation of manage.py does little else than check for an ImportError on settings. Unfortunately using try-except to check for module existence in Python is not a very good practise. The problem is that it hides any possible import errors that may occur within settings.py. Sure, the error displayed by manage.py does mention that possibility, but only in parenthesis, after bold text about settings.py missing.

Having just spend several hours trying to figure out what was going on I believe that, in balance, it would be better just to let the Python error fall through and for a proper stacktrace to occur. Alternatively try to figure out a way to detect if it was, in fact, the settings module which was missing, or something else. Annoyingly Python's ImportError does not seem to easily have that information available :(

Attachments (2)

fix14130.patch (1.1 KB) - added by mk 4 years ago.
trac-14130.diff (1.1 KB) - added by steph 3 years ago.
Updated the patch and removed an unused import.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by russellm

  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

This is an example where Django is using ImportError to establish existence of a module; in Django 1.2, we added tools to check for existence of a module without importing the module; we should be doing the same here.

comment:2 Changed 4 years ago by lrekucki

  • Cc lrekucki added

comment:3 Changed 4 years ago by bberes

  • Cc bberes added

comment:4 Changed 4 years ago by mk

  • Has patch set

Patch attached.

Changed 4 years ago by mk

Changed 3 years ago by steph

Updated the patch and removed an unused import.

comment:5 Changed 3 years ago by steph

.. just updated the patch (removed an unused import).

comment:6 Changed 3 years ago by steph

  • Keywords sprintnov13 added

comment:7 Changed 3 years ago by ramiro

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 3 years ago by ramiro

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

In [15522]:

Fixed #14130 -- Made manage.py error reporting more useful when the settings.py file triggers import errors (in new projects). Thanks Setok for the report, mk and steph for their work.

comment:9 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.