Opened 9 months ago
Closed 8 months ago
#35233 closed Cleanup/optimization (fixed)
Push templates checks down to backend engine classes
Reported by: | Adam Johnson | Owned by: | Giannis Terzopoulos |
---|---|---|---|
Component: | Core (System checks) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently, the three system checks for template settings are individual functions in django.core.checks.templates. This structure leads to some issues:
- The checks are specific to DTL (Django Template Language), but get run for all backends. DTL-specific code gets loaded and run even when not using it, notably the fairly slow
get_template_tag_modules()
. check_for_template_tags_with_the_same_name
is inaccurate because it combineslibraries
from all template backends, so might report an issue when none exists.- The checks are still run when no template backend is configured (
settings.TEMPLATES = []
).
I propose the checks be restructured to live within the engine classes in django.template.backends
, adopting the same pattern used for model and field checks, admin checks, etc.
In practice, this would mean adding a dummy BaseEngine.check()
method, then moving the existing checks into methods called by an overridden DjangoTemplates.check()
. A single function left in django.core.checks.templates
would loop over django.template.engines.all()
to combine results from backends’ check()
methods.
Change History (9)
comment:1 by , 9 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 9 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 months ago
comment:4 by , 8 months ago
Thank you for the pointers Adam! I finally found the time to look into this and I have some questions.
check_for_template_tags_with_the_same_name
is inaccurate because it combineslibraries
from all template backends, so might report an issue when none exists.
Regarding this, am I right in assuming that we want to allow same names for tags across different engines; Should this test pass?
def test_different_backends_may_have_same_tags(self): with self.settings( TEMPLATES=[ {"BACKEND": "django.template.backends.django.DjangoTemplates", "OPTIONS": {"libraries": { "same_tags": "check_framework.template_test_apps.same_tags_app_1" ".templatetags.same_tags"}}}, {"BACKEND": "django.template.backends.OtherBackend", "OPTIONS": {"libraries": { "same_tags": "check_framework.template_test_apps.same_tags_app_2" ".templatetags.same_tags"}}} ] ): self.assertListEqual(check_for_template_tags_with_the_same_name(None), [])
And one more thing, I am noticing that the E001
logic is duplicated:
- in
Engine.__init__
- raisesImproperlyConfigured
(template_backends.test_utils.TemplateUtilsTests.test_backend_improperly_configured
) - and in
check_setting_app_dirs_loaders
(tests undercheck_framework.test_templates.CheckTemplateSettingsAppDirsTest
)
and I am wondering if there are reasons to keep both, or we could remove one of the two? 🤔
comment:5 by , 8 months ago
Regarding this, am I right in assuming that we want to allow same names for tags across different engines; Should this test pass?
Yes, I think so. You can use two DjangoTemplates
backends and it would still be valid.
And one more thing, I am noticing that the E001 logic is duplicated:
Indeed, that is true. The system check was added in #24922 because the exception would only appear at runtime, because the backend would only be created lazily. But by actioning this ticket, we’ll force backends to all be instantiated during checks, which will make the exception appear then. So we should be able to remove E001 as part of this work.
comment:6 by , 8 months ago
Has patch: | set |
---|
Thank you for the explanation. I opened a PR for this PR
comment:7 by , 8 months ago
Patch needs improvement: | set |
---|
comment:8 by , 8 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
The staticfiles finders checks are actually the perfect template:
https://github.com/django/django/blob/35e63b7f2896959d3f1939346fd467af92f0d80f/django/contrib/staticfiles/checks.py#L11-L21
https://github.com/django/django/blob/35e63b7f2896959d3f1939346fd467af92f0d80f/django/contrib/staticfiles/finders.py#L23-L27
https://github.com/django/django/blob/35e63b7f2896959d3f1939346fd467af92f0d80f/django/contrib/staticfiles/finders.py#L74