Silverstripe-framework: Cookies cannot expire in less than 1 day.

[patricknelson]

Note: I initially send an email to the silverstripe-dev group here but I feel it may make more sense to go ahead and submit this as an issue since, to me, this is not correct functionality (but now depended-on).

There should be a finer degree of resolution for setting a cookie expiration in SilverStripe. Since the Cookie::set() method already has parameters that exactly match that of PHP's native setcookie() function (link), I think it should also accept the same type of input (i.e. enhance this to accept unix timestamp instead). I could not find any immediate explanation at all in the documentation as to why this diverged from the unix timestamp and instead chose days. As such, I find this extremely confusing.

EDIT: Corrected to indicate this should be a unix timestamp, not just "number of days from now". Another part of why this is a little confusing.

[patricknelson]

Could we possibly create a new method or add a new parameter to toggle use of unix timestamp instead of "number of days from now"? That way we don't break compatibility. Obviously there's no good fix for this since this is likely very heavily relied upon. I'm pretty surprised this issue hasn't already been raised, actually (maybe I'm missing something).

For example:

// Native approach.
setcookie('nocache', '1', strtotime('+1 minutes'), $page->Link(), $_SERVER['HTTP_HOST'], Director::is_https());

// Parameter based approach.
Cookie::set('nocache', '1', strtotime('+1 minutes'), $page->Link(), $_SERVER['HTTP_HOST'], Director::is_https(), true);

// Method based approach.
Cookie::setSeconds('nocache', '1', strtotime('+1 minutes'), $page->Link(), $_SERVER['HTTP_HOST'], Director::is_https());

Until this is updated, I have to do one of the following, none of which I like:

• From the unit test, manually set a global static flag to tell it not to send cookies at all (since I currently have to use the native approach to accomplish cookies that expire after a few minutes).
• Abstract this to my own function which checks global state to determine if currently unit testing and avoid setting cookies, if so.
• Manually patch core and add either my parameter approach or method approach.
• Manually patch core to convert this to using seconds and then try to find every single call to ::set() and multiply all instances of the call $expiry * 60 * 60 * 24.

EDIT: Corrected "number of seconds" to unix timestam here as well and also fixed my math in that last bullet :joy:

[patricknelson]

Another approach: Current timestamp 1460761131775. That's a huge number. If you were to pass a number that is larger time() then you're probably not trying to set number of days, or even number of years for that matter. So - maybe we can just do that check first and, if it's specified, use it literally?

For CookieJar's ->set() method: https://github.com/silverstripe/silverstripe-framework/blob/3/control/CookieJar.php#L76

//expiry === 0 is a special case where we set a cookie for the current user session
if ($expiry !== 0) {
    // Don't do the maths if we are clearing.
    if ($clear) {
        $expiry = -1;
    } elseif ($expiry >= time()) {
        // Assume this is a UNIX timestamp and leave as is.
    } else {
        // Assume days.
        $expiry = SS_Datetime::now()->Format('U') + (86400 * $expiry);
    }
}

[patricknelson]

Ok I'm annoying now, but: SS_Datetime::now()->Format('U') returns a string so it's probably best to cast explicitly to integer first before applying + :+1: Alright done!

[muskie9]

I ran into a need recently for something similar but didn't know where to start as I haven't had much experience with the cookie api. I'd be for having the ability to set cookie expiry for less than a day.

[sminnee]

I'm a bit worried about breaking the API on this one, even in a major release, because it will be likely to take people by surprise, since it won't cause obvious breakages.

There's a couple of things I can think of:

• Allowing a float value for $expiry is a bit weird but is the most backwards-compatible. In fact, although I haven't tested it but from a read of the code it looks like that's already works.
• Allowing a unit suffix such as s, m or d to the number would disambiguate. We could either incorporate some of the strtotime() logic in here or just do a simple regex decoder.

I'm less keen on either extra methods or extra arguments as it seems to make the API more bloated, which is why I'm suggesting a unit suffix.

If we thought that was a good idea, we could potentially deprecate (i.e. still works but throws a deprecation notice) the passing of a regular int/float to the method and favour a string with a unit. Since it's not an API breakage, that should be fine for the 3 branch.

But, if you're trying to get a site launched now, why not try setting expiry to 60/86400 and see if that gives you a 1 minute expiry?

[sminnee]

I'm also less keen on special magic for handing very large ints. It's true that it's unlikely that a user would be asking for an expiry billions of years in the future, but it just makes the whole API that much harder to understand. We've been burned by magic in the past...

[patricknelson]

I've already implemented a pretty stable solution with some solid unit test coverage using the UNIX time stamp technique. Still a 32 bit integer. I have it currently doing a little jig and overriding outputCookie() by reversing the multiplication if the number is too high before it passes the finals number to use to its parent implementation of the same method. Will have to post when I get to a computer.

[patricknelson]

I'm also less keen on special magic for handing very large ints. It's true that it's unlikely that a user would be asking for an expiry billions of years in the future, but it just makes the whole API that much harder to understand. We've been burned by magic in the past...

Magic? Meh... to me following the API of settime() but saying "Nah, let's make this 'number of days' instead of a unix timestamp, that'll do just fine" seems a bit more "magical" to me. This is a step in the right direction for consistency _and_ control. I think leaving this as-is makes it harder to understand for those two exact reasons. Supporting a unix timestamp will hopefully become the norm and we can fade this magic you speak of out. I know I'm not the only one (ask @muskie9 who happened to chime in).

[sminnee]

OK, I think the best way forward is to deprecate the existing functionality — throw a deprecation notice when someone passes days rather than a timestamp. Then we can remove support for days in a future version (probably SS5 rather than SS4)

[patricknelson]

So tell me if I have this right: Accept unix timestamps starting in v3, have use of days in v4 throw the deprecation notice and v5 possibly remove the days format altogether. Is that accurate?

[sminnee]

• 3 branch: accept unix timestamps, start throwing deprecation notice on days
• master branch: the same (we'll merge forward the 3 branch to apply this, no need for a 2nd PR)
• v5: remove support for days

[patricknelson]

Maybe I'm hardheaded, but... You mean, put this in the 3 branch?

Deprecation::notice('3.4', 'Specifying days as $expiry has been deprecated. Use Unix timestamps instead.');

And not:

Deprecation::notice('4.0', 'Specifying days as $expiry has been deprecated. Use Unix timestamps instead.');

My assumption was the latter, not the former. But either is fine with me I suppose. That would just require ensuring the framework has flipped over to the dark site of Unix timestamps as well (not much, really). Even Session is sort of hacking around this by taking the value and dividing it by 86400 in order to convert to a fraction of a day (passing a float, which gets multiplied back out again against 86400 and then added to a string which is then coerced to a float, but often times coerced to an integer, depending on the weather).

[sminnee]

Umm, I'm going to defer to @dhensby or @tractorcow about how we've dealt with deprecations elsewhere....

[tractorcow]

I like

Cookie::set('Name', 'Value', [ 'seconds' => $stamp ]);

$expiry could either be a literal (treated as days) or a units and value.

'timestamp' could be used to specify an explicit timestamp as well if you don't want to do a relative time.

[patricknelson]

My initial concern with this (and the major reason why I opened this ticket) was finding a path that would bring us back to the standard parameters already set out in setcookie(). This diverges even further and, in my opinion makes it even less intuitive :disappointed: So unintuitive right now, in fact, that I didn't even really realize it was possible to set expiry's less than 1 day until fairly recently when I found another instance passing a fraction of 1 (requiring manual division by s / 86400 if you want to list out seconds, m / 1440 if you want to specify minutes, etc).

[sminnee]

Yeah, my vote goes to shifting back to the same arg type as setcookie() and deprecating the current semantics.

I just was't sure what number to place in the Deprecation::notice() call.

[tractorcow]

Sure, ok. Fine with me. :D

[jakxnz]

Comprehensively changing the Cookie::set() API to follow the php setCookie() convention could have the following 2 impacts:

• The $expiry default value can't be a fixed value above 0 (because the $expiry would become relative), and would potentially best be 0
• Internal use of Cookie::set() is easy enough to refactor, but external usage would consider this a breaking change

Fortunately, displaying a Deprecation::notice() is trivial.

I've raised a PR with the working code, based on the above.

[jakxnz]

It occurs to me that some users will have registered different Cookie_Backend services. That means changing the internal use of the Cookie::set() API to supply $expiry as a timestamp will be a breaking change for those users.

Is this an affects/v4 issue? Or does this need to be re-labelled to change/major affects/v5?

[ScopeyNZ]

IMO, yeah, has to be a breaking change if you're changing the intention of any argument in an interface.

[patricknelson]

Yeah I think there's no way around it. It probably wouldn't impact me, but (for example) if someone implemented a DB driven implementation of Cookie_Backend it will not get the deprecation notice.

This may indeed need to wait until v5, unfortunately. The only way around it would be to implement a crutch/polyfill somehow in the Cookie::set() static function, which seems particularly evil and and should not be done.

[jakxnz]

I've adjusted current PR to address only the 4.x compatible changes https://github.com/silverstripe/silverstripe-framework/pull/9548

Major changes can be addressed at a later time

[dhensby]

If the main issue is "how do I set a cookie less than one day" the answer is to send a parameter with a value that is a fraction of a day, it is possible to set cookies less than a day.

If the issue is "make the API more like set_cookie" then I think I disagree. The problem is we're actually using Max-Age but we've called it $expiry which implies (wrongly) it's the equivalent of the expires directive on the cookie (see https://en.wikipedia.org/wiki/HTTP_cookie#Expires_and_Max-Age). We aren't trying to reflect the PHP API, we're for a different attribute than the PHP API.

[patricknelson]

If the main issue is "how do I set a cookie less than one day" the answer is to send a parameter with a value that is a fraction of a day, it is possible to set cookies less than a day.

Yeah, I think for me the main issue was, in a dynamically typed language, taking the doctype declaration for the parameter of int a bit too literally. If you can only define int's, you obviously can only use days, however if float's are allowed, seconds become available.

If the issue is "make the API more like set_cookie" then I think I disagree.

I concur. There are two competing ideas with some overlap, i.e. A.) What's easy/intuitive vs. B.) What's consistent? Speaking of, I proposed what I think might be a very good middle ground for not only retaining the current max-age style $expires parameter but using method chaining to accomplish a migration to an alternative API. https://github.com/silverstripe/silverstripe-framework/pull/9548#issuecomment-645525529

Also, one thing we could potentially do _right now_ would be to, at minimum, fix the documentation on the various @param int $expiry to instead say @param float $expiry since I didn't think of that until now 😄

[jakxnz]

If the main issue is "how do I set a cookie less than one day" the answer is to send a parameter with a value that is a fraction of a day, it is possible to set cookies less than a day.

@dhensby if that's acceptable, then it sounds like the rest of this conversation belongs to a separate, greater effort to implement a more comprehensive Cookie pattern and API. Can this be closed?

[sminnee]

Yeah given where things have gone I'd suggest a PR that allows for float values of relative days in addition to int. Assuming we haven't used type hints in the API to date, that shouldn't cause breakages in upgrades, either to project code or to custom cookie jars?

IMO I would implement that and then close this ticket.

If it's already implemented, then I would patch the docs and maybe add a test, and then close this ticket.

[patricknelson]

I agree we could maybe discuss expanding the Cookie API a bit more as well. I'd be interested to see where that goes. Also, I can close this ticket if/when #9552 get's merged.

EDIT: Referenced wrong PR 😬

[emteknetnz]

The linked pull-request with an update to docs has been merged, I'll close this issue now