Opened 3 months ago

Closed 7 weeks 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:

  1. 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().
  2. check_for_template_tags_with_the_same_name is inaccurate because it combines libraries from all template backends, so might report an issue when none exists.
  3. 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 Simon Charette, 3 months ago

Triage Stage: UnreviewedAccepted

comment:2 by Giannis Terzopoulos, 3 months ago

Owner: changed from nobody to Giannis Terzopoulos
Status: newassigned

in reply to:  description comment:4 by Giannis Terzopoulos, 2 months ago

Thank you for the pointers Adam! I finally found the time to look into this and I have some questions.

  1. check_for_template_tags_with_the_same_name is inaccurate because it combines libraries 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__ - raises ImproperlyConfigured (template_backends.test_utils.TemplateUtilsTests.test_backend_improperly_configured)
  • and in check_setting_app_dirs_loaders (tests under check_framework.test_templates.CheckTemplateSettingsAppDirsTest)

and I am wondering if there are reasons to keep both, or we could remove one of the two? 🤔

Last edited 2 months ago by Giannis Terzopoulos (previous) (diff)

comment:5 by Adam Johnson, 2 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 Giannis Terzopoulos, 2 months ago

Has patch: set

Thank you for the explanation. I opened a PR for this PR

comment:7 by Mariusz Felisiak, 2 months ago

Patch needs improvement: set

comment:8 by Adam Johnson, 7 weeks ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 7 weeks ago

Resolution: fixed
Status: assignedclosed

In d658a31:

Fixed #35233 -- Moved template engine system checks to backend methods.

Thanks Adam Johnson for reviews.

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