Hi Preston, Thanks for working on this ticket — and for bearing with my changes :-)
> On 28 déc. 2014, at 06:30, Preston Timmons <[email protected]> wrote: > > ## 1. Add new template loader apis > > I tried to solve this patch without changing the template loader apis, but I > eventually decided this was inevitable for two reasons: > > 1. The loaders don't implement a consistent api. For instance, the > filesystem and app loaders define a get_template_source method that is > used elsewhere for debug information, whereas the egg and cached loaders do > not. The cached loader implements load_template but not load_template_source. Even if it isn’t consistently implemented, the loader API is documented: https://docs.djangoproject.com/en/1.7/ref/templates/api/#using-an-alternative-template-language <https://docs.djangoproject.com/en/1.7/ref/templates/api/#using-an-alternative-template-language> To sum up: load_template_source(template_name) —> (template_string, template_origin) load_template(template_name) —> (template, template_origin) The behavior is loosely defined; anything that matches the signature is fine. template_origin is especially ill-defined. Are you proposing to change the API outright? Or are you planning to implement a deprecation path for existing third-party template loaders? > 2. The loaders need to be able to skip templates that have already been > extended, but adding a new argument to load_template is difficult to do in a > backwards-compatible way. At first sight adding a “skip” optional argument to Engine.get_template, Engine.find_template, BaseLoader.__call__ and BaseLoader.load_template may work. Could you explain where the difficulty lies? > This led me to the following loader api: > > ### Loader API > > Loader.get_template_sources > > A method that yields all paths the loader will look at for a given template > name. For example, if the filesystem loader receives "index.html" as an > argument, this yields the full path of "index.html" for each directory on the > search path. The cached loader yields tuples of (loader_class, template_paths) > for each configured loader. > > This method already exists for the filesystem and app loaders. This patch > implements it for all loaders. How would a loader that loads templates from a database implement this method? It seems to me that the proper way to define such a method is “yields some opaque objects that implement the following API: …” I suspect that API would be quite similar to the Origin API — maybe an idealized version if the current version doesn’t suffice. > Loader.get_internal > > Returns the contents for a template given a ``source`` as returned by > ``get_template_sources``. > > This is where a filesystem loader would read contents from the filesystem, > or a database loader would read from the database. If a matching template > doesn't exist this should raise a ``TemplateDoesNotExist`` error. Attaching this method to the objects yielded by get_template_sources would provide better encapsulation than attaching it to the Loader itself. > BaseLoader.get_template > > Returns a ``Template`` object for a given ``template_name`` by looping > through results from ``get_template_sources`` and calling ``get_internal``. > This returns the first matching template. If no template is found, > ``TemplateSyntaxError`` is raised. > > This method takes an optional ``skip`` argument. ``skip`` is a set > containing template sources. This is used when extending templates to > allow enable extending other templates of the same name. It's also used > to avoid recursion errors. > > In general, it will be enough to define ``get_template_sources`` and > ``get_internal`` for custom template loaders. ``get_template`` will > usually not need to be overridden. With my suggestions, skip would become an iterable of opaque objects. These objects would have to be hashable and implement equality. Your design makes sense. My main suggestion is to fit it in the current APIs. I would try to reuse load_template instead of introducing get_template and to hook to a revamped Origin API. The Origin API was documented in Django 1.7; changing it it less disruptive than changing the Loader API. Did you try this, and if you did, why didn’t it work? > ## 2. Update the extends tag > > The new extends tag keeps track of which sources were tried in the local > context. These sources are passed to the loader ``get_template`` method as > the ``skip`` argument. In doing so, the extends tag never extends the same > source file twice. This enables recursive extending, and also avoids > filesystem recursion errors when extending is truly circular. > > The main caveat here is that I changed Template.origin to always be available > on the template--not just when TEMPLATE_DEBUG is true. Without this, the > extends tag has no way to know the source path of the current template. > > I think this change is okay, but I don't know the original reasons for > excluding this in the first place. It could be simply because there was no > use-case presented outside of debug. Yes, origin was a debug-only API for performance reasons. Most of Django's template engine dates back to 2006. Back then when performance was a different story from what it i are today, due to improvements in hardware, improvements in CPython and availability of PyPy. > ## 3. Debug api > > Django displays a template postmortem message when a matching template isn't > found. This is a linear list grouped by template loader. This api is currently > supported only by the filesystem and app directories loaders. My general idea is that Django should define generic debugging APIs that all template backends, built-in or third-party, can implement. The debugging API when a template isn’t found shouldn’t know about template loaders because they’re specific to the Django Template Language. > (…) > > ### The approach > > Rather than trying to rerun template engines or loaders, this patch passes in > debug information as part of ``TemplateDoesNotExist``. This is done by > updating > the multiple places where Django catches ``TemplateDoesNotExist`` exceptions > and silently throws them away. Instead of simply silencing them, this patch > accumulates which templates failed and includes them in subsequent > ``TemplateDoesNotExist`` exceptions that are raised. > > This information is kept as a list of tuples saved to a ``tried`` attribute of > the exception. That makes a lot of sense. django.template.TemplateDoesNotExist is Django’s canonical API for dealing with these errors. For example Django’s jinja2 backend reraises jinja2’s TemplateNotFound as TemplateDoesNotExist. > Here's a somewhat complex example of this attribute when using multiple > loaders. It extends through multiple loaders, but fails when no more matching > templates exist: > > [ > # Note: (loader, path, status) > (filesystem.Loader, "/path/to/fs/base.html", "Found") > (filesystem.Loader, "/path/to/fs/base.html", "Skipped") > (app_directories.Loader, "/path/to/app/base.html", "Found") > (filesystem.Loader, "/path/to/fs/base.html", "Skipped") > (filesystem.Loader, "/path/to/app/base.html", "Skipped") > (app_directories.Loader, "/path/to/app2/base.html", "Source does not > exist") > ] This API is still tied to the Django Template Language. I would prefer a generic list of (“template identifier”, “loading mechanism”, “reason for failure”) that every backend could implement. Furthermore, I’m not sure what this list represents. Is it a “template call stack” i.e. the chain of templates included up to the point where loading a template failed? Or is it a list of ways the template backend tried to load the failing template before giving up? I think the list is intended to be the latter but your complex example below makes me think it may be the former. > ### Multiple engine support > > I'll publish an updated pull request once Aymeric's multiple template engines > branch lands. At that time, I think the ``tried`` attribute will change to > this: > > [ > # (loader, path, status) > (engine, filesystem.Loader, "/path/to/fs/base.html", "Found") > (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped") > (engine, app_directories.Loader, "/path/to/app/base.html", "Found") > (engine, filesystem.Loader, "/path/to/fs/base.html", "Skipped") > (engine, filesystem.Loader, "/path/to/app/base.html", "Skipped") > (engine, app_directories.Loader, "/path/to/app2/base.html", "Source does > not exist") > ] Adding the engine to the API makes sense, however my suggestion to make the rest of the data more generic remains. > ## Conclusion > > Overall, I'm pretty happy with this new api. I have a few concerns though, > which are as follows: > > 1. This patch deprecates the old loader apis: Loader.load_template, > Loader.load_template_sources, etc. I think this is good, but there's > already > a lot of deprecations happening with the multiple engine branch. Yes, you need a better argument than “difficult to do in a backwards-compatible way” :-) > 2. The origin api needs a design decision. I made that available regardless of > the debug setting. Also, it sounds like Aymeric may be doing some work to > refactor this api. I agree with making it always available. The real question is about the API itself. We documented the existing concrete implementations in Django 1.7 but they’re inconsistent and their purpose is still unclear. https://docs.djangoproject.com/en/1.7/ref/templates/api/#template-origin > 3. The debug api is also something Aymeric is scheduled to work on. Aymeric, > if you've got something better in mind I've no problem waiting for that > to be > in place. I was mainly thinking about providing tracebacks for errors in templates. Jinja2 has fancy integration with Python’s traceback system. Django emulates it in its debug view with exc.django_template_source and get_template_exception_info. That’s independent from template loading exceptions. -- Aymeric. -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/77ECAAEA-352F-4B9B-B6BB-C6F13B8D1BA3%40polytechnique.org. For more options, visit https://groups.google.com/d/optout.
