dflydev / dflydev-fig-cookies

Cookies for PSR-7 HTTP Message Interface.
MIT License
224 stars 29 forks source link

Add shortcut methods for long-living and expiring cookies #4

Closed franzliedke closed 8 years ago

franzliedke commented 9 years ago

With this, we can do stuff like the following:

// Keep a user preference for a very long time (5 years)
SetCookie::forever('colorpref', 'green');

// Expire the named cookie
SetCookie::forget('rememberme');
simensen commented 9 years ago

I like the direction here! The +/- 5 years feels a bit odd but I get it. I'm not sure what the standard way is to set a cookie to stick around "forever" or expire as far as expiration time goes.

I think I'd like to see these methods mirrored on the FigResponseCookies facade as well. Or at the very least, the ::forget method. I'm not sold on ::forever on the facade yet.

Thanks!

franzliedke commented 9 years ago

What about forever as a factory method on the Cookie class and forget on the FigResponseCookies facade?

simensen commented 9 years ago

@franzliedke I apologize for the delay on this one.

I'm rewriting my response because I changed my mind again while writing it the first time. :) I think I like the idea of having these be mutator methods rather than named constructors:

$setCookie = SetCookie::create('colorpref', 'green')->rememberForever();
$setCookie = SetCookie::create('rememberme')->forget();

This will allow us to remember and forget existing instances of SetCookie.

I can't think of many cases where someone would want to bump an existing cookie up to "remember forever" but I can think of a lot of cases where someone might want to just forget a cookie. So I think that we should have a forget method in FigResponseCookies like you suggested.

What do you think?

franzliedke commented 9 years ago

No worries.

I'm basically fine with these suggestion, except for the fact that SetCookie->create->forget reads kind of akward.

simensen commented 9 years ago

Yes that is unfortunate. :-/ not sure I can think of a better way around that for now.

franzliedke commented 9 years ago

Well, what about my original proposal (at least for that method)?

simensen commented 9 years ago

What would you think of another named constructor:

SetCookie::createForgottenCookie('rememberme');

That way you could:

$setCookie = $setCookies->get('rememberme')->forget();

// internally calls static::create($name)->forget()
$setCookie = SetCookie::createForgottenCookie('rememberme');

$response = FigResponseCookies::forget('rememberme');

I think this covers all of our use cases? The only question is whether you like createForgottenCookie as a name. Alternatives could be, createExpiredCookie or something along those lines, but then I think we should change the verb everywhere ( $setCookie->expire(), FigResponseCookies::expire(), etc ).

franzliedke commented 9 years ago

It's just so verbose... :-/

simensen commented 9 years ago

createForgotten is the shortest I can think of. I like the convenience of creating them from scratch but I also like being able to call forget on an existing instance.

simensen commented 9 years ago

I'll be flying for a day now. Will not have a lot of ability to check things but if you send a PR I can try to merge it on the go. :)

franzliedke commented 9 years ago

What I don't like about createForgotten is that it is so confusing: after all, I want to destroy (or forget, for that matter) the cookie.

Then again, the "create" makes it clear that I get a result that needs to be added to the response to have any effect...

franzliedke commented 8 years ago

Hey @simensen, let's get this merged.

You know my objections, but you're the maintainer. What do you want me to implement? :)

simensen commented 8 years ago

@franzliedke Let's do:

$setCookie = $setCookies->get('rememberme')->forget();
$setCookie = SetCookie::createForgotten('rememberme');
$response = FigResponseCookies::forget('rememberme');

$setCookie = $setCookies->get('rememberme')->rememberForever();
$setCookie = SetCookie::createRememberedForever('rememberme');
$response = FigResponseCookies::rememberForever('rememberme');

I'd love to use something shorter than "rememberForever" but I'd very much like this action to be a verb. All cookies "remember" so I think I'd want to avoid something like ->remember so unless we can come up with a single word verb that means "remember something for a very long time" I think we're stuck with rememberForever.

I appreciate your willingness to work with me on this. I know it is more verbose than you would like. I apologize for the delay in getting back to you. :)

franzliedke commented 8 years ago

Okay, PR is incoming... just one more thought on naming:

Since createForgotten is so awkward, how about createExpired?

franzliedke commented 8 years ago

Superseded by PR #10.