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

Default cookie to / #444

Closed
wants to merge 1 commit into from
Closed

Default cookie to / #444

wants to merge 1 commit into from

Conversation

ambs
Copy link
Member

@ambs ambs commented Sep 8, 2013

Refactored PR from old @bigpresh branch.
Shouldn't hurt.

@ambs
Copy link
Member Author

ambs commented Sep 12, 2013

The alternative is this commit: 01baec5

@xsawyerx, whose do you prefer?

@xsawyerx
Copy link
Member

What does the RFC specify?

@bigpresh
Copy link
Member

http://tools.ietf.org/html/rfc6265#section-5.2.4 says:

If the attribute-value is empty or if the first character of the attribute-value is not %x2F ("/"):

Let cookie-path be the default-path.

http://tools.ietf.org/html/rfc6265#section-5.1.4 explains how the default-path should be calculated - but it will be based on the URL requested.

For e.g. if the cookie is being set as a result of a request for /foo/bar, the default-path for the cookie would be '/foo/bar- so it would *not* be sent in future requests for e.g./baror/foo/baz` - which is somewhat surprising behaviour for most users.

Defaulting to / makes cookies behave the way I think most people would expect it to behave in the absence of a specified path.

@ambs
Copy link
Member Author

ambs commented Sep 14, 2013

I would say that with my vote and @bigpresh one, we could just merge, but I'll be a good boy and wait for @xsawyerx 👍

@xsawyerx
Copy link
Member

Since this is not an urgent issue, I'd rather we not rush into it.
I generally believe going by the RFC is best. In case of surprised users, we can point to it and show we're not trying to be clever. If someone is already used to the RFC, we're screwing with them, and that won't be taken fondly.

@shumphrey
Copy link
Contributor

If I read D1 correctly, it defaults to '/' if not otherwise specified.
@xsawyerx have you had any more thoughts on RFC vs intuitiveness?

@xsawyerx
Copy link
Member

xsawyerx commented Apr 2, 2014

Will review today. @shumphrey thanks for poking!

@ambs
Copy link
Member Author

ambs commented Apr 9, 2014

@xsawyerx, one week, so, poking again :)

@veryrusty veryrusty modified the milestones: 0.13, 0.11 Apr 10, 2014
@veryrusty
Copy link
Member

Pr #121 changed the default value of Dancer2::Core::Cookie->path to be '/', which makes this Pr unnecessary. Closing :)

@veryrusty veryrusty closed this Apr 11, 2014
@veryrusty veryrusty deleted the pr/default_cookie_path branch April 11, 2014 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants