Opened 5 years ago
Closed 5 years ago
#30872 closed Cleanup/optimization (fixed)
ManagementUtility.fetch_command prints "No Django settings specified." even if they are.
Reported by: | Keryn Knight | Owned by: | Carlton Gibson |
---|---|---|---|
Component: | Core (Management commands) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
fetch_command(...)
currently does the following:
if os.environ.get('DJANGO_SETTINGS_MODULE'): # If `subcommand` is missing due to misconfigured settings, the # following line will retrigger an ImproperlyConfigured exception # (get_commands() swallows the original one) so the user is # informed about it. settings.INSTALLED_APPS else: sys.stderr.write("No Django settings specified.\n")
which doesn't account for settings being set via a UserSettingsHolder
by doing settings.configure(...)
But the parent execute
method correctly checks if settings.configured:
I've not checked deeply, but I don't think the intent or outcome depends specifically on the LazySettings
having been configured via a Settings
through a named module import, and it would seem that if settings.configured:
could/should apply here too.
Change History (8)
comment:1 by , 5 years ago
comment:2 by , 5 years ago
Easy pickings: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
Hi Keryn.
startproject
and startapp
(at least) can bypass that if settings.configured
check in execute()
that you link, so we can get to fetch_command()
without them in play.
As it is, I take re-triggering of the You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure()
error to be targeting the usual case. But, I think you're right, the message is wrong: No Django settings specified.
— you might well have manually called settings.configure()
. (So a small clean up there.)
PR.
comment:3 by , 5 years ago
Has patch: | set |
---|
comment:4 by , 5 years ago
Apologies, I've clearly not explained well enough, because whilst the PR makes more sense now for what the code does, it's not what I was driving at.
Whether or not the if settings.configured:
check has been performed previously (wrt to having been run via django-admin to do things like startproject
or whatever), that check seems correct, where the check for the enrivonment variable may not be (ignoring that passing --settings
may ultimately set the env var anyway via handle_default_options
etc?).
Consider, the default manage.py
, trimmed down to the minimum:
import os import sys os.environ.setdefault('DJANGO_SETTINGS_MODULE', 't30872.settings') from django.core.management import execute_from_command_line execute_from_command_line(sys.argv)
using that to do something like python manage.py invalid_command
should give back something like:
Unknown command: 'invalid_command' Type 'manage.py help' for usage.
However, a simple change:
import os import sys from django.conf import settings settings.configure( DEBUG=True, INSTALLED_APPS=(), SECRET_KEY='30872' ) from django.core.management import execute_from_command_line execute_from_command_line(sys.argv)
and re-running, gets this output:
No Django settings specified. Unknown command: 'invalid_command' Type 'manage.py help' for usage.
As far as I'm aware, ignoring TZ shenanigans, whether settings come via a module and are internally a Settings
, or are manually configured and come as UserSettingsHolder
is an implementation choice, and ultimately if settings.configured:
ought to suffice for testing whether some form of bootstrapping has been provided (because ultimately it just checks _wrapped
, which is one of the aforementioned class instances).
Ultimately what I'm trying to suggest is that the occurance of the stdout No Django settings specified
at all is the problem, and not the messaging itself, which is currently vague enough to be correct.
As a corollary, the only other time I can see DJANGO_SETTINGS_MODULE
being referenced in a similar way (searching only django.core.management
) is in compilemessages, where again it's used as a stand-in for testing if settings are usable.
comment:5 by , 5 years ago
As I read it, the check is to catch the (I'd guess) common enough, error of providing a DJANGO_SETTINGS_MODULE, maybe via --settings
, but misspelling it or such. Then we want to re-trigger the settings error (which is swallowed twice before). (This is also why we can't just check settings.configured
, because that'll be False
but not re-trigger the error.
The else
is for the other (I'd guess) common error of having an app command, but forgetting to provide settings, so it doesn't get found.
Then we have the (how common?) case of using settings.configure()
with execute_from_command_line()
and misspelling a command (despite autocomplete()
) and then getting a slightly misleading message. For that we could do something like this:
diff --git a/django/core/management/__init__.py b/django/core/management/__init__.py index adc7d173eb..cf6b60c93e 100644 --- a/django/core/management/__init__.py +++ b/django/core/management/__init__.py @@ -229,7 +229,7 @@ class ManagementUtility: # (get_commands() swallows the original one) so the user is # informed about it. settings.INSTALLED_APPS - else: + elif not settings.configured: sys.stderr.write("No Django settings specified.\n") possible_matches = get_close_matches(subcommand, commands) sys.stderr.write('Unknown command: %r' % subcommand)
That passes the existing tests.
Is it what you're after? If so I can write a test case for it on Tuesday.
comment:6 by , 5 years ago
This is also why we can't just check settings.configured, because that'll be False but not re-trigger the error.
I think that's the crux of what I wasn't understanding. I'm guessing that always testing for settings.INSTALLED_APPS (ie: without being a in conditional branch) is some other use-case that won't work for all scenarios.
Then we have the (how common?) case of using settings.configure() with execute_from_command_line() and misspelling a command (despite autocomplete()) and then getting a slightly misleading message [...]
Yeah, I fully appreciate that it's an edge-case, and I'm certainly not going to push hard on it being done (it's been there for however many years already), and you definitely don't need to commit to a specific day(!) on which to do it, but it would seem that what you've proposed would quash my quibble :) [minor aside: I'd wager the usage of autocomplete is also vanishingly small, given it requires additional steps]
comment:7 by , 5 years ago
I'd wager the usage of autocomplete is also vanishingly small...
Yes, quite possibly — by the time we're in settings.configure()
land we're well out there... 😀
I updating the PR. Thanks for the report, and for your patience in explaining it to me. 👍
This is some sensible part of code which was altered a few times already. I guess the test suite should tell you if you can use
settings.configured
in that line.