Laravel cache duration changes

[Quezler]

https://github.com/Kevinrob/guzzle-cache-middleware/blob/master/src/Storage/LaravelCacheStorage.php#L47

https://laravel-news.com/cache-ttl-change-coming-to-laravel-5-8

[ymilin]

Hi @Kevinrob,

As a Laravel 5.8 user, I would like this to be fixed and would gladly work on a PR for it.

Reverting #74 would be a BC break for Laravel < 5.8 users which is not acceptable since 5.5, 5.6 and 5.7 are still supported.

A possible fix would mean to:

• release a new major version with "illuminate/cache": "^5.8" in composer.json and LaravelCacheStorage: Fix cache age calculation #74 reverted
• release a patch on the current 3.x branch with "illuminate/cache": ">=5.0 <5.8"

I'm not sure you would accept to go this route just for such a small change. What do you think?

[grray]

I propose this PR #120, which just checks Laravel version.

[Kevinrob]

Thanks @grray for this PR 👍

@Quezler @ymilin Is it good for you too?

[Quezler]

i'll have to check it out 😇

[ymilin]

Hi @Kevinrob,

Since then, I ended up using my own version of this middleware until this can be resolved.

@grray idea is pretty good but has 2 drawbacks:

• It doesn't work with Lumen.
  Lumen's version() returns a non standard version while Laravel does.

https://github.com/laravel/lumen-framework/blob/ffa356c72e374b6a2c4e8880378f3133cb3b7385/src/Application.php#L136-L144

• It won't work for users that overrides Application::version()
  I personally do this in my Lumen based APIs to make all micro-services have a GET /version that returns project-name-x.y.z

@grray 's version definitely works for Laravel based apps that doesn't override Application::version()

I still think that having separate branches would be best and support for Laravel 5.X will end soon

5.5 (LTS) : August 30th, 2020
5.8 : February 26th, 2020

[Kevinrob]

@ymilin Thank you for those information! I never used Laravel and Lumen.
Maybe we should release different package for each storage? One for the main library and one for each storage (Larvel, etc...). With that, we can release multiple version for each Laravel. What do you thing?

[ymilin]

Sorry for bothering you with the Laravel/Lumen specifics 😅

Laravel is the full framework and Lumen is the official micro-framework maintained by Laravel. They share a lot in common (basically everything under the \Illuminate namespace) but have some subtle differences especially when using helpers like app()->version() for example.

Since the caching implementation is under the \Illuminate namespace, it doesn't make sense to have separate guzzle cache implementations for Laravel and Lumen. They should use the same and I think the name LaravelCacheStorage for both Laravel and Lumen is correct since Laravel is the main project.

So I think using the version_compare on app()->version() is not robust enough as it makes our implementation depend on the framework instead of the common illuminate/cache (and app()->version() output can be tampered with by developers with a simple class overload)

Silly me, I just saw that my original proposal doesn't work because illuminate/cache is not a direct dependency of your project but a dev one 🤷‍♂

Maybe we should just release two LaravelCacheStorage, one in minutes and one in seconds and drop the minute one after Laravel stops supporting any <5.8 versions (end of august 2020)

The issue I see is that the one in second should be the main one named LaravelCacheStorage but is a huge BC break for your library.

[grray]

That's a nice discussion, guys, but keep in mind, that LaravelCacheStorage is still broken in Laravel 5.8+, and it's hard to debug (or find corresponding bug on github) because it looks like library doesn't work at all (because short ttl/60 will be expired almost immediately). So, if it's hard to find perfect solution, we should have at least good enough one, and new devs will not waste time figuring it out 😄

[ymilin]

How about checking version's format before using version_compare?

protected function getTTL(CacheEntry $data): int
{
    $version = app()->version();

    if (preg_match('/\d+\.\d+\.\d+/', $version) !== 1) {
        return $data->getTTL();
    }

    if (version_compare($version, '5.8.0') < 0) {
        return $data->getTTL() / 60;
    }

    return $data->getTTL();
}

• Works for all Laravel apps
• Works for Lumen >= 5.8
• Uses seconds where version is not a SemVer string

[Kevinrob]

Yeah, it's a good compromise!
What about:

protected function getTTL(CacheEntry $data): int
{
    $version = app()->version();
    if (preg_match('/\d+\.\d+\.\d+/', $version) && version_compare($version, '5.8.0') < 0) {
        return $data->getTTL() / 60;
    }

    return $data->getTTL();
}

[ymilin]

Yeah even better 👍

[Kevinrob]

@grray @Quezler okay for you?

[ymilin]

Hi,

There is a small mistake in my regex, it should be /^\d+\.\d+\.\d+/.

Ensuring start of the string is the SemVer string is important as the current Lumen version is 'Lumen (6.2.0) (Laravel Components ^6.0)' which would match the initial regex and make version_compare return -1.

[grray]

@grray @Quezler okay for you?

Looks good