Opened 16 years ago

Last modified 10 years ago

#8122 new Cleanup/optimization

Better way of testing for cookies

Reported by: Joost Cassee Owned by: nobody
Component: contrib.sessions Version: dev
Severity: Normal Keywords:
Cc: joost@…, Domen Kožar Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

While trying to get the cookie test in the login form working again by default (see ticket #8061) I think I found a way to get rid of the set_test_cookie() / test_cookie_worked() dance.

The attached patch makes the session middleware set an empty cookie if the session does not yet have any variables set. This makes it possible to check whether the browser accepts cookies without poluting the session store. You can check for cookies using accepts_cookies() method. The old methods are marked deprecated but still work.

Attachments (3)

8122-r8161.diff (4.1 KB ) - added by Joost Cassee 16 years ago.
8122-r8277.diff (7.8 KB ) - added by Joost Cassee 16 years ago.
8122-rmaster.diff (7.9 KB ) - added by Ed Henderson 10 years ago.
Updated the diff to the latest master

Download all attachments as: .zip

Change History (13)

by Joost Cassee, 16 years ago

Attachment: 8122-r8161.diff added

comment:1 by Joost Cassee, 16 years ago

Has patch: set
Triage Stage: UnreviewedDesign decision needed

The new patch 8122-r8277.diff renames the session.accepts_cookies() method to the session.cookie_received property to match the semantics of the check.

The patch also includes tests to show the procedure works. I'm setting triage to 'design decision needed' because I feel the patch is in good shape to be judged by the Django devs. If accepted an update to the session docs needs to be included.

by Joost Cassee, 16 years ago

Attachment: 8122-r8277.diff added

comment:2 by Joost Cassee, 16 years ago

After applying patch 8122-r8277.diff, add an empty __init__.py file in the sessions_regress directory. I couldn't get svn diff to include it.

comment:3 by Chris Beaven, 16 years ago

Why the change in behaviour to set an initial session cookie for an unmodified session (if it wasn't found)?

comment:4 by Joost Cassee, 16 years ago

The idea is that every session has a session cookie so that you avoid the need to call set_test_cookie() (which sets the cookie by added to the session). The session cookie is empty if the session is not yet modified to avoid a call to the session backend.

comment:5 by Chris Beaven, 16 years ago

Ah, I understand now - I was just a bit worried that it would be picking up that the key wasn't None and doing something with it. After a closer look at the session innards I see this isn't the case.

This ticket looks good - why don't you raise it on the django-dev group?

Of minor importance - the Django test client isn't really necessary here is it? (it's a lot slower than the testcase one) and wouldn't it be better to use the proper testcase assertions rather than just assert (but I'm not sure if it really matters)?

comment:6 by Joost Cassee, 16 years ago

I agree that the difference between the session key being None and being "" is a subtle one, yet it is a useful distinction here. I wrote tests precisely to check whether I did the tests right!

The tests were written before I noticed how other middleware was tested. I agree that changing it to just calling the middleware directly would be nice.

I will post a message about this ticket on django-dev, as you suggested. Thanks for looking at it!

comment:7 by Domen Kožar, 14 years ago

Cc: Domen Kožar added

comment:8 by Luke Plant, 14 years ago

Severity: Normal
Type: Cleanup/optimization

comment:9 by Alex Gaynor, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

comment:10 by Tim Graham, 10 years ago

Patch needs improvement: set

Patch no longer applies cleanly.

by Ed Henderson, 10 years ago

Attachment: 8122-rmaster.diff added

Updated the diff to the latest master

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