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

Add support for custom session cookie serializers #114

Merged
merged 1 commit into from
Nov 19, 2014
Merged

Add support for custom session cookie serializers #114

merged 1 commit into from
Nov 19, 2014

Conversation

cconstantin
Copy link
Contributor

This is to ease integration with existing applications, in particular rails.

Current behaviour is maintained (serialize to ETF), but support for JSON is added.

@scrogson
Copy link
Contributor

scrogson commented Nov 3, 2014

My original PR (#72) included this feature, but it appears it was removed before merging. @josevalim do you remember why? I'm a 👍 on allowing a serializer to be configured.

@cconstantin the only thing I'm not a big fan of is the namespaces for the serializers. Since a serializer could be used for things other than cookies I think Plug.Serializer(s).JSON may be more appropriate.

@josevalim
Copy link
Member

@scrogson it was removed because it was added as part of the encryptor while it should be a responsibility of the cookie store (as in this pull request). :)

@scrogson
Copy link
Contributor

scrogson commented Nov 3, 2014

Ah! Makes sense.

@cconstantin
Copy link
Contributor Author

@josevalim anything you'd change with this PR then? I'm not sure what to expect about how/when feedback is provided. :)

@josevalim
Copy link
Member

I am on holidays so I don't have a lot of time for reviewing and merging code lately but I plan to get back to it soon-ish. :)

@cconstantin
Copy link
Contributor Author

Enjoy your time off. :)

@josevalim
Copy link
Member

Let's merge this but I would like to propose some changes.

  1. Let's only expect atoms (i.e. modules) as argument to serializer. Strings/binaries won't be supported
  2. The default serializer can be :external_term_format and we don't need to implement a module. We can just check if the value is :external_term_format and call term_to_binary / binary_to_term directly.
  3. Let's not provide a default JSON serializer. Instead, people can simply pass Poison and it will work because Poison exports decode/encode. We expect those functions to return {:ok, _}. Anything else will be ignored.

The goal is to make the code simpler and remove abstractions. We need to rebase and improve the docs on the exact API a serializer is required to implement.

@cconstantin
Copy link
Contributor Author

@josevalim updated the code. Any other suggestions? Thanks.

josevalim added a commit that referenced this pull request Nov 19, 2014
Add support for custom session cookie serializers
@josevalim josevalim merged commit 76ac267 into elixir-plug:master Nov 19, 2014
@josevalim
Copy link
Member

Beautiful ❤️ 💚 💙 💛 💜

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.

None yet

3 participants