Skip to content
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

[Review Required] Drop cache adapter support with Laravel 5.8+ #73

Merged
merged 7 commits into from Oct 10, 2019

Conversation

kslimani
Copy link
Contributor

Disclaimer : It is a proposition only, i am not sure if it is the way to go.

Since Laravel 5.8, cache repository instance implements \Psr\SimpleCache\CacheInterface, so (if i am not wrong) does not require simple cache bridge anymore.

This PR is an attempt to drop these dependencies (by default) and also add support for Laravel 6.

For Laravel 5.7 or lesser it should simply require to add cache/illuminate-adapter and cache/simple-cache-bridge dependencies using composer. (i edited readme file to reflect theses changes).

If you disagree, simply close this PR. If it require some changes, just let me know !

@kslimani kslimani mentioned this pull request Sep 30, 2019
@rez1dent3 rez1dent3 mentioned this pull request Oct 3, 2019
Copy link
Owner

@florianv florianv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me after the small changes!
Thanks :)

return new SimpleCacheBridge(new IlluminateCachePool($store));
return $store instanceof \Psr\SimpleCache\CacheInterface
? $store
: new SimpleCacheBridge(new IlluminateCachePool($store->getStore()));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you throw an exception if the class does not exist with a message suggesting to install the php-cache package ?

Copy link
Contributor Author

@kslimani kslimani Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added changes in order to throw an exception if PSR-6 adapter or PSR-16 bridge is missing.

```

These dependencies are not required with Laravel 5.8 or greater.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add the same to /doc/readme.md ?

@florianv florianv merged commit 9f6f769 into florianv:master Oct 10, 2019
@florianv
Copy link
Owner

Thanks a lot @kslimani

@kslimani kslimani deleted the laravel-6 branch October 10, 2019 15:02
@josiasmontag
Copy link

Can we get a new version tagged? @florianv
Thanks! 🙂

@florianv
Copy link
Owner

@josiasmontag Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants