Closed franzliedke closed 8 years ago
Tests would be lovely but it will be hard to use the existing tests and still have them be deterministic. At best you could probably test to make sure the cookies are at least 4 years into the future or 4 years into the past.
Pseudocode for inspiration:
public function it_expires_cookies()
{
$setCookie = SetCookie::createExpired('shouldbeexpired');
$fourYearsAgo = new DateTimeImmutable('now')->dateSub(new DateInterval('P4Y');
$this->assertTrue($setCookie->getExpires() < $fourYearsAgo);
}
Otherwise I'm happy with the changes you've made. If you decide you don't want to do tests juet let me know and I'll click merge as-is (or write them myself if I have time).
Okay, I added two tests - please check them out, then it's ready for merge, I'd say. :)
Thank you so much! This is a great addition. :) I appreciate you going the extra mile on this!
You can thank me by tagging a new release. ;)
That way, I can use it for the next beta of Flarum - IIRC that's what I originally created this. ;)
@franzliedke Tagged v1.0.2. Thanks. :)
(If you get very bored you can add docs for these new methods! Totally forgot that part...)
Second shot at this after #4.
I went with
expired
instead offorgotten
, because that sounded nicer to me. I'll change it back if you disagree.I also did not add a
rememberForever
shortcut to theFigResponseCookies
facade. The one you suggested (FigResponseCookies::rememberForever('rememberme')
) did not make sense to me, as you'd still want to provide content for these cookies. Also, you mentioned earlier that you were unsure about whether that would need a method in the facade. Given that we now have->rememberForever()
onSetCookie
instances, that should do the trick. Again, I'll honor your wishes if you prefer something else.Last question: Would monsieur like some tests with that src? ;) (If so, please hint at what exactly you'd like to see tested.)