-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.4] Add support for PhpRedis #15160
Conversation
@@ -31,12 +31,12 @@ class RedisStore extends TaggableStore implements Store | |||
/** | |||
* Create a new Redis store. | |||
* | |||
* @param \Illuminate\Redis\Database $redis | |||
* @param \Illuminate\Contracts\Redis\Database $redis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've set the target of this PR to 5.4 due to this breaking change.
Is it possible for us to get integration tests on this? |
We'll also need some docs on the master branch of the docs. Thanks! |
Sure thing. |
*/ | ||
public function psubscribe($channels, Closure $callback, $connection = null) | ||
{ | ||
$this->psubscribe($channels, $callback, $connection, __FUNCTION__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a typo, should be $this->subscribe(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please send a PR. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Now I am using laravel 5.3, is this feature can support 5.3 ? |
It's coming in 5.4. For 5.3 you need to use this package https://github.com/tillkruss/laravel-phpredis |
@tillkruss Can you please tell why you made this change:
I think it should be reverted. |
I messed up the branch and didn't know how to recover. This is the original PR: #14850
Followup from laravel/docs#2500 to add PhpRedis support to Laravel, because it's significatly faster than Predis.
This is a proof of concept. Feedback/suggestions are greatly appreciated.
Illuminate\Redis\Database
is now an abstract class.PredisDatabase
andPhpRedisDatabase
were introduced, extendingIlluminate\Redis\Database
Illuminate\Contracts\Redis\Database
instead ofIlluminate\Redis\Database
in most places.Illuminate/Cache/RedisStore
andIlluminate/Cache/Repository
now check key existence usingnull
orfalse
.database.redis.client
is set tophpredis
.Redis
andRedisCluster
parameters.redis
to the abstractIlluminate\Redis\Database
class. I'm not sure if that's correct, or if we need a Redis Database Manager or similar.To test/use this:
Redis
facade inconfig/app.php
.'client' => 'phpredis',
toredis
array inconfig/database.php
.