Fixed #36410 -- Added named template partials to DTL #19643
Conversation
1fb9159 to
0c95342
Compare
There was a problem hiding this comment.
Hello @FarhanAliRaza, amazing start! 🌟
Thank you for working on this. I have added a first round of initial comments. Let me know if you have any questions.
Looking forward to seeing this progressing!
|
Thanks for the review. |
There was a problem hiding this comment.
@FarhanAliRaza Thank you for the round of fixes, I appreciate the quick turnaround!
I added some more comments, specifically around tests we should use a bit more of subTest, but this is looking great overall.
Then, there is an outstanding design question about where best to place the "template proxy". I like Carlton's suggestions and I have added a few more notes. Also I'm looking forward of @ngnpope input!
Once all the above is resolved, I will do a top-bottom final review to ensure consistency and live testing to check correctness.
|
@nessita I don't know why GitHub is not showing it on the main Conversations tab, but I replied to your #19643 (comment), and it does show in the Files view. |
|
Extra review points are fixed. Now, I'm just waiting for the code-moving bit to be finalized. |
Thank you for pointing that out, I commented in the same thread. |
There was a problem hiding this comment.
Astonishing progress, thank you @FarhanAliRaza and @carltongibson!
|
Added syntax tests. Refactored tests to match other syntax tests like |
Thank you @FarhanAliRaza for the updates, I'll be restarting work/review on this PR tomorrow morning. |
|
@ngnpope Hello! Is this PR something you wanted/could take a look? Non worries if not, but let me know when you can. |
There was a problem hiding this comment.
I've been doing a deep dive on tests, both the unit and manual tests.
I'll push some tweaks, along with some cleanup, including a squash of all commits. I'm also spent an embarrassing amount of time trying to replace the current mock-heavy test_partial_template_get_exception_info with a test that would be more realistic (more on that below). This has lead to the following potential bug discovery.
Local setup
- Basic Django project with a
testappapp andtestapp/urls.pythat include:
path("partialtest/", TemplateView.as_view(template_name="partialtest.html")),partialtest.htmllooks like this:
<h1>Title</h1>
{% partialdef testtestest %}
<p>{{ nonexistent|default:"alsonotthere" }}</p>
{% endpartialdef %}
<h2>Sub Title</h2>
{% partial testtestest %}Bug?
- Should
path("partialtestpartial/", TemplateView.as_view(template_name="partialtest.html#testestest"))work? Currently is crashes with traceback:
Internal Server Error: /testapp/partialtestpartial/
Traceback (most recent call last):
File "/home/nessita/fellowship/django/django/core/handlers/exception.py", line 55, in inner
response = get_response(request)
File "/home/nessita/fellowship/django/django/core/handlers/base.py", line 221, in _get_response
response = response.render()
File "/home/nessita/fellowship/django/django/template/response.py", line 114, in render
self.content = self.rendered_content
^^^^^^^^^^^^^^^^^^^^^
File "/home/nessita/fellowship/django/django/template/response.py", line 90, in rendered_content
template = self.resolve_template(self.template_name)
File "/home/nessita/fellowship/django/django/template/response.py", line 72, in resolve_template
return select_template(template, using=self.using)
File "/home/nessita/fellowship/django/django/template/loader.py", line 47, in select_template
raise TemplateDoesNotExist(", ".join(template_name_list), chain=chain)
django.template.exceptions.TemplateDoesNotExist: partialtest.html#testestest
[30/Jul/2025 19:24:50] "GET /testapp/partialtestpartial/ HTTP/1.1" 500 87799
- If we change
"alsonotthere"withalsonotthere(i.e. make the literal string a missing var in the context), IRL we get the expected exceptionVariableDoesNotExistand I seePartialTemplate.get_exception_infobeing called. But I tried to model a test after this and is close but it needs tweaks to fully mimic a real scenario, could you take a look and see what's missing?
from django.template import Context, Engine, Template, VariableDoesNotExist
from django.template.base import Token, TokenType
from django.test import SimpleTestCase, override_settings
class TemplateExceptionInfoTests(SimpleTestCase):
@override_settings(DEBUG=True)
def test_get_exception_info_reports_correct_location_locmem(self):
buggy_template = """<h1>Title</h1>
{% partialdef testtestest %}
<p>{{ nonexistent|default:alsonotthere }}</p>
{% endpartialdef %}
<h2>Sub Title</h2>
{% partial testtestest %}
"""
engine = Engine(
loaders=[
("django.template.loaders.locmem.Loader", {"template": buggy_template}),
]
)
# This does NOT trigger PartialTemplate.get_exception_info AND has an origin.
template = engine.get_template("template")
# This DOES trigger PartialTemplate.get_exception_info, but it has NO origin.
template = Template(buggy_template, origin=template.origin)
try:
template.render(Context({}))
except VariableDoesNotExist as e:
# Simulate token from the failed parse.
fake_token = Token(
token_type=TokenType.VAR,
contents="nonexistent|default:alsonotthere",
position=(10, len(buggy_template)),
)
exc_info = template.get_exception_info(e, token=fake_token)
# exc_info has the same content whether `PartialTemplate.get_exception_info`
# returns something or {}!
self.assertEqual(exc_info.get("message"), "Failed lookup for key [%s] in %r")I need to stop for now but as said I will some changes so please pull them before making any further changes. Thanks! 🌟
a7a6263 to
16804e4
Compare
|
@nessita I don't know if this is the issue but, the partial name in the url is
But the partial in the template is
There's an extra |
Thank you for looking so quickly, that was a typo (caused by one of the manual testing stages to test an undefined partial name). I've swicthed to Template: <h1>Title</h1>
{% partialdef partialtest %}
<p>{{ nonexistent|default:alsonotthere }}</p>
{% endpartialdef %}
<h2>Sub Title</h2>
{% partial partialtest %} |
16804e4 to
0024ea2
Compare
|
@FarhanAliRaza See if you can reproduce. Against template-partials (at b6adac3) I get:
I didn't test this branch, but if that's not what we're seeing a difference snuck in, that we'll need to pin down. |
OK, I tested. I can't reproduce this @nessita. With the 
|
Thank you! I'll debug more tomorrow, will start from scratch with your project. |
I can confirm this works, as Carlton pointed out. I have just now tested with a bunch of prints removed, I wonder if that was causing the issues I saw before 🤔 As a side note, and not related to this branch nor to the issue above, a print of @FarhanAliRaza I guess that one thing that would be useful for me is if you could take a look at the test trying to test more realistically |
@carltongibson I can not reproduce it. Working as expected.
The same applies to me for this current branch. @nessita Working on the test case. |
|
@nessita I have added tests for with self.assertRaises(VariableDoesNotExist) as cm:
template.render(context)
exc_info = cm.exception.template_debug |
Thank you! I can't look right away because I'm deep in other review, but could you please double check that if the implementation for |
|
Yes, I made sure it gave KeyError |
There was a problem hiding this comment.
Hey @carltongibson and @nessita. I managed to do a review this evening - hope it helps. This is shaping up to be very nice and feels like it'll provide a lot of power with very little code to implement it - thanks for your efforts @FarhanAliRaza! 🥇
|
Implemented the suggestion from @ngnpope . |
|
@FarhanAliRaza @nessita on the new-features repo there's a suggestion to move |
There was a problem hiding this comment.
Thank you @FarhanAliRaza! This is shaping nicely and with a delight precision. 🌟
Today I focused mostly on the new syntax tests. Overall I would like to see more meaningful names used for the tests and the templates, and also more of subTest when semantically adequate. I added some suggestions for both of these points.
Then, a minor nitpick: I think that the partial name used for testing could be partial-name or testing-name to signal clearly to readers that this is a name identifier.
Thinking about performance, we may need one or two tests with big chunky templates (using html test files) making heavy use of the new tags to see if the regexes show any obvious catastrophic side-effect.
| def _render(self, context): | ||
| return self.nodelist.render(context) |
There was a problem hiding this comment.
I think we are missing some tests or something else is going on. If I change this code to be this instead:
| def _render(self, context): | |
| return self.nodelist.render(context) | |
| def _render(self, context): | |
| return "" |
No test fails. I'm sure there is a reasonable explanation but I can't find it :-).
|
@nessita Now tests will fail if we change |
django/test/utils.py
Outdated
| if isinstance(self, PartialTemplate): | ||
| return _TestState.saved_data.partial_template_render(self, context) | ||
|
|
There was a problem hiding this comment.
Hang on, this can't be right. Let's slow down.
We don't do this for Template._render() — which is replaced in exactly the same way — so why are we doing it here?
If you make the same change (return "") to Template._render() the only failures are in admin_scripts, which are using call_command so in a subprocess likely.
So I don't think there's actual coverage here, even on Template._render()
What needs to happen here — if we need tests — is a test_private_render_is_actually_called() (or similar) that restores the _render() method, and then checks the output.
There was a problem hiding this comment.
So add a test test_private_render_is_actually_called.
or
We do the same solution with Template class.
return _TestState.saved_data.template_render(self, context)
As this makes more sense because this way always the original function is being called, we are getting the signal that we want from instrumenting. It will make it easier to test any future changes if a contributor makes any changes, and they will see the changes during testing.
If this causes no other unexpected problems ...
There was a problem hiding this comment.
Option 3 is to do nothing, but a regression test is likely worthwhile.
I don't like the suggested change here. If we want to go this kind of route then we want to adjust instrumented_test_render to decorate the original method, and apply that, rather than using a pair of isinstance checks.
Given how long instrumented_test_render has worked how it has, I'd suggest that's a separate cleanup not part of this PR. (Would it not need a ticket?)
There was a problem hiding this comment.
If we decide to go to the Renderer Mixin route to refactor _render and render method to it. We will not even need that isinstance check (I think).
|
Thanks for the updates @FarhanAliRaza. I have some additional test cases I knocked up the other day after I reviewed that could be added if you haven't already thought of them - I didn't have time to check thoroughly if you have covered these, but wanted to give you what I had. Here are some for def test_nested_partials_with_duplicate_name(self):
template_source = """
{% partialdef duplicate %}{% partialdef duplicate %}
CONTENT
{% endpartialdef %}{% endpartialdef %}
"""
with self.assertRaisesMessage(
TemplateSyntaxError,
"Partial 'duplicate' is already defined in the 'template.html' template.",
):
Template(template_source, origin=Origin(name="template.html"))
def test_conditional_partials_with_duplicate_names(self):
# The correct way to resolve this is to push the conditional inside the partial.
template_source = """
{% if ... %}
{% partialdef duplicate %}
CONTENT
{% endpartialdef %}
{% else %}
{% partialdef duplicate %}
OTHER-CONTENT
{% endpartialdef %}
{% endif %}
"""
with self.assertRaisesMessage(
TemplateSyntaxError,
"Partial 'duplicate' is already defined in the 'template.html' template.",
):
Template(template_source, origin=Origin(name="template.html"))Here is one for def test_find_partial_source_supports_named_end_tag(self):
template_source = "{% partialdef thing %}CONTENT{% endpartialdef thing %}"
template = Template(template_source)
partial_proxy = template.extra_data["template-partials"]["thing"]
result = partial_proxy.find_partial_source(template_source, "thing")
self.assertEqual(
result, "{% partialdef thing %}CONTENT{% endpartialdef thing %}"
)(This is already implicitly tested in Here are some for @setup(
{
"partial-broken-nesting": (
"<div>Before partial</div>\n"
"{% partialdef outer %}\n"
"{% partialdef inner %}...{% endpartialdef outer %}\n"
"{% endpartialdef inner %}\n"
"<div>After partial content</div>"
)
}
)
def test_broken_partial_nesting(self):
with self.assertRaises(TemplateSyntaxError) as cm:
self.engine.get_template("partial-broken-nesting")
self.assertIn("endpartialdef", str(cm.exception))
self.assertIn("Invalid block tag", str(cm.exception))
self.assertIn("'endpartialdef inner'", str(cm.exception))
reporter = ExceptionReporter(None, cm.exception.__class__, cm.exception, None)
traceback_data = reporter.get_traceback_data()
exception_value = str(traceback_data.get("exception_value", ""))
self.assertIn("Invalid block tag", exception_value)
self.assertIn("'endpartialdef inner'", str(cm.exception))
@setup(
{
"partial-broken-nesting-mixed": (
"<div>Before partial</div>\n"
"{% partialdef outer %}\n"
"{% partialdef inner %}...{% endpartialdef %}\n"
"{% endpartialdef inner %}\n"
"<div>After partial content</div>"
)
}
)
def test_broken_partial_nesting_mixed(self):
with self.assertRaises(TemplateSyntaxError) as cm:
self.engine.get_template("partial-broken-nesting-mixed")
self.assertIn("endpartialdef", str(cm.exception))
self.assertIn("Invalid block tag", str(cm.exception))
self.assertIn("'endpartialdef outer'", str(cm.exception))
reporter = ExceptionReporter(None, cm.exception.__class__, cm.exception, None)
traceback_data = reporter.get_traceback_data()
exception_value = str(traceback_data.get("exception_value", ""))
self.assertIn("Invalid block tag", exception_value)
self.assertIn("'endpartialdef outer'", str(cm.exception))And finally I had some more for testing def test_find_partial_source_supports_nested_partials(self):
template_source = (
"{% partialdef outer %}"
"{% partialdef inner %}...{% endpartialdef %}"
"{% endpartialdef %}"
)
template = Template(template_source)
empty_proxy = template.extra_data["template-partials"]["outer"]
other_proxy = template.extra_data["template-partials"]["inner"]
outer_result = empty_proxy.find_partial_source(template_source, "outer")
self.assertEqual(
outer_result,
(
"{% partialdef outer %}{% partialdef inner %}"
"...{% endpartialdef %}{% endpartialdef %}"
),
)
inner_result = other_proxy.find_partial_source(template_source, "inner")
self.assertEqual(inner_result, "{% partialdef inner %}...{% endpartialdef %}")
def test_find_partial_source_supports_nested_partials_and_named_end_tags(self):
template_source = (
"{% partialdef outer %}"
"{% partialdef inner %}...{% endpartialdef inner %}"
"{% endpartialdef outer %}"
)
template = Template(template_source)
empty_proxy = template.extra_data["template-partials"]["outer"]
other_proxy = template.extra_data["template-partials"]["inner"]
outer_result = empty_proxy.find_partial_source(template_source, "outer")
self.assertEqual(
outer_result,
(
"{% partialdef outer %}{% partialdef inner %}"
"...{% endpartialdef inner %}{% endpartialdef outer %}"
),
)
inner_result = other_proxy.find_partial_source(template_source, "inner")
self.assertEqual(
inner_result, "{% partialdef inner %}...{% endpartialdef inner %}"
)
def test_find_partial_source_supports_nested_partials_and_mixed_end_tags_1(self):
template_source = (
"{% partialdef outer %}"
"{% partialdef inner %}...{% endpartialdef %}"
"{% endpartialdef outer %}"
)
template = Template(template_source)
empty_proxy = template.extra_data["template-partials"]["outer"]
other_proxy = template.extra_data["template-partials"]["inner"]
outer_result = empty_proxy.find_partial_source(template_source, "outer")
self.assertEqual(
outer_result,
(
"{% partialdef outer %}{% partialdef inner %}"
"...{% endpartialdef %}{% endpartialdef outer %}"
),
)
inner_result = other_proxy.find_partial_source(template_source, "inner")
self.assertEqual(inner_result, "{% partialdef inner %}...{% endpartialdef %}")
def test_find_partial_source_supports_nested_partials_and_mixed_end_tags_2(self):
template_source = (
"{% partialdef outer %}"
"{% partialdef inner %}...{% endpartialdef inner %}"
"{% endpartialdef %}"
)
template = Template(template_source)
empty_proxy = template.extra_data["template-partials"]["outer"]
other_proxy = template.extra_data["template-partials"]["inner"]
outer_result = empty_proxy.find_partial_source(template_source, "outer")
self.assertEqual(
outer_result,
(
"{% partialdef outer %}{% partialdef inner %}"
"...{% endpartialdef inner %}{% endpartialdef %}"
),
)
inner_result = other_proxy.find_partial_source(template_source, "inner")
self.assertEqual(
inner_result, "{% partialdef inner %}...{% endpartialdef inner %}"
) |
|
Thank you, @ngnpope and @nessita . I added the tests you suggested and made other refactors, including changes to test and template names. @carltongibson I removed that change of instrumented method. |
4ad61a0 to
264fc67
Compare
I'll try to think of a concrete example and I'll share here.
Good point, while going deep in the review vertical I lost track of this detail. Can you help me understand which flow exactly triggers this code path? |
264fc67 to
44c2bbd
Compare
…plate Language.
Introduced `{% partialdef %}` and `{% partial %}` template tags to
define and render reusable named fragments within a template file.
Partials can also be accessed using the `template_name#partial_name`
syntax via `get_template()`, `render()`, `{% include %}`, and other
template-loading tools.
Adjusted `get_template()` behavior to support partial resolution, with
appropriate error handling for invalid names and edge cases. Introduced
`PartialTemplate` to encapsulate partial rendering behavior.
Includes tests and internal refactors to support partial context
binding, exception reporting, and tag validation.
Co-authored-by: Carlton Gibson <carlton@noumenal.es>
Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
Co-authored-by: Nick Pope <nick@nickpope.me.uk>
44c2bbd to
eb8c218
Compare
There was a problem hiding this comment.
@FarhanAliRaza Thank you for the steady and great work on this PR! 🌟
I went thru "tests vertical" today. I've pushed some cleanups, including two tests that are now commented out because they are failing. The tests are the ones below and I think using the {% include 'partial_included.html#included-partial' %} should work, right?
@setup(
{
"partial_as_include_in_other_template": (
"MAIN TEMPLATE START\n"
"{% include 'partial_included.html#included-partial' %}\n"
"MAIN TEMPLATE END"
)
},
partial_templates,
)
def _test_partial_as_include_in_template(self):
# XXX: Needs fixing?
output = self.engine.render_to_string("partial_as_include_in_other_template")
expected = (
"MAIN TEMPLATE START\n"
"THIS IS CONTENT FROM THE INCLUDED PARTIAL\n"
"MAIN TEMPLATE END"
)
self.assertEqual(output, expected)
@setup(
{
"nested_simple": (
"{% extends 'base.html' %}"
"{% block content %}"
"This is my main page."
"{% partialdef outer inline %}"
" It hosts a couple of partials."
" {% partialdef inner inline %}"
" And an inner one."
" {% endpartialdef inner %}"
"{% endpartialdef outer %}"
"{% endblock content %}"
),
"use_outer": "{% include 'nested_simple#outer' %}",
"use_inner": "{% include 'nested_simple#inner' %}",
}
)
def _test_nested_partials(self):
# XXX: Needs fixing?
with self.subTest(template_name="use_outer"):
output = self.engine.render_to_string("use_outer")
self.assertEqual(output, "It hosts a couple of partials. And an inner one.")
with self.subTest(template_name="use_inner"):
output = self.engine.render_to_string("use_inner")
self.assertEqual(output, "And an inner one.")@carltongibson do you think I'm missing something? I double checked and I don't have local changes that could be messing things up. I have errors such as:
django.template.exceptions.TemplateDoesNotExist: partial_included.html#included-partial
I think when the Some options that I can think of to fix this are to change
Change the loading logic in the |
|
@FarhanAliRaza The examples from those tests work standardly. I use that pattern a lot. Wondering if it's a test artifact? 🤔 The base |
|
@carltongibson
The first time that happened to me, too. Can you check if that specific test is running because it has _ in front of it? So it did not run for me for first try. |
Thanks. Got it. |
|
@FarhanAliRaza Yep, so you're bang on. It's that the We setup the loaders on
I don't think that's right: Partials are being added to the Django Backend, not the backend agnostic template functionality per se.
No, include is fine. It's the test that's at fault here. I think we just need to not use the Update:
I guess we could create a mini Engine subclass just with the necessary change for the Personally I'd rewrite the test but 🤷 |
|
I tried without @setup decorator also . Because in |
|
@FarhanAliRaza Can you push your rewritten test? This pattern... ... has been working since the package was created. We've had tests on that since ≈ day one no? Suddenly there's a failure: it's unlikely to now need a change to include. (It's a test artefact is first suspect.) |
Co-authored-by: Carlton Gibson <carlton@noumenal.es>
|
I only tested the usage of partial inside the included HTML file. Not tested partial loading like this only tested this about |
|
test.zip |
|
This solution works if we change the loading in the Another method is refactoring |
|
OK, we've just inserted the partial lookup in the wrong place. Confusing names strike again. @FarhanAliRaza If you restore django/template/backends/django.py and implement the logic there in django/template/engine.py we'll be in the right place, I think. (When migrating the partial lookup logic from the loader in template-partials, we've moved it up two levels to the backend, rather than just one to engine. Pushing it down again will give us what was intended.) |
Since I started reviewing, I did not see a test for this specific case. I think we may have missed it since the start (at least in this PR, unsure about template-partials).
A little bit more of info around this: I added the tests because the include wasn't working for me in a real (test) project. Amazing that you both could debugged it so quickly! |
get_templatein Django template backend to load a partial templateTrac ticket number
ticket-36410
Branch description
Provide a concise overview of the issue or rationale behind the proposed changes.
Checklist
mainbranch.