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

Use Cookie.__init__ instead of settings keys as a dict [Fixes #254] #255

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Pirulax
Copy link

@Pirulax Pirulax commented Apr 7, 2024

Goal

Fix issue #254

New Features/Changes

The functionality should've remained the same, it just fixes the deprecation warning that was emitted.
I'm unsure how this affects the min. supported version of Sanic, perhaps that should be updated too.

Open Questions and TODOs

  • I think there might've been a bug with setting the secure key. The original code did a falsey check, but False is also a falsey value, so the code didn't set the secure key to False, but rather left it at whatever it was by default. I fixed this by doing an explicit None check. Now, the TODO here is to check if my assumption/fix is correct.
  • Brief test - I tested the changes by logging into my site, and everything works as it used to, but this time with no deprecation warnings.

@Pirulax Pirulax changed the title Fix cookie deprecation [Fixes #254] Fix CookieJar deprecation warning [Fixes #254] Apr 7, 2024
@Pirulax Pirulax changed the title Fix CookieJar deprecation warning [Fixes #254] Fix setting values on a Cookie object as a dict [Fixes #254] Apr 7, 2024
@Pirulax Pirulax changed the title Fix setting values on a Cookie object as a dict [Fixes #254] Use Cookie.__init__ instead of settings keys as a dict [Fixes #254] Apr 7, 2024
@Pirulax
Copy link
Author

Pirulax commented Apr 16, 2024

Okay, so in the meanwhile I discovered 2 bugs.

  1. On localhost cookie_domain() is '' (empty string), which originally became cookie_domain=None, but with my code it (was) cookie_domain='', which didn't really work all that well :D

  2. There's something up with max_age too, and I'm unsure if it's intentional or not: config.cookie_max_age() is 0, which, again, is a falsey value, thus it's not set. I'm unsure if it's meant to be set in this case, or is supposed to be converted to None instead.

I've now changed my code to work like it originally did, but (2) should be looked into by someone more familiar with the lib.

@Pirulax Pirulax mentioned this pull request Oct 24, 2024
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.

1 participant