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 Claude Paroz, 5 years ago

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.

comment:2 by Carlton Gibson, 5 years ago

Easy pickings: set
Owner: changed from nobody to Carlton Gibson
Status: newassigned
Triage Stage: UnreviewedAccepted

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 Carlton Gibson, 5 years ago

Has patch: set

comment:4 by Keryn Knight, 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.

Version 0, edited 5 years ago by Keryn Knight (next)

comment:5 by Carlton Gibson, 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.

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:6 by Keryn Knight, 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 Carlton Gibson, 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. 👍

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 1de9a92:

Fixed #30872 -- Improved unknown command message when settings are manually configured.

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