Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive for debug_toolbar.W006? #1550

Closed
leon-matthews opened this issue Dec 15, 2021 · 7 comments · Fixed by #1556
Closed

False positive for debug_toolbar.W006? #1550

leon-matthews opened this issue Dec 15, 2021 · 7 comments · Fixed by #1556

Comments

@leon-matthews
Copy link

After upgrading to v3.2.3 I get the system warning W006, and an advisement to set APP_DIRS=True for at least one of my template settings.

Shouldn't that check also be looking at the value of loaders in order to avoid false warnings?

For example, the project in question had the following template settings:

TEMPLATES = [{
    'BACKEND': 'django.template.backends.django.DjangoTemplates',
    'DIRS': [SITE_ROOT / 'templates'],
    'OPTIONS': {
        'builtins': [
            'django.templatetags.static',
            'common.templatetags.builtins',
        ],
        'context_processors': [
            'common.context_processors.constants',
            'django.template.context_processors.debug',
            'django.template.context_processors.request',
            'django.template.context_processors.media',
            'django.contrib.auth.context_processors.auth',
            'django.contrib.messages.context_processors.messages',
        ],
        'loaders': [
            'django.template.loaders.filesystem.Loader',
            'django.template.loaders.app_directories.Loader',
        ],
    },
}]

Django Debug Toolbar works wonderfully with these settings. My understanding is that the app_directories loader is able to find the templates that the toolbar needs. The W0006 warning seems to be a false postive here (but to be honest, it's been a couple of years since I created this config).

As a quick experiment I tried just deleting the loaders option and adding the suggested APP_DIRS setting. It resulted in a key error for 'django' during deployment while trying to run migrate - so I've reverted back to v3.2.2 for now.

@tim-schilling
Copy link
Contributor

I was able to confirm that the example app can run with APP_DIRS set to False, but with the django.template.loaders.app_directories.Loader loader specified.

You can silence this check by making use of the SILENCED_SYSTEM_CHECKS setting.

@matthiask
Copy link
Member

We could make the check a bit smarter by inspecting ...["OPTIONS"]["loaders"].

@matthiask
Copy link
Member

I attempted to do this and discovered, that Django automatically adds the cached loader since 1.11 when "debug" is False:
django/django@277fe2e

So, there's no good reason to specify "loaders" except when you really want to add a custom loader of your own. It's even worse than NOT specifying it, since you don't get the automatic advantage of caching used templates.

This only affected us since we have been cargo culting our settings for years and years now.

I think it's fine to require an addition to SILENCED_SYSTEM_CHECKS if you really want to specify your own loaders.

I have a new suggestion to make: We could add the hint that people may just want to drop "loaders" from their configuration and use "APP_DIRS": True (or False) instead 😄

@tim-schilling
Copy link
Contributor

Thanks for doing the research on that!

I have a new suggestion to make: We could add the hint that people may just want to drop "loaders" from their configuration and use "APP_DIRS": True (or False) instead smile

It makes sense to include the information for devs to make a choice from. It's only a warning check and doesn't break anything so I'm in agreement that the requirement of needing to use SILENCED_SYSTEM_CHECKS is reasonable.

@leon-matthews
Copy link
Author

This only affected us since we have been cargo culting our settings for years and years now.

You may be right about this, it looks like it was the cause of my problem too. I found the time to look into the error I was getting at deployment time and it was related to us manually wrapping the template loaders in the cached loader for production (obviously no longer needed). Deleting all template loader settings resolved both the debug_toolbar warning and the error.

I apologise if my issue was in fact the red herring! Thank you, everybody.

@matthiask
Copy link
Member

Don't worry, there definitely isn't a need to apologize.

@tim-schilling
Copy link
Contributor

I attempted to do this and discovered, that Django automatically adds the cached loader since 1.11 when "debug" is False:
django/django@277fe2e

@matthiask it looks like that loader isn't included when debug = True, so it shouldn't matter for the toolbar.

tim-schilling added a commit to tim-schilling/django-debug-toolbar that referenced this issue Dec 18, 2021
tim-schilling added a commit that referenced this issue Dec 18, 2021
* Don't raise W006 warning when app loader is specified.

Fixes #1550

* Update docs/changes.rst

Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
nijel added a commit to WeblateOrg/weblate that referenced this issue Dec 20, 2021
Django now does caching by default with disabled debug, so
there is no need ot do that as well.

See jazzband/django-debug-toolbar#1550
pbanaszkiewicz added a commit to pbanaszkiewicz/amy that referenced this issue Jan 22, 2022
https://django-debug-toolbar.readthedocs.io/en/latest/checks.html
jazzband/django-debug-toolbar#1550 (comment)

Based on the discussion in the linked thread, loader configuration was
replaced with `"APP_DIRS": True`.

Django adds required loaders by default, so there was
no need to specify them directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants