Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15721 closed Bug (fixed)

{% include %} and RequestContext fails since r15795

Reported by: mk Owned by: SmileyChris
Component: Template system Version: 1.3
Severity: Release blocker Keywords:
Cc: d1b, mvt, 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 mk 3 years ago.
15721.2.diff (4.0 KB) - added by SmileyChris 3 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by mk

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by mk

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

Changed 3 years ago by mk

comment:3 Changed 3 years ago by lukeplant

  • Keywords regression added
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned

Changed 3 years ago by SmileyChris

comment:5 Changed 3 years ago by SmileyChris

  • 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 3 years ago by mk

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 3 years ago by SmileyChris

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 3 years ago by lukeplant

@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 3 years ago by lukeplant

  • Type set to Bug

comment:10 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:11 Changed 3 years ago by d1b

  • Cc d1b added

comment:12 Changed 3 years ago by lukeplant

  • Keywords regression removed
  • Severity changed from Normal to Release blocker

comment:13 Changed 3 years ago by mvt

  • Cc mvt added

comment:14 Changed 3 years ago by julien

  • Patch needs improvement set

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

comment:15 Changed 3 years ago by SmileyChris

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 3 years ago by dfoerster

  • Cc david@… added

comment:17 Changed 3 years ago by SmileyChris

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

comment:18 Changed 3 years ago by SmileyChris

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16031]:

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

comment:19 Changed 3 years ago by ikelly

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 3 years ago by SmileyChris

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

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 3 years ago by lukeplant

I think this needs to be backported to 1.3.X

comment:22 Changed 3 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 3 years ago by SmileyChris

  • 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 3 years ago by anonymous

Has the fixed version already been released to PyPI?

comment:25 Changed 3 years ago by SmileyChris

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.

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.