[Django] #30872: ManagementUtility.fetch_command prints "No Django settings specified." even if they are.
Django
if they are.
-------------------------------------+-------------------------------------
Reporter: Keryn | Owner: nobody
Knight |
Type: | Status: new
Cleanup/optimization |
Component: Core | Version: master
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`fetch_command(...)` currently
[https://github.com/django/django/blob/7d1bf29977bb368d7c28e7c6eb146db3b3009ae7/django/core/management/__init__.py#L226
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
[https://github.com/django/django/blob/7d1bf29977bb368d7c28e7c6eb146db3b3009ae7/django/core/management/__init__.py#L351
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30872>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:1>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* status: new => assigned
* owner: nobody => Carlton Gibson
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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.)
[https://github.com/django/django/pull/11903 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:2>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:3>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Keryn Knight):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:4>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:5>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Keryn Knight):
> 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]
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:6>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
> 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. 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:7>
Django
if they are.
-------------------------------------+-------------------------------------
Type: | Gibson
Component: Core (Management | Version: master
commands) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1de9a92295af87846e810a40fbb9ceb6b8d3e773" 1de9a92]:
{{{
#!CommitTicketReference repository=""
revision="1de9a92295af87846e810a40fbb9ceb6b8d3e773"
Fixed #30872 -- Improved unknown command message when settings are
manually configured.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30872#comment:8>