Opened 13 years ago

Closed 13 years ago

Last modified 12 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: no UI/UX: no

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 13 years ago.
15368-requestcontext.diff (1000 bytes ) - added by Chris Beaven 13 years ago.
15368_lp_tests.diff (3.1 KB ) - added by Luke Plant 13 years ago.
tests
15368_lp_solution.diff (2.2 KB ) - added by Luke Plant 13 years ago.
my solution

Download all attachments as: .zip

Change History (21)

comment:1 by Russell Keith-Magee, 13 years ago

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

by Timothée Peignier, 13 years ago

comment:2 by Timothée Peignier, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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 by anonymous, 13 years ago

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

comment:5 by Chris Beaven, 13 years ago

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.

by Chris Beaven, 13 years ago

Attachment: 15368-requestcontext.diff added

comment:6 by Chris Beaven, 13 years ago

Has patch: set
Needs tests: set

Ok, put up a patch but needs tests still.

comment:7 by Luke Plant, 13 years ago

@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.

by Luke Plant, 13 years ago

Attachment: 15368_lp_tests.diff added

tests

by Luke Plant, 13 years ago

Attachment: 15368_lp_solution.diff added

my solution

comment:8 by Chris Beaven, 13 years ago

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.

in reply to:  8 comment:9 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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 by Chris Beaven, 13 years ago

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 by Luke Plant, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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

comment:17 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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