[5.8] Fix cache TTL compliance to PSR interface

[driesvints]

This PR does a couple of things:

• It fixes some types on certain method signatures indicating that null can be passed as well
• Fixes the return type of the add method in the cache repository when a call cascaded to the put method.
• Allows for cache drivers to implement a default TTL

The PSR spec for the TTL value goes as follows:

Optional. The TTL value of this item. If no value is sent and the driver supports TTL then the library may set a default value for it or let the driver take care of that.

The way we currently handle null TTLs in our own cache repository is that we return false and don't save anything. But according to the PSR cache repository spec there should be support for handling defaults in drivers. At the moment the null TTL isn't cascaded to the cache stores so there's no way for them to implement a default. This commit allows for the TTL to be passed to the cache stores so they may set their default TTL is they have done so. I've updated our own cache stores to return false immediately when they receive a null TTL because they don't offer a default at the moment.

It also still makes sure that for any TTL below or equal to 0 nothing will be set.

No tests were adjusted (just a really minor adjustment to a Redis test) and more tests have been added to verify the new behavior.

All in all this PR should make us conform to the PSR spec better. This PR may later be extended to allow Laravel to offer a default from its own but that's a different PR.

Fixes #27160

PS.: there seems to be an additional problem that the TTL according to the PSR spec should be in seconds while we handle it in minutes. We should maybe have a look at that as wel.

[asgrim]

Thank you for the PR @driesvints . However, I still don't think this conforms to either PSR-6 or PSR-16. All that has changed here is that instead of \Illuminate\Cache\Repository being a no-op, now every one of the cache storage adapters are no-ops.

If a calling library asks for an item to be saved but does not specify an expiration time, or specifies a null expiration time or TTL, an Implementing Library MAY use a configured default duration. If no default duration has been set, the Implementing Library MUST interpret that as a request to cache the item forever, or for as long as the underlying implementation supports.

The first sentence, you've already addressed this in the description:

This PR may later be extended to allow Laravel to offer a default from its own but that's a different PR.

Which I am fine with, however the next parts I believe are important:

If no default duration has been set, the Implementing Library MUST interpret that as a request to cache the item forever, or for as long as the underlying implementation supports.

and

If a negative or zero TTL is provided, the item MUST be deleted from the cache if it exists, as it is expired already.

To sum up, I read this as: if null is passed, you should either cache for a "default" time (which I appreciate is potentially a larger change), or cache it "forever" or "as long as the store can cache it for". If 0 or -1 (or other negative value) is passed explicitly, the we shouldn't just return false; as implemented, but actually delete the cache entry, as it is equivalent to expiring it.

This implementation, if I have understood it correctly, will: if you pass 0, -1 or null, will no-op and return false. To put into a concrete example, the behaviour of:

Cache::put('foo', 'bar');

is currently to no-op. The expectation of PSR-16 is that the value bar should be stored either for a default duration (happy to defer this), or forever/as long as possible for the foo key.

As \Illuminate\Contracts\Cache\Repository does indeed extend the PSR-16 interface \Psr\SimpleCache\CacheInterface and these are "MUST" clauses in the spec (not "SHOULD" or "MAY", see RFC-2119), I do think this should be fixed, BUT is indeed a BC break (I note, that this is targeting master anyway, which is 👍)

[driesvints]

Heya @asgrim, thanks for your feedback. I've read through the PSR and your remarks and mostly agree with them. I've tried to whip up a small example of the put method on the Illuminate\Cache\Repository which should conform to this:

    /**
     * Store an item in the cache.
     *
     * @param  string  $key
     * @param  mixed   $value
     * @param  \DateTimeInterface|\DateInterval|float|int|null  $minutes
     * @return bool
     */
    public function put($key, $value, $minutes = null)
    {
        if (is_array($key)) {
            return $this->putMany($key, $value);
        }

        if ($minutes === null) {
            return $this->forever($key, $value);
        }

        $minutes = $this->getMinutes($minutes);

        if ($minutes <= 0) {
            return $this->delete($key);
        }

        $result = $this->store->put($this->itemKey($key), $value, $minutes);

        if ($result) {
            $this->event(new KeyWritten($key, $value, $minutes));
        }

        return $result;
    }

One thing still concerns me with the above implementation: this will prevent the null TTL from cascading to the cache stores themselves (which we fixed in this PR). It's the following sentence of the TTL parameter of the set method that's now confusing to me:

If no value is sent and the driver supports TTL then the library may set a default value for it or let the driver take care of that.

In my opinion this conflicts with the expiration part of the PSR. Wouldn't the following description be better?

If no value is sent and the driver supports TTL then the library may set a default value for it or the library should cache the item forever.

Let me know what you think.

[asgrim]

If no value is sent and the driver supports TTL then the library may set a default value for it or let the driver take care of that.

I agree, it is a little confusing, and the "that" (which I bolded above) is the ambiguous article in this sentence. My interpretation is done with the assumption that most caching libraries have a "library" (that would typically implement PSR-16 CacheInterface, e.g. \Illuminate\Cache\Repository) with underlying "drivers" (or "adapters" or "storage" or whatever word you want to use, and may or may not implement PSR-16 depending on the design of the library, e.g. \Illuminate\Cache\RedisStore). When you look at it like this, that could be replaced like this:

If no value is sent and the driver supports TTL then the library (\Illuminate\Cache\Repository) may set a default value for it or let the driver (\Illuminate\Cache\RedisStore) take care of setting the default value.

That is to say, the CacheInterface implementation's set() method could determine the default value (e.g. by passing it from a config somewhere), or the actual adapter/driver/storage can determine a sane default, for example some cache stores may have different "maximum" TTLs, so perhaps "forever" doesn't always mean "forever" because some cache store may not actually support "forever".

However, this is of course based on an assumption about cache library design (that I've seen certainly in Doctrine Cache, as well as from what I've seen so far, the Laravel Cache too), which may indeed be incorrect. If this doesn't hold ground, then I'd really defer further detailing to the authors of PSR-16 for interpretation.

[driesvints]

@asgrim I see. Well in that case I believe I can revert my changes to the store implementations (because the null TTL won't cascade to them anymore) if we're going to go with the example I posted above. Do you think the example posted above would be a proper implementation of the PSR interface?

@Ocramius do you have any more remarks about the above?

[asgrim]

@driesvints yes, I think your example above is a good approach. The other option is to implement the null-handling in each individual driver ("store"), the benefit of that being that each store can make a decision on the maximum possible TTL (e.g. "forever" or whatever), but I'm going to go out on a limb without checking and assume that the Repository::forever method you use in the example does that anyway 😁 if that's the case then that implementation seems fine I think (and saves duplicating code into every adapter anyway!)

[asgrim]

For fear of being too lazy, I went and checked Repository::forever and indeed, it calls $this->store->forever and each adapter (that I checked) does indeed handle this in a reasonable manner 👍

[driesvints]

@asgrim awesome. Thanks for your reply 👍

I'm going to close this PR and open up a new one in a few days as the new implementation will differ quite a bit. This will keep it easier and more clear for Taylor to review it once it's ready.

Thanks for everyone's feedback!

[driesvints]

@asgrim @Ocramius I've sent in a new PR here: #27217

Please give feedback here if you have any more remarks and if this is what you expect 🙂