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 HTTP::Cookie#expire #14819

Merged

Conversation

a-alhusaini
Copy link
Contributor

Reasoning

I tried to use cookies.delete("...") today and it didn't cause the cookie to expire from the browser. It sent it again on the next request.

This method sets the cookie value to an empty string and seets the expiry date to 5 minutes ago. Browsers automatically delete expired cookies.

My aim is to provide a convenience to anyone who happens to need to use HTTP::Cookie

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jul 23, 2024

Since the intent is to expire the cookie, then #expire would be a better name than #destroy.

I'd also put an expiration date far further than 5 minutes ago (clocks can get unsynced) and use a fixed date, like the UNIX timestamp (1970-01-01 00:00:00 UTC).

@a-alhusaini
Copy link
Contributor Author

@ysbaddaden Hey julian! This little function is a convenience for destroying cookies (which usually means setting their expiry to some date in the past and setting their value to blank)

I just thought others might want a convenient function to destroy cookies.

Although as you pointed out, it is superflous. I just PR'd it to promote developer happiness :3

@straight-shoota
Copy link
Member

It would be much appreciated to follow the guidelines for feature discussions and open an issue for a new proposal first, to keep the general discussion independent of a PR with a concrete implementation.
Since this is relatively trivial, I suppose we can continue the discussion here. But next time, please open an issue first please.

@straight-shoota
Copy link
Member

Actually, setting Expires to a date in the past does not fully ensure the cookie to expire. Max-Age takes precedence. If Max-Age is set, the value of Expires doesn't matter.

So I think the functionality should be implemented as self.max_age = Time::Span.zero.
We could also consider self.expires = Time::UNIX_EPOCH on top, just for good measure, but that should not really be necessary.

@a-alhusaini
Copy link
Contributor Author

Sorry about the typo. I can't run the test suite locally so I'm relying on the CI.

src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota changed the title added HTTP::Cookie#destroy. It expires cookie and sets its value to b… Add HTTP::Cookie#destroy Aug 5, 2024
@straight-shoota
Copy link
Member

IMO the name #destroy makes sense. Expiration is just the mechanism used to signal to the client to forget this cookie.

@straight-shoota straight-shoota self-assigned this Nov 12, 2024
@beta-ziliani beta-ziliani requested review from ysbaddaden and bcardiff and removed request for ysbaddaden November 13, 2024 19:24
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that #expire or #expire! would reflect better the behavior.

There was a issue in the past for rails where the behavior of #delete for cookies was discussed. An outcome of that discussion was that can be applied here is that HTTP::Cookies type is not only used for browsers. For example #destroy in the context of a MITM proxy would have a different expectation. So I think #expire or #expire! would be better and more application agnostic.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Nov 25, 2024

I initially proposed #expire so I wholeheartedly agree with @bcardiff.

@straight-shoota straight-shoota changed the title Add HTTP::Cookie#destroy Add HTTP::Cookie#expire Dec 16, 2024
@straight-shoota straight-shoota added this to the 1.15.0 milestone Dec 16, 2024
@straight-shoota straight-shoota merged commit 4f00889 into crystal-lang:master Dec 17, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants