Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#17199 closed Bug (invalid)

Base Template Loader should return the origin

Reported by: German M. Bravo Owned by: nobody
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: German M. Bravo, bmihelac@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've been trying to get the origin from the loaders, but the origin was lost when BaseLoader didn't return it. I believe it should return origin... any thoughts? I'm attaching a patch.

Attachments (1)

#17199-template_loader_origin.diff (1.5 KB ) - added by German M. Bravo 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by German M. Bravo, 12 years ago

Cc: German M. Bravo added
Component: UncategorizedTemplate system
Has patch: set
Type: UncategorizedBug

by German M. Bravo, 12 years ago

comment:2 by German M. Bravo, 12 years ago

I'm now not sure if template loaders should return the tempalte and the origin, or the template and the display_name. I guess they should return the origin instead, as it has more information... My doubt rises from the fact BaseLoader returns display_name, but seemingly, cached template loader returns origin when load_template() fails to find the template... My patch returns display_name (from origin.name) as it seems to be consistent with all the other load_template() methods.

Last edited 12 years ago by German M. Bravo (previous) (diff)

comment:3 by Aymeric Augustin, 12 years ago

Resolution: needsinfo
Status: newclosed
Version: SVN

Per the docs:

Custom Loader classes should inherit from django.template.loader.BaseLoader and override the load_template_source() method, which takes a template_name argument, loads the template from disk (or elsewhere), and returns a tuple: (template_string, template_origin).

The load_template() method of the Loader class retrieves the template string by calling load_template_source(), instantiates a Template from the template source, and returns a tuple: (template, template_origin)


I hope this clarifies the expected behavior. With this new information, could you explain what Django does wrong exactly, and provide some steps to reproduce the issue -- or upload a test case? Thanks!

comment:4 by German M. Bravo, 12 years ago

Using TEMPLATE_DEBUG = True (so that origin is returned, otherwise origin is never actually returned, see make_origin() in django.template.loader), if I do: template, origin = loader.find_template('mytemplate.html'), it does not return origin (this is because load_template() in BaseLoader explicitly returns None instead of the origin). Same thing with the load_template() in the "cached" loader. It returns the tuple (template, None).

After reading that bit of documentation, I can see load_template() doesn't even remotely return (as it should) a template_origin, but a string with the display_name (and only if the template is not found). I could also make a new patch that would make django consistent with said behavior in the documentation, however, BaseLoader.load_template() currently already returns display_name instead of the origin, and simply changing that could break some things.

In my opinion, as I said first, load_template() should always return origin (as stated in the documentation) and not display_name... and only when the template is not found. Whether it should always return origin (even if TEMPLATE_DEBUG is not enabled) is yet another dilemma, as the documentation never states load_template() returns template_origin *only* if TEMPLATE_DEBUG is enabled.

comment:5 by German M. Bravo, 12 years ago

Resolution: needsinfo
Status: closedreopened

I'm reopening because I truly see this being wrong in some way... or I might be. After further digging, it truly seems there are some inconsistencies, and I need the be able to get origin of the template while using django.template.loader.find_template(), which I currently can't, the way django code is now.

comment:6 by bmihelac, 12 years ago

Cc: bmihelac@… added

comment:7 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedDesign decision needed

comment:8 by Aymeric Augustin, 11 years ago

Resolution: invalid
Status: reopenedclosed

#16096 (filed by a core dev) suggests that origin is purely for debug puposes. That would explain why it's set only when the template isn't found and when TEMPLATE_DEBUG = True.

As far as I can tell, you're trying to use the origin returned by BaseLoader.find_template() for much more than its purpose.

Wouldn't #16096 be a much more simple solution for the problem you're trying to solve?

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:9 by Mitar, 10 years ago

It is true that origin is currently used only for debugging purposes, but it could be used for much more. For example, in django-overextends it would allow to better detect cycles between templates. If a tag could know what is the filename of the current template it would know which templates it has to find when extending templates with the same name (should it hide the current one to prevent a cycle or not - it should hide only if current template filename is the same as the filename in extends).

It is true that only origin name would be needed, not whole origin (with template content).

comment:10 by Mitar, 10 years ago

See an example implementation of a extends tag which allows arbitrary nesting of extends and prevents import cycles. But for it to work, origin information is needed.

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