Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: set_cookie datetime #292

Merged
merged 4 commits into from Nov 18, 2016
Merged

Bugfix: set_cookie datetime #292

merged 4 commits into from Nov 18, 2016

Conversation

@bertjwregeer
Copy link
Member

@bertjwregeer bertjwregeer commented Nov 18, 2016

This fixes the bug reported by @dobesv in #254. Also documents that expires should not be a localtime, and that it should not be set in the past.

@bertjwregeer bertjwregeer added this to the 1.7.0 milestone Nov 18, 2016
@bertjwregeer bertjwregeer force-pushed the bugfix/set_cookie_datetime branch from 4fba852 to 5be7ff4 Nov 18, 2016
@mmerickel
Copy link
Member

@mmerickel mmerickel commented Nov 18, 2016

What is the issue with setting expires in the past? AFAIK this is a common technique for clearing a cookie.

@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Nov 18, 2016

@mmerickel: There is unset_cookie for that and I would rather warn because it may mean the user accidentally passed in a local time datetime object and setting the cookie is not doing what they think it is going to do.

@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Nov 18, 2016

I can change the warning message, expires is going away in the future anyway. Setting max_age to 0 is a better idea.

@mmerickel
Copy link
Member

@mmerickel mmerickel commented Nov 18, 2016

Yeah this is a little wishy washy but it sort of feels like something webob shouldn't have an opinion on.

@bertjwregeer
Copy link
Member Author

@bertjwregeer bertjwregeer commented Nov 18, 2016

If you don't think it is a good idea, I can remove that warning. I only added it late last night because of the local time vs utc time issue and maybe a warning would be nice, I have no opinion either way.

This will convert the TZ aware datetime to one that is not timezone
aware and to UTC. This can then be used in math to generate the
timedelta for max_age.

Also documents that using a local datetime object is not supported,
unfortunately there is no good way to find out whether a datetime is
local or not, so there is no way to warn the user about bad input.

Closes #254
@bertjwregeer bertjwregeer force-pushed the bugfix/set_cookie_datetime branch from 5be7ff4 to ceb4ade Nov 18, 2016
@bertjwregeer bertjwregeer merged commit ab1aa6d into master Nov 18, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
bertjwregeer added a commit that referenced this pull request Nov 18, 2016
bertjwregeer added a commit that referenced this pull request Nov 18, 2016
@bertjwregeer bertjwregeer deleted the bugfix/set_cookie_datetime branch Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants