Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15368 closed (fixed)

Regression in Context

Reported by: cyberdelia Owned by: lukeplant
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 cyberdelia 3 years ago.
15368-requestcontext.diff (1000 bytes) - added by SmileyChris 3 years ago.
15368_lp_tests.diff (3.1 KB) - added by lukeplant 3 years ago.
tests
15368_lp_solution.diff (2.2 KB) - added by lukeplant 3 years ago.
my solution

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by russellm

  • Keywords blocker regression added
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

Changed 3 years ago by cyberdelia

comment:2 Changed 3 years ago by cyberdelia

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

  • Owner changed from nobody to SmileyChris
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:4 Changed 3 years ago by anonymous

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

comment:5 Changed 3 years ago by SmileyChris

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

comment:6 Changed 3 years ago by SmileyChris

  • Has patch set
  • Needs tests set

Ok, put up a patch but needs tests still.

comment:7 Changed 3 years ago by lukeplant

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

tests

Changed 3 years ago by lukeplant

my solution

comment:8 follow-up: Changed 3 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 3 years ago by lukeplant

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

  • Owner changed from SmileyChris to lukeplant

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

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

In [15660]:

Fixed #15368 - test failures due to regression with RequestContext

Thanks to cyberdelia for the reports on this.

comment:12 Changed 3 years ago by lukeplant

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

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

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

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

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

comment:17 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.