New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Flysystem caching #22310

Merged
merged 7 commits into from Dec 7, 2017

Conversation

Projects
None yet
7 participants
@jasonvarga
Contributor

jasonvarga commented Dec 5, 2017

This PR adds support for Flysystem caching without the need for a custom filesystem.

To enable it:

composer require league/flysystem-cached-adapter

Add a cache value to one of your existing disks in config/filesystems.php:

Memory

Simply add 'cache' => true to add per-request caching:

's3' => [
    'driver' => 's3',
    'key' => env('AWS_ACCESS_KEY_ID'),
    'secret' => env('AWS_SECRET_ACCESS_KEY'),
    'region' => env('AWS_DEFAULT_REGION'),
    'bucket' => env('AWS_BUCKET'),
    'cache' => true,
],

Redis (via Predis)

To use Redis, you should point to a redis-driven store defined in config/cache.php:

Cache Stores

You can use any cache store defined in config/cache.php by referencing it's name.

's3' => [
    'driver' => 's3',
    'key' => env('AWS_ACCESS_KEY_ID'),
    'secret' => env('AWS_SECRET_ACCESS_KEY'),
    'region' => env('AWS_DEFAULT_REGION'),
    'bucket' => env('AWS_BUCKET'),
    'cache' => [
        'store' => 'redis',
        'expire' => 300,
        'prefix' => 'custom-cache-key-prefix',
    ]
],

The prefix is optional. If omitted, the prefix falls back to whatever is defined in cache.php.

@sisve

This comment has been minimized.

Show comment
Hide comment
@sisve

sisve Dec 5, 2017

Contributor
  1. How would we flush the cache if needed?
  2. Why does this only support redis as a cache driver, and not all of the default drivers (database, apc, ...)?
Contributor

sisve commented Dec 5, 2017

  1. How would we flush the cache if needed?
  2. Why does this only support redis as a cache driver, and not all of the default drivers (database, apc, ...)?
@jasonvarga

This comment has been minimized.

Show comment
Hide comment
@jasonvarga

jasonvarga Dec 5, 2017

Contributor

You could flush it like Storage::disk('s3')->getAdapter()->getCache()->flush();

It doesn't support the other drivers because there's either no equivalent store class bundled with the flysystem-cached-adapter package, I don't have a way to test them, or I just don't have the time. I thought it would be better to support something than nothing.

(Also, selfishly, we only need Redis for our purposes)

Contributor

jasonvarga commented Dec 5, 2017

You could flush it like Storage::disk('s3')->getAdapter()->getCache()->flush();

It doesn't support the other drivers because there's either no equivalent store class bundled with the flysystem-cached-adapter package, I don't have a way to test them, or I just don't have the time. I thought it would be better to support something than nothing.

(Also, selfishly, we only need Redis for our purposes)

@GrahamCampbell GrahamCampbell changed the title from Flysystem caching to [5.5] Flysystem caching Dec 5, 2017

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 5, 2017

Member

Thanks but 👎. A more complete implementation is available at https://github.com/GrahamCampbell/Laravel-Flysystem/tree/master/src/Cache, and connects to any laravel cache driver.

Member

GrahamCampbell commented Dec 5, 2017

Thanks but 👎. A more complete implementation is available at https://github.com/GrahamCampbell/Laravel-Flysystem/tree/master/src/Cache, and connects to any laravel cache driver.

@jasonvarga

This comment has been minimized.

Show comment
Hide comment
@jasonvarga

jasonvarga Dec 6, 2017

Contributor

Thanks for your feedback, @GrahamCampbell !

I took a look at your package - which is awesome - and actually updated the PR using your cache class to funnel everything through to the Illuminate cache. So, credit to you on that.

Now, all the native cache stores are supported as @sisve asked.
PhpRedis is supported as @tillkruss requested.

I appreciate that the functionality exists in a third party package, but bringing this little part of it into core Laravel would be nice. It's not that much extra code.

Contributor

jasonvarga commented Dec 6, 2017

Thanks for your feedback, @GrahamCampbell !

I took a look at your package - which is awesome - and actually updated the PR using your cache class to funnel everything through to the Illuminate cache. So, credit to you on that.

Now, all the native cache stores are supported as @sisve asked.
PhpRedis is supported as @tillkruss requested.

I appreciate that the functionality exists in a third party package, but bringing this little part of it into core Laravel would be nice. It's not that much extra code.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Dec 6, 2017

Member

It may be useful to add a ->flushCache() method on the FlysystemAdapter just to avoid that kinda gnarly chaining?

Member

taylorotwell commented Dec 6, 2017

It may be useful to add a ->flushCache() method on the FlysystemAdapter just to avoid that kinda gnarly chaining?

@jasonvarga

This comment has been minimized.

Show comment
Hide comment
@jasonvarga

jasonvarga Dec 6, 2017

Contributor

Good idea. Added.

Contributor

jasonvarga commented Dec 6, 2017

Good idea. Added.

@taylorotwell taylorotwell merged commit 47fbf49 into laravel:5.5 Dec 7, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@LasseRafn

This comment has been minimized.

Show comment
Hide comment
@LasseRafn

LasseRafn Dec 12, 2017

Contributor

This is crazy cool!

Contributor

LasseRafn commented Dec 12, 2017

This is crazy cool!

{
$adapter = $this->driver->getAdapter();
if ($adapter instanceof CachedAdapter) {

This comment has been minimized.

@deleugpn

deleugpn Dec 12, 2017

Contributor

Can $driver->getAdapter() ever return something different? If not, why the if condition? If yes, what will happen then? A silent failure that lead to believe that the cache was flushed, but in fact it wasn't?

@deleugpn

deleugpn Dec 12, 2017

Contributor

Can $driver->getAdapter() ever return something different? If not, why the if condition? If yes, what will happen then? A silent failure that lead to believe that the cache was flushed, but in fact it wasn't?

This comment has been minimized.

@jasonvarga

jasonvarga Dec 12, 2017

Contributor

Only CachedAdapter classes have getCache() methods.

The alternative might be to throw an exception if it's not a cached adapter, but then you'd need to do a check in your own code. Might as well make things easier.

@jasonvarga

jasonvarga Dec 12, 2017

Contributor

Only CachedAdapter classes have getCache() methods.

The alternative might be to throw an exception if it's not a cached adapter, but then you'd need to do a check in your own code. Might as well make things easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment