`Illuminate\Cache\Repository#put()` type signature incompatible with `Illuminate\Support\InteractsWithTime#parseDateInterval()` leads to unexpected results

[Ocramius]

• Laravel Version: master
• PHP Version: irrelevant
• Database Driver & Version: irrelevant

Description:

Following this trace:

framework/src/Illuminate/Cache/Repository.php

Lines 191 to 199 in 5bd78e1

	/**
	* 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)

Then reaching getMinutes:

framework/src/Illuminate/Cache/Repository.php

Lines 199 to 207 in 5bd78e1

	public function put($key, $value, $minutes = null)
	{
	if (is_array($key)) {
	$result = $this->putMany($key, $value);
	return $result;
	}
	if (! is_null($minutes = $this->getMinutes($minutes))) {

Then parseDateInterval:

framework/src/Illuminate/Cache/Repository.php

Lines 573 to 581 in 5bd78e1

	/**
	* Calculate the number of minutes with the given duration.
	*
	* @param \DateTimeInterface|\DateInterval|float|int $duration
	* @return float|int|null
	*/
	protected function getMinutes($duration)
	{
	$duration = $this->parseDateInterval($duration);

Which is defined as follows:

framework/src/Illuminate/Support/InteractsWithTime.php

Lines 41 to 53 in 5bd78e1

	* If the given value is an interval, convert it to a DateTime instance.
	*
	* @param \DateTimeInterface|\DateInterval|int $delay
	* @return \DateTimeInterface|int
	*/
	protected function parseDateInterval($delay)
	{
	if ($delay instanceof DateInterval) {
	$delay = Carbon::now()->add($delay);
	}
	return $delay;
	}

There are some type issues here:

1. Illuminate\Cache\Repository#put() has a possible null as TTL
2. Illuminate\Cache\Repository#getMinutes() does not accept null as TTL, but can return null
3. Illuminate\Support\InteractsWithTime#parseDateInterval() does not support null parameters

Besides being a type declaration issue, this leads to the weird side-effect of calling Illuminate\Cache\Repository#put('foo', 'bar', null) being valid, yet not producing a cache entry.

Another (worse) design issue is that Illuminate\Cache\Repository#put('foo', 'bar') (no third argument) is valid according to specification, but effectively dead/misleading code.

This is a design issue strictly related with #10504

Possible fixes

A possible fix is to:

• introduce a deprecation warning for null being passed to the method
• add a throwing assertion on the given parameter, so that the 5 supported types of CacheRepository::put() are checked
• remove support for null in a major release (6.0)

The assertion is completely optional, as a type check in userland would suffice, but the interface is indeed too broad due to the type being too extensive.

[driesvints]

Heya, thanks for sending this in! I do however believe the current implementation to be the correct one.

Illuminate\Cache\Repository implements Illuminate\Contracts\Cache\Repository which implements Psr\SimpleCache\CacheInterface. As put is an alias of set (see below) it conforms to the same interface.

framework/src/Illuminate/Cache/Repository.php

Lines 220 to 226 in 5bd78e1

	/**
	* {@inheritdoc}
	*/
	public function set($key, $value, $ttl = null)
	{
	return $this->put($key, $value, $ttl);
	}

The description of the $ttl argument is the following:

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.

It's the may keyword here that concerns me. Judging from the sentence above there's no obligation to require a TTL or default value so a value of null is valid and just results in the passed value not being stored at all. This results in the put method returning false.

This is also the reason why the docs always have examples which explicitly add the $minutes parameter: https://laravel.com/docs/5.7/cache#storing-items-in-the-cache. This is simply because we offer no default value.

Please let me know if you see this differently and I'll gladly get to the bottom of this with you 👍

PS.: We follow a different versioning scheme than SemVer so we could perform an update in the next minor release (5.8): https://laravel.com/docs/5.7/releases#versioning-scheme

P.P.S.: One thing that does concern me is that the TTL for the PSR interface is represented in seconds instead of minutes. We might need to revise this but this is a different issue than the one above. https://www.php-fig.org/psr/psr-16/#12-definitions

[nunomaduro]

PS.: We follow a different versioning scheme than SemVer so we could perform an update in the next minor release (5.9): https://laravel.com/docs/5.7/releases#versioning-scheme

The next minor version is 5.8 - the target is laravel:master branch.

[driesvints]

@nunomaduro Yeah, sorry, that's what I meant :)

[Ocramius]

There's a 100% issue with the type declarations in there, as they are incompatible with each other (twice).

As for this bit:

I do however believe the current implementation to be the correct one.
The description of the $ttl argument is the following:

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 driver is not being reached at all, according to following block:

framework/src/Illuminate/Cache/Repository.php

Line 207 in 5bd78e1

if (! is_null($minutes = $this->getMinutes($minutes))) {

PS.: We follow a different versioning scheme than SemVer so we could perform an update in the next minor release (5.8): https://laravel.com/docs/5.7/releases#versioning-scheme

I'm aware: I was just being pushy about adopting sensible conventions for versioning.

[driesvints]

There's a 100% issue with the type declarations in there, as they are incompatible with each other (twice).

They are indeed not a one-on-one match but the general gist of handling the TTL/minutes parameter should be the same. However, you're right about the contract not having the null type as well. I'll send in a fix for this.

The driver is not being reached at all, according to following block:

Now this is indeed an issue. Thanks for pointing that out. The is_null is indeed unnecessary and indeed prevents a driver from implementing a default. I'll check if I can send in a fix for this.

I'm aware: I was just being pushy about adopting sensible conventions for versioning.

On a personal note I also am more in favor of SemVer but this has been a discussion which has been held a lot in the past. In the end we decided to go for the current one because it allows us to make minor breakages in point releases (which we do every half year) while still keeping large architectural changes for major releases. Otherwise we'd have to release a new major release every half year. Not that this is an excuse or anything, just the way we decided to handle our releases.

[driesvints]

I've made a PR which should properly implement the PSR spec: #27166

Basically since Laravel doesn't provides a default TTL of its own, it should at least allow you to create a driver which does. Maybe in a follow up PR we could add support for a default TTL.

[driesvints]

@Ocramius btw, the PR also fixes the problem addressed in the issue title as the null value will not get to the parseDateInterval method anymore.

[driesvints]

Closing this as this is fixed for the next release.