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

Expose all cookie directives #82

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

actionshrimp
Copy link
Contributor

  • Expose all the HTTP cookie directives, not just expiration
  • Remove the defaults from set_cookies and use Set_cookie_hdr.make's defaults. This is the same for the expiration (both are Session), but path is no longer fixed/defaulted to /.
  • Use Set_cookie_hdr.t as the internal representation of the cookie used by the middleware as it has all the fields we want and a sexp representation.

NB: encoding of the cookie data is now done when adding to the Env rather than at the point of serialisation - this might actually make debugging a bit easier as it will be clearer that opium is encoding itself rather than something lower level? But happy to change this back if that seems sensible.

@actionshrimp
Copy link
Contributor Author

I've put the defaulting of path back to "/" to avoid breaking existing apps, as otherwise it apparently defaults to the request URI's path rather than 'nothing' (i.e. = to "/"), I misunderstood here!

@rgrinberg
Copy link
Owner

Thanks.

@rgrinberg rgrinberg merged commit dbc92e8 into rgrinberg:master Sep 26, 2018
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 4, 2019
CHANGES:

- Switch to dune (rgrinberg/opium#88, anuragoni)

- Keep the "/" cookie default and expose all cookie directives (rgrinberg/opium#82,
  @actionshrimp)

- Do not assume base 64 encoding of cookies (rgrinberg/opium#74, @malthe)

- Add caching capabilities to middleware (rgrinberg/opium#76, @mattjbray)
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.

2 participants