Altered CookieJar behaviour to follow php setCookie

[jakxnz]

#5325

Refactored CookieJar::set() expiry convention to follow php setCookie()'s $expires argument (https://www.php.net/manual/en/function.setcookie.php)

The impact here is not insignificant. Because timestamp is relative, there's no meaningful default value to use, unless we use a relative value internally. The comments in the issue specifically suggest using the same arg type as php, so I chose to do that.

The expected impact is low, the changes simply now allow users to supply $expiry expressed in days or a Unix timestamp

The resulting internal behaviour is the same, but the Cookie::set() API has changed to require a Unix timestamp.

Also tweaked internal use of CookieAuthenticationHandler::clearCookies() to not supply null as $expiry

[lozcalver]

We’ll need to preserve backward compatibility with passing a number of days if we’re changing this in 4.x, or people will upgrade and suddenly find their cookies expiring.

I think we should be safe to rely on $expiry < time() to check whether someone has passed a number of days or a timestamp. It’s unlikely someone is intentionally setting a cookie to expire in 1.5 billion days 😛

[patricknelson]

@kinglozzer makes a very good point about backward compatibility. Also, it turns out that the interface documentation for Cookie_Backend::set() might need to get updated too (just to reduce confusion). Other than that, this looks good so far and was a long time coming! 👍

[sminnee]

We could potentially do:

• a deprecation noticed for use of values that appear to use relative days instead of timestamps
• a config option (false by default) that forces the use of timestamps over relative days

A bit more cruft involved, but it means that projects that want to force the use of timestamps across the board are able to do so on their projects.

EDIT: Upon reflection I don't think a config option is a good idea, as it increases the change of bugs when using modules that either require this setting or can't work with it.

One case I'd be marginally worried about is if someone has used "a million/billion days" or something as signifying "forever". The current date is about 1.592 billion; 1 billion is 9 sept 2001. Assuming anything less than today's timestamp is a relative date might cause bugs; perhaps we say anything <= 1,000,000,000 is treated as relative?

The other case that might be a bit unclear is passing "0". That would probably need to be interpreted as time(); it's possible that some people are trying to set the expiry date to the epoch as a way of saying "expire immediately". This kind of short-hand won't be available to developers.

We should update the docs to explain these cases.

[jakxnz]

I'm happy to implement any of the above.

One case I'd be marginally worried about is if someone has used "a million/billion days" or something as signifying "forever".

I agree, not supporting large numbers as an expression of "forever number of days" does have the slim potential of being a breaking change.

The other case that might be a bit unclear is passing "0". That would probably need to be interpreted as time();

@sminnee sounds good. Just to be double sure, is that another way of saying what the php docs are saying? i.e

If set to 0, or omitted, the cookie will expire at the end of the session (when the browser closes).

If we want to make this change with 4.x compatibility, can I propose we...

• Use 0 in the same way php does (i.e expires at end of session)
• Default $expiry parameter to -2 i.e set(..., $expiry = -2, ...)
• Add a 4.x considerations in which we...
  • Check for small numbers (< DBDatetime::now()->getTimestamp()), and support them as days with a deprecation warning
  • Add a config parameter to suppress deprecation warnings
  • Switch a -2 $expiry value to the default number of days
  • Hook an extension point if value is >= DBDatetime::now()->getTimestamp() so users can clarify large numbers (if needed)
• Write decent docs
• Raise an enhancement issue to remove backwards compatibility for next major version (i.e 5.x)

Note: -1 is assigned if ($value === false || $value === '' || $expiry < 0) { aka "clearing the cookie", so there is also the capacity for users to have set a large negative number as well. I believe it's within reasonable parameters to assume they have not.

[jakxnz]

This solution doesn't have comprehensive backwards compatibility. I've commented on this in the issue #5325 (comment)

[ScopeyNZ]

As long as we maintain backwards compatibility, I think it's fine to make this change in a minor and deprecate the existing usage.

• Use 0 in the same way php does (i.e expires at end of session)

Agreed. We can handle 0 the same for both "time" and "day" mode

• Default $expiry parameter to -2 i.e set(..., $expiry = -2, ...)

I feel like we should leave the default as 90 for now.

• Add a 4.x considerations in which we...
  • Check for small numbers (< DBDatetime::now()->getTimestamp()), and support them as days with a deprecation warning
  • Add a config parameter to suppress deprecation warnings

This isn't necessary if you use in current Deprecation API.

• Switch a -2 $expiry value to the default number of days

See above

• Hook an extension point if value is >= DBDatetime::now()->getTimestamp() so users can clarify large numbers (if needed)

I really wonder if it's worth the extra API for this incredibly unlikely scenario.

• Write decent docs
• Raise an enhancement issue to remove backwards compatibility for next major version (i.e 5.x)

You're welcome to just PR this for master 😄

Sounds good to me.

[patricknelson]

My only concern (where now I concur with @jakxnz's point from 2 days ago) is that any custom implementations of Cookie_Backend will not receive this change, causing inconsistencies between any user code and core code which will have assumed now that input is in UNIX timestamp instead of number of days (when the custom implementation will still be treating it as days and will not have the deprecation notice).

So to be clear, I think even this deprecation should be flagged v5 (and the notice potentially remain in place until even later version[s]) 😞

[ScopeyNZ]

I'm torn on this. I think the consistency with PHP is a good goal, and I'm in favour with the general intention.

I think we can probably not change the interface (or the intention of the argument in the interface), support the new and old method, have some indication that the new "timestamp" method is the way it'll be in SS5, and ideally have some sort of deprecation notice about it.

We can change it in SS5, but I'm not keen on doing that with some sort of deprecation guiding developers through the process.

Otherwise, I'll be in favour of deprecating in SS5, and changing the SS6. And then banging the old drum about shorter major release cycles 😉

[patricknelson]

And then banging the old drum about shorter major release cycles

/me trembles ... I'm still stuck on SS 3.x. 😄 I'm all for progress, but that's why I'm always so super careful about using only well defined API's, not touching core and etc. The less dramatic the changes are, the easier it is to modernize. Iterating too quickly on major versions (or at least being too disruptive) can actually have the ironic effect of discouraging upgrades.

[ScopeyNZ]

I don't want to digress too much so I'll just say this:

The idea with shorter major release cycles is that you have a much more palatable list of breaking changes to deal with. While you might be right, where some teams don't have a workflow that allows them to stay on top of more frequent major updates, I think the majority would be more accepting of a days work every 6 months or so to stay on top of changes, with the benefit of the SS codebase being a little nicer in terms of semantics and simplicity (like this PR aims to resolve).

[patricknelson]

Absolutely @ScopeyNZ that's a sensible way to approach it. 😄

[jakxnz]

Updated the PR description:

#5325

Refactored CookieJar::set() expiry convention to follow php setCookie()'s $expires argument (https://www.php.net/manual/en/function.setcookie.php)

The impact here is not insignificant. Because timestamp is relative, there's no meaningful default value to use, unless we use a relative value internally. The comments in the issue specifically suggest using the same arg type as php, so I chose to do that.

The expected impact is low, the changes simply now allow users to supply $expiry expressed in days or a Unix timestamp

The resulting internal behaviour is the same, but the Cookie::set() API has changed to require a Unix timestamp.

Also tweaked internal use of CookieAuthenticationHandler::clearCookies() to not supply null as $expiry

[lozcalver]

Having different functionality like this if you switch implementations of an interface isn’t ideal, but I can’t see any way to introduce a deprecation cycle without ending up with that at some point.

I wonder if we could fix the interface in 5.0 by adding a deprecation notice in this PR to Cookie::set() (rather than CookieJar::set()), then changing the interface, Cookie::set() docs and core usage in 5.0? That, and a note in the docs, should offer the upgrade guidance @ScopeyNZ mentioned.

The only drawback I can see is that there won’t actually be any way to fix the deprecation notice because core code will have to keep passing a number of days (for BC) until 5.0 is released. Again, I can’t see any way around that though even if we delay this until 5.0 or 6.0...

[dhensby]

This change also makes me nervous; mostly because of BC issues. Our Cookie layer isn't that advanced and I regularly completely circumvent it with my own Cookie_Backend, so internal changes that would expect custom Backends to know that we now accept unix and 0 instead of null would be a big pain point for people who implement a custom backend.

The Cookie helper is just a bit too naive and that forces people who want to automatically set the secure flag on HTTPS connections or set the SameSite directive have to implement a custom solution.

Whilst consistency with the underlying PHP APIs can be a worthwhile goal, I'm not sure that we need to be completely compatible with them for its own sake. I wouldn't use this as a justification for large disruption.

There's a lot of work to improve cookies and I don't think this would be a change that should make it's way to 5.x because out cookie approach needs modernising.

Other things that need addressing:

• Setting of secure cookies automatically
• Setting of additional directives (SameSite specifically, but also any new ones that are added over time)
• How cookies are sent to the browser (at the moment we use setcookie but this has limitations and drawbacks that mean we lose control of how the cookies are set / they are out of scope of middlewares and other request handlers)
• Session cookies are disconnected from our cookie layer, making management of them unnecessarily difficult

---

My point being that we shouldn't implement something painful for 4.x users when the likelihood is that in 5.x it won't even be a linear upgrade path

[dhensby]

I'd also point out that some of the confusion is likely coming from the fact that our implementation is effectively setting the Max-Age attribute (time until expiry) vs the Expires attribute which the PHP API is asking for. To try to change our implementation to reflect setcookie is a slightly misguided attempt to reflect an API that we are not trying to mimic.

The decision was likely made at the time because it's a more common use-case as a developer to want a cookie to expire in 30 days than it is to want a cookie to expire on 1st July 2020 12:34:03+1200 (conceptually speaking)

[dhensby]

My view is that we shouldn't make this change. I don't think it provides any tangible value (sub day cookies can be set with some simple maths), it adds a changeable API where it's inferred that the expiry argument changes between expires and max-age on the cookie (where as before it was consistently max-age).

[patricknelson]

Here's a crazy idea (ready to be shot down instantly 😄): Could you change the interface to require a return type declaration so you can use chaining to define additional parameters? e.g. Laravel also uses the max-age style (in this case, via minutes) and you could potentially encourage people to start calling Cookie::set('name', 'val') leaving out the expiration parameter which, for historical reasons, currently takes $days and instead use the new ->expires($minutes) chain method and etc.

You could then (in future versions, e.g. 5 and onward) due to the API breakage, add the following to your Cookie_Backend interface:

    public function set(
        $name,
        $value,
        $expiry = 90,
        $path = null,
        $domain = null,
        $secure = false,
        $httpOnly = true
    ) : Cookie_Backend;

That way you can use the extensibility of PHP to continue using the return type to then use more methods to add more functionality as needed. Thoughts?

[jakxnz]

@patricknelson if there will be an effort to expand the Cookie API, then it could be assigned to establishing a more comprehensive Cookie pattern and API.

I don't see enough clarity in the conversation for a pull request of this nature, so I'm going to close this.

I have raised #9552 to request a "docs only" enhancement.