Add Laravel cache persistence #13
Conversation
Adds a persistence driver for Laravel
kamermans
left a comment
kamermans
left a comment
There was a problem hiding this comment.
Thanks for the contribution @threesquared, and sorry it took so long for me to review it! I'm happy to pull it in once the concerns addressed.
|
|
||
| public function deleteToken() | ||
| { | ||
| $this->cache->delete($this->cacheKey); |
There was a problem hiding this comment.
This line is causing the following error in some environments:
1) kamermans\OAuth2\Tests\Persistence\LaravelCacheTokenPersistenceTest::testDeleteToken
call_user_func_array() expects parameter 1 to be a valid callback, class 'Illuminate\Cache\ArrayStore' does not have a method 'delete'
/home/travis/build/kamermans/guzzle-oauth2-subscriber/guzzle_environments/6/vendor/illuminate/cache/Repository.php:487
/home/travis/build/kamermans/guzzle-oauth2-subscriber/src/Persistence/LaravelCacheTokenPersistence.php:44
/home/travis/build/kamermans/guzzle-oauth2-subscriber/src/Persistence/LaravelCacheTokenPersistence.php:44
/home/travis/build/kamermans/guzzle-oauth2-subscriber/tests/Persistence/TokenPersistenceTestBase.php:73
It seems the correct method is forget(), not delete(), but then I don't understand how the tests in the other environments pass?
There was a problem hiding this comment.
I see, the delete() method was added in Laravel 5.5 in July 2017 (laravel/framework#20194) to satisfy SimpleCache / PSR-16, but it's just an alias for forget(), so I guess that is still an acceptable method to use.
| use Illuminate\Contracts\Cache\Repository; | ||
| use kamermans\OAuth2\Token\TokenInterface; | ||
|
|
||
| class LaravelCacheTokenPersistence implements TokenPersistenceInterface |
There was a problem hiding this comment.
Do you think perhaps this should contain the major version number, like Laravel5CacheTokenPersistence, just because Laravel is so prone to making breaking changes? We used to be flying through versions in Laravel although now it seems to have settled on 5.x for a few years.
|
No worries. Updated with your comments now. |
2 participants
Adds a persistence driver for Laravel