[5.5] Fixed incorrect implementation of PSR-16

[GrahamCampbell]

You may have noticed StyleCI was down yesterday repeatedly. This was caused by this bug causing our cache expiration to be much too long which resulted in the redis hogging all the resources and ultimately bringing everything down with it. 48 hours expiration was being treated as 120 days instead!

PSR-16 mandates that set and setMultiple have their TTL specified in seconds, but Laravel's put and putMany methods expect minutes, so a correct implementation must compensate for this by converting seconds to minutes before passing to put or putMany.

---

// cc @joecohens

[driesvints]

There's a little more to it than just these two lines I'm afraid. Here's my PR which fixed this for 5.8: #27276

I'm not sure if it's wanted to make these breaking changes on a minor release? People updating their apps will see their TTL being configured wrong.

Besides that supports for 5.5 ends this month.

[GrahamCampbell]

These changes are not a breaking, but a bug fix (well, every bug fix is breaking technically :P). Laravel's cache claims it implements PSR-16, but it does not, so when I use it with code that expects a PSR-16 implementation, it silently does the wrong thing.

I know Laravel 5.8 uses seconds across the board instead of minutes for caching, however I'm not proposing we change that in 5.5. I'm only asking that we fix the bug of set and setMultiple.

I don't anciptiate this change will affect any other 5.5 users because 99.999% of people will be calling put and putMany. Only those who are using it as a PSR-16 instance will be using set and setMultiple, and we need to behaves correctly for those users surely?

[GrahamCampbell]

That is, anyone calling set and setMultiple already will think it uses seconds, because the phpdoc on that method says it uses seconds (inheritdoc is also a gripe of mine... if this doc had been copy-pasted over i bet this bug would have never happened, because it says seconds not minutes in there).

[taylorotwell]

Do we ever claim to implement PSR-16 in the 5.5 cache? I can't find where we ever implement the interface.

[GrahamCampbell]

Do we ever claim to implement PSR-16 in the 5.5 cache? I can't find where we ever implement the interface.

Yeh. The cache repository contract extends the PSR one: https://github.com/laravel/framework/blob/5.5/src/Illuminate/Contracts/Cache/Repository.php.

[GrahamCampbell]

Originally implemented (poorly) in #20194. Since that original PR, I have fixed 2 important bugs (#29038, #29046). This PR should fix the remaining issue with the TTL not being handled correctly.

[GrahamCampbell]

Maybe some more context will help here actually. I am using the fact that Laravel's cache repository is a PSR-16 cache implementation in order to cache http requests to github.com as part of https://github.com/GrahamCampbell/Laravel-GitHub. Under the hood, it uses a PSR HTTP client and expects PSR-6 cache implementation. Symfony have a bridge that makes PSR-16 implementations look like PSR-6, so we can setup the caching very easily (https://github.com/GrahamCampbell/Laravel-GitHub/blob/master/src/GitHubFactory.php#L107-L119). N.B. Laravel's store method on the cache factory actually returns a repository and not a store, which is a little confusing to read. :P

[GrahamCampbell]

Besides that supports for 5.5 ends this month.

With no LTS to replace it?

[driesvints]

@GrahamCampbell 6.0 will be LTS

[GrahamCampbell]

Oh? Kinda odd though, given that there will be no breaking changes in 6.1, 6.2,... only new features. Maybe LTS makes no sense, and instead Laravel should just rebrand LTS, replacing it with the fact that each major release will be around for a while, and then, similar to how symfony does things, once a new major release is around, support the previous major for a bit, calling it LTS, or whateve?

[driesvints]

@GrahamCampbell major versions always receive 6 months of bug fixes. That's always been the case. Nothing changes here. 5.5 was the previous major version (old versioning scheme) that was LTS. Now the new 6.0 major version (SemVer) will be LTS.

[GrahamCampbell]

By major, I mean in the semver sense, where Laravel 5.6 and 5.7 are minor releases.

[GrahamCampbell]

It makes no sense for 6.0 to be LTS but 6.1 not to be, surely. Surely we'd have to call 6.x LTS, which just means every release is effectively LTS, so we need some rebranding of LTS if you see what I mean...

[driesvints]

Laravel 5.6 and 5.7 are major releases in the old versioning scheme. See here: https://laravel.com/docs/5.8/releases#versioning-scheme

It makes no sense for 6.0 to be LTS but 6.1 not to be, surely

Ah I see what you mean. Yeah indeed. I meant 6.x of course, sorry for any confusion :')