Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15368 closed (fixed)

Regression in Context

Reported by: Timothée Peignier Owned by: Luke Plant
Component: Template system Version: 1.2
Severity: Keywords: blocker regression
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

There is a regression since r15601 (in the 1.2.X branch), that notably break tests :

def _new(self): 
 	return self.__class__(autoescape=self.autoescape, 
 	                                    current_app=self.current_app, 
 	                                    use_l10n=self.use_l10n)

But the use_l10nattribute doesn't exist in 1.2.

Attachments (4)

15368-regression-context.diff (879 bytes) - added by Timothée Peignier 6 years ago.
15368-requestcontext.diff (1000 bytes) - added by Chris Beaven 6 years ago.
15368_lp_tests.diff (3.1 KB) - added by Luke Plant 6 years ago.
tests
15368_lp_solution.diff (2.2 KB) - added by Luke Plant 6 years ago.
my solution

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by Russell Keith-Magee

Keywords: blocker regression added
milestone: 1.3
Triage Stage: UnreviewedReady for checkin

Changed 6 years ago by Timothée Peignier

comment:2 Changed 6 years ago by Timothée Peignier

I've added a patch, that actually only scratch the surface, I have a new error when trying to access request.user in a context_processor in my tests :

  File "django/template/context.py", line 23, in __copy__
    duplicate = self._new()
  File "django/template/context.py", line 172, in _new
    current_app=self.current_app)
  File "django/template/context.py", line 168, in __init__
    self.update(processor(request))
  [..]
    if request.user.is_authenticated():
AttributeError: 'HttpRequest' object has no attribute 'user'

I will try to get a regression test working.

comment:3 Changed 6 years ago by Russell Keith-Magee

Owner: changed from nobody to Chris Beaven
Triage Stage: Ready for checkinAccepted

Ok - moving back to accepted. Also putting it on SmileyChris' radar, since he broke it :-)

comment:4 Changed 6 years ago by anonymous

Yes, I thought about this earlier today before reading this ticket...

comment:5 Changed 6 years ago by Chris Beaven

Actually, I've fixed the use_l10n part of this in r15618.

I'll leave this ticket open to track the issue of copying the context. The issue is that we need to make a new instance of RequestContext, but don't have any reference to request at that time so I faked it with a bare HttpRequest (and the problems around this is what I was saying I thought about earlier).

I'm thinking that perhaps I could change RequestContext to allow for a request of None which simply skips the running of the context processors during instantiation.

Changed 6 years ago by Chris Beaven

Attachment: 15368-requestcontext.diff added

comment:6 Changed 6 years ago by Chris Beaven

Has patch: set
Needs tests: set

Ok, put up a patch but needs tests still.

comment:7 Changed 6 years ago by Luke Plant

@SmileyChris - why do we need to use the class constructor to copy the instance? Could we not just implement __copy__ differently on RequestContext? In fact, do we need to implement __copy__() at all? You can simplify things quite a bit by implementing a copy() method which just uses copy.copy() with a few additions, and then using that copy method rather than copy.copy() on the Context object.

I'll attached some tests for the regression involving RequestContext, and my proposed fix.

Changed 6 years ago by Luke Plant

Attachment: 15368_lp_tests.diff added

tests

Changed 6 years ago by Luke Plant

Attachment: 15368_lp_solution.diff added

my solution

comment:8 Changed 6 years ago by Chris Beaven

luke: elsewhere in core the __copy__ method had been used, so I was just following precidence. I'm also fine with an explicit .copy() or .clone() method.

comment:9 in reply to:  8 Changed 6 years ago by Luke Plant

Replying to SmileyChris:

luke: elsewhere in core the __copy__ method had been used, so I was just following precidence. I'm also fine with an explicit .copy() or .clone() method.

Sure, I think it just happens that in this case it is simpler not to implement __copy__().

However, there is a way to implement __copy__() and still avoid the need for calling constructors - like this:

class NoInitArgs(object): pass

class MyClass(object):
   def __copy__(self):
       duplicate = NoInitArgs()
       duplicate.__class__ = self.__class__
       duplicate.__dict__ = self.__dict__.copy()
       return duplicate

comment:10 Changed 6 years ago by Luke Plant

Owner: changed from Chris Beaven to Luke Plant

Taking ownership, since we didn't hear from Chris for a few days, and I'm aware of the earthquake in New Zealand (hope everything is OK with you and family and friends, Chris!).

comment:11 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: newclosed

In [15660]:

Fixed #15368 - test failures due to regression with RequestContext

Thanks to cyberdelia for the reports on this.

comment:12 Changed 6 years ago by Luke Plant

In [15661]:

[1.2.X] Fixed #15368 - test failures due to regression with RequestContext

Thanks to cyberdelia for the reports on this.

Backport of [15660] from trunk.

comment:13 Changed 6 years ago by Chris Beaven

Sorry, should have mentioned here that I had a branch ready for merging :P

https://github.com/SmileyChris/django/tree/15368-copy-requestcontext

I'll apply the 1.3 notes today some time.
Note that in my branch, I just used copy(super(...)) which works rather than requiring a placeholder object.

comment:14 Changed 6 years ago by Luke Plant

Sorry, Chris, didn't mean to be impatient, but with this blocking the RC we were supposed to release this week, and with it affecting my own projects, I went ahead and committed a fix. Feel free to improve what I did!

comment:15 Changed 6 years ago by Russell Keith-Magee

In [15876]:

Fixed #15368 -- Ensure that raw unittest TestCase instances can be invoked individually by the test runner. Thanks to SmileyChris for the report and patch.

comment:16 Changed 6 years ago by Russell Keith-Magee

Dang - that last comment should have been for #15638.

comment:17 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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