add decorator which forces django template admin

[alligatortower]

Known exceptions to PR

https://github.com/django/django/blob/b7a17b0ea0a2061bae752a3a2292007d41825814/django/contrib/admin/widgets.py#L245
Any field rendered through this widget wrapper. I have not found a sane way to inject a renderer into this

Dead Ends

Here are some dead ends I went down, preserved here so I feel better about the time spent

overwrite jinja renderer

example:

class Jinja3(Jinja2):
    def get_template(self, template_name):
        template = super().get_template(template_name)
        if [is admin template]:
            raise TemplateDoesNotExist
        return template

problem:

At this point in the code there is no context for whether the template is being rendered inside an admin template. The base admin templates include "admin/" in their path, but the sub-templates (e.g. fields) they render do not. You don't have any more helpful context here than is given to the regex which is already filtering out base admin templates correctly.

However for debugging purposes something like this is helpful for identifying the templates that have fallen into jinja2-land

class JinjaSnitch(Jinja2):
    def get_template(self, template_name):
        print("^^^ Jinja template attempt", template_name)
        template = super().get_template(template_name)
        print(
            "     ~#~#~#~#~#~#~#~#~#~#~#~ jinja template was used  ~#~#~#~#~#~#~#~#~#~#~#~ "
        )
        return template

A loathsome global

example:

from django.template import loader


class Jinja2Custom(Jinja2):
    def render(self, template_name, context, request=None):
        global is_admin
        if is_admin:
            raise TemplateDoesNotExist
        template = self.get_template(template_name)
        return template.render(context, request=request).strip()


old_select = loader.select_template


def select_template(template_name, using=None):
    global is_admin
    if isinstance(template_name, str):
        # imagine a regex here instead
        is_admin = "admin" in template_name
    elif isinstance(template_name, list):
        is_admin = any(["admin" in template for template in template_name])
    return old_select(template_name, using=using)


setattr(loader, "select_template", select_template)

problem

This "works" in contrived local testing. However...
Globals are a code-smell on their own. And in this case I believe this would cause all kinds of nonsense in an async environment.

Notice also the using=None kwarg in the select_template function? That seemed promising, but django does not pass this value from anywhere sane, and in many cases passes no value. It's either a vestigial or overly-hopeful feature.

Keep you in django template land once you find yourself there

example

class WalledEngine(Engine):
    def find_template(self, name, dirs=None, skip=None):
        del dirs
        tried = []
        for loader in self.template_loaders:
            try:
                template = loader.get_template(name, skip=skip, using="django")
                return template, template.origin
            except TemplateDoesNotExist as e:
                tried.extend(e.tried)
        raise TemplateDoesNotExist(name, tried=tried)


class WalledDjangoTemplates(DjangoTemplates):
    def __init__(self, params):
        # overridden to set custom engine
        copy_params = params.copy()
        super().__init__(params)
        options = copy_params.pop("OPTIONS").copy()
        options.setdefault("autoescape", True)
        options.setdefault("debug", DEBUG)
        options.setdefault("file_charset", "utf-8")
        libraries = options.get("libraries", {})
        options["libraries"] = self.get_templatetag_libraries(libraries)
        self.engine = WalledEngine(self.dirs, self.app_dirs, **options)

problem

This was only a half-hearted attempt as mentioned above, django admin rarely passes any information about previously used renderers down to sub templates. It effectively does not matter how the parent template was rendered.

Other Notes

Throughout this whole process it has felt like I was just missing something especially around
https://github.com/django/django/blob/b7a17b0ea0a2061bae752a3a2292007d41825814/django/template/loader.py#L22
(and get_template above that)
Like there was some cleaner way to cut the gordion knot that I just wasn't seeing.

Digging into this problem has soured me a bit in general regarding django admin. There's a lot more competing organization and patterns than I had thought.

[gone]

What does this do?

[alligatortower]

oh
del [attr] is to just my LSP up from complaining about unused attrs. del just removes the variable from local (function) context. When you're defining an actual method in a class its smart enough to ignore self, but not here.

I was annoyed at having to do it at first, but grew to appreciate having a list of unused attrs at the top

[brianedelman]

I don't have time to play around with this locally but it seems like we would also need to override the templates for the admin? Right now they're being rendered with jinja, htmx, alpine in many cases. But maybe I'm just unclear on what this PR is solving

[alligatortower]

Good catch. I forgot to move over the settings change from my test app. I'm pointing DTL at a completely separate template dir. Correct me if this is a bad assumption, but I think it's cleanest to just completely divorce all template customization between the two loaders -- have two sets of customized templates (if we ever even want to customize any of the DTL templates, that is).

[brianedelman]

The only problem I see with this is that now we need to check every time there is a django update if these files have changed. I can't think of a better way to divorce them though given the constraints

[brianedelman]

Do we need to include the dtl_tempaltes dir in this PR as well?

[alligatortower]

In all cases being adequately captured by this PR (not all but hopefully enough):

• The DTL will be used instead of Jinja2
• DTL will look for templates defined locally
• By default (unless we add one), it will never find a locally defined template, because even the base dir it's looking in does not exist
• It will default back to the base django templates

specifically
re django update collisions: I don't think this makes us any more vulnerable to collisions than we would otherwise be
re adding the dir: It'll fail gracefully if the file isn't found: https://github.com/django/django/blob/d1855c4847215f3afe3708736be13388bb6437eb/django/template/loaders/filesystem.py#L20 (I did not include the dir in my test project)
edit: to be more exact, it'll fail gracefully assuming it does eventually find a template. and since the admin does not expect you to have defined any templates locally, we can be confident it'll find a default.

[alligatortower]

Oh sorry, I may have misinterpreted because of the line the comment was attached to -- If you're referring to the methods the decorator is overriding. yes absolutely that is vulnerable to the underlying django updating. I tried to minimize the surface area but it's a fundamentally hacky approach. EXTREMELY HAPPY to throw all of this away in favor of a less fragile approach.

[brianedelman]

No you had it right the first comment, I meant the templates. That's awesome that'll just fail and go back to the default in django so I'm good with that. The methods you mentioned don't seem to scary about being updated too often and us needing to fix