search

sausin / laravel-ovh

Wrapper for OVH Object Storage integration with laravel
MIT License
37 stars 12 forks source link

Add cache option #52

Closed wfeller closed 4 years ago

wfeller commented 4 years ago

Is it useful to support multiple cache drivers like Laravel or would it be enough to simply only use the memory cache?

Solves #51

iksaku commented 4 years ago

I think using a Generic Cache adapter could suffice... Lets hear what @sausin thinks about this

sausin commented 4 years ago

Loving it!! I think the generic adapter looks good. ~We're primarily a laravel package but no harm in making things more generic if it's possible~ :+1:

@wfeller Could you consider some tests for it?

EDIT:- Had misread what you meant. I think memory cache support is sufficient for now. We can consider additional complexity with other drivers if times call for it

sausin commented 4 years ago

Also just noticed, shouldn't the new dependencies be in require instead of require-dev ?

wfeller commented 4 years ago

No, the other dependencies are only required if the users want to use the cache features, in which case they can install them themselves (I'll add a suggest part in composer.json like Laravel does it - see https://github.com/laravel/framework/blob/1dc032a15037330c5065b84ce3937d075a673b5d/composer.json#L130).

I'll update the PR to only support memory cache + tests tonight or tomorrow.

sausin commented 4 years ago

I see, makes sense. We should reflect the same in the config documentation as well :+1:

wfeller commented 4 years ago

@sausin @iksaku Please take a look at the PR and let me know if you think anything should be updated, otherwise I believe it's good to go.

sausin commented 4 years ago

Looks good to me. Your thoughts @iksaku ?