Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15721 closed Bug (fixed)

{% include %} and RequestContext fails since r15795

Reported by: Matthias Kestenholz Owned by: Chris Beaven
Component: Template system Version: 1.3
Severity: Release blocker Keywords:
Cc: d1b, Michael van Tellingen, david@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Problem 1 is that RequestContext? does not accept the autoescape parameter.

Problem 2 is that self.class might be a RequestContext? instance, not only a context instance -- meaning that either RequestContext? would have to override new() and include the request, or (as is in the patch) the Context class should be hardcoded, because the "only" handling would be circumvented when a new RequestContext? is created and all context processors are run, again.

This patch fixes the problem for me:

diff --git a/django/template/context.py b/django/template/context.py
index bcfaa3b..5749e0c 100644
--- a/django/template/context.py
+++ b/django/template/context.py
@@ -104,7 +104,7 @@ class Context(BaseContext):
         Returns a new Context with the same 'autoescape' value etc, but with
         only the values given in 'values' stored.
         """
-        return self.__class__(dict_=values, autoescape=self.autoescape,
+        return Context(dict_=values, autoescape=self.autoescape,
                               current_app=self.current_app, use_l10n=self.use_l10n)
 
 class RenderContext(BaseContext):
@@ -167,8 +167,8 @@ class RequestContext(Context):
     Additional processors can be specified as a list of callables
     using the "processors" keyword argument.
     """
-    def __init__(self, request, dict=None, processors=None, current_app=None, use_l10n=None):
-        Context.__init__(self, dict, current_app=current_app, use_l10n=use_l10n)
+    def __init__(self, request, dict_=None, processors=None, autoescape=True, current_app=None, use_l10n=None):
+        Context.__init__(self, dict_, autoescape=autoescape, current_app=current_app, use_l10n=use_l10n)
         if processors is None:
             processors = ()
         else:

Attachments (2)

15721.diff (1.7 KB) - added by Matthias Kestenholz 6 years ago.
15721.2.diff (4.0 KB) - added by Chris Beaven 6 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by Matthias Kestenholz

This problem exists since #15572 was fixed in [15795].

comment:2 Changed 6 years ago by Matthias Kestenholz

Ok, patch with a really, really simple test attached.

Changed 6 years ago by Matthias Kestenholz

Attachment: 15721.diff added

comment:3 Changed 6 years ago by Luke Plant

Keywords: regression added
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

AFAICS, the new method should hard-code Context rather than self.class. That means that RequestContext.init does not need to be modified.

The test needs to go somewhere more obvious - most likely a new test method in Templates class in regressiontests/templates/tests.py

Thanks!

comment:4 Changed 6 years ago by Chris Beaven

Owner: changed from nobody to Chris Beaven
Status: newassigned

Changed 6 years ago by Chris Beaven

Attachment: 15721.2.diff added

comment:5 Changed 6 years ago by Chris Beaven

Patch needs improvement: unset

I picked a different approach to fix the bug which keeps the original context type still (even though it's most likely not necessary like Luke pointed out).
Luke, can you take a once-over of the code and tests to see if that's an ok way to do it?

Also picked up on another very minor bug (.new() could potentially mess up the context since it wasn't using values or {})

comment:6 Changed 6 years ago by Matthias Kestenholz

Nice. I'm not sure whether we should keep RequestContext around inside {% include only %} -- the resulting context does not fulfill any of the promises of a RequestContext.

Maybe it's useful for custom Context subclasses though. I'm not sure whether they are officially supported?

comment:7 Changed 6 years ago by Chris Beaven

Yeah, I had the same thoughts: wasn't sure about RequestContexts importance but it seemed to make the most sense to keep the context around, in case of custom subclasses.

comment:8 Changed 6 years ago by Luke Plant

@SmileyChris:

Looks good to me, though I reckon a bit of the duplication from the test_template method could be removed - perhaps a top level function something like this (untested):

    def template_loader_from_dict(template_dict):
        """Returns a custom template loader that loads templates from the supplied dictionary"""
        def test_template_loader(template_name, template_dirs=None):
            try:
                return (template_dict[template_name][0] , "test:%s" % template_name)
            except KeyError:
                raise template.TemplateDoesNotExist(template_name)
        return test_template_loader

comment:9 Changed 6 years ago by Luke Plant

Type: Bug

comment:10 Changed 6 years ago by Luke Plant

Severity: Normal

comment:11 Changed 6 years ago by d1b

Cc: d1b added

comment:12 Changed 6 years ago by Luke Plant

Keywords: regression removed
Severity: NormalRelease blocker

comment:13 Changed 6 years ago by Michael van Tellingen

Cc: Michael van Tellingen added

comment:14 Changed 6 years ago by Julien Phalip

Patch needs improvement: set

Patch can do with a little improvement as per Luke's comment above ;)

comment:15 Changed 6 years ago by Chris Beaven

Just for an update, I've been working on separating out that logic in a separate ticket. I'll ensure the addition of the test_template_loader util is a separate commit so it can be backported along with this one.

comment:16 Changed 6 years ago by dfoerster

Cc: david@… added

comment:17 Changed 6 years ago by Chris Beaven

See #15814 for the separation of the util. A review of that would be appreciated.

comment:18 Changed 6 years ago by Chris Beaven

Resolution: fixed
Status: assignedclosed

In [16031]:

Fixes #15721 -- Make {% include %} and RequestContext work together again.

comment:19 Changed 6 years ago by Ian Kelly

Resolution: fixed
Status: closedreopened

Reopening because I'm seeing a whole bunch of errors in the tests from this revision on. A couple of examples:

======================================================================
ERROR: test_current_site_in_context_after_login (django.contrib.auth.tests.views.LoginTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python25\Lib\site-packages\django.trunk\django\contrib\auth\tests\views.py", line 203, in test_current_site_in_context_after_login
    self.assertEqual(response.context['site'], site)
  File "C:\Python25\Lib\site-packages\django.trunk\django\template\context.py", line 55, in __getitem__
    raise KeyError(key)
KeyError: 'site'

======================================================================
ERROR: test_action_column_class (regressiontests.admin_views.tests.AdminActionsTest)
Tests that the checkbox column class is present in the response
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python25\Lib\site-packages\django.trunk\tests\regressiontests\admin_views\tests.py", line 1985, in test_action_column_class
    self.assertNotEqual(response.context["action_form"], None)
  File "C:\Python25\Lib\site-packages\django.trunk\django\test\utils.py", line 42, in __getitem__
    raise KeyError(key)
KeyError: 'action_form'

comment:20 Changed 6 years ago by Chris Beaven

Resolution: fixed
Status: reopenedclosed

In [16044]:

Fixes #15721 (again) via a minor tweak to avoid unexpected behaviour of copy() which caused a range of test failures

comment:21 Changed 6 years ago by Luke Plant

I think this needs to be backported to 1.3.X

comment:22 Changed 5 years ago by anonymous

Easy pickings: unset
UI/UX: unset

Agreed, this is kind of a huge deal. The "only" parameter is kind of useless without this.

comment:23 Changed 5 years ago by Chris Beaven

Patch needs improvement: unset

After some head-scratching, I realised it was backported already in r16089 -- not sure why it wasn't linked here.

That revision also includes the change in r16044, so 1.3.X is fully up to date.

comment:24 Changed 5 years ago by anonymous

Has the fixed version already been released to PyPI?

comment:25 Changed 5 years ago by Chris Beaven

No, it hasn't been. You'll either use the 1.3.X branch of the respository, or wait for a 1.3.1 release.

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