Code

Opened 2 years ago

Closed 13 months ago

Last modified 4 months ago

#17199 closed Bug (invalid)

Base Template Loader should return the origin

Reported by: Kronuz Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Kronuz, 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 Kronuz 2 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 2 years ago by Kronuz

  • Cc Kronuz added
  • Component changed from Uncategorized to Template system
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

Changed 2 years ago by Kronuz

comment:2 Changed 2 years ago by Kronuz

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 2 years ago by Kronuz (previous) (diff)

comment:3 Changed 2 years ago by aaugustin

  • Resolution set to needsinfo
  • Status changed from new to closed
  • Version set to 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 Changed 2 years ago by Kronuz

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 Changed 2 years ago by Kronuz

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

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 Changed 2 years ago by bmihelac

  • Cc bmihelac@… added

comment:7 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design decision needed

comment:8 Changed 13 months ago by aaugustin

  • Resolution set to invalid
  • Status changed from reopened to closed

#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 13 months ago by aaugustin (previous) (diff)

comment:9 Changed 4 months ago by mitar

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 Changed 4 months ago by mitar

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.