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

[5.3] Added support for PhpRedis #14850

Closed
wants to merge 249 commits into from
Closed

Conversation

tillkruss
Copy link
Contributor

@tillkruss tillkruss commented Aug 16, 2016

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 and PhpRedisDatabase were introduced, extending Illuminate\Redis\Database
  • Laravel uses Illuminate\Contracts\Redis\Database instead of Illuminate\Redis\Database in most places.
  • Illuminate/Cache/RedisStore and Illuminate/Cache/Repository now check key existence using null or false.
  • Laravel will use Predis as default unless the config value database.redis.client is set to phpredis.
  • Predis connection parameters are mapped to the slightly different Redis and RedisCluster parameters.

A few things are still unfinished:

  • New tests for PhpRedis
  • Redis Facade when using PhpRedis
  • Alias for redis in src/Illuminate/Foundation/Application.php

@GrahamCampbell
Copy link
Member

Doesn't the redis php library already use that extention if it's present?

@GrahamCampbell GrahamCampbell changed the title Added support for PhpRedis [5.3] Added support for PhpRedis Aug 16, 2016
@tillkruss
Copy link
Contributor Author

Predis doesn't fallback to PhpRedis.

@taylorotwell
Copy link
Member

Oh. This was a lot more involved than I thought.

@GrahamCampbell
Copy link
Member

Redis Facade when using PhpRedis

Why do we need two facades?

@tillkruss
Copy link
Contributor Author

What I meant by that is, that we need to make sure that Redis facade is working when using PhpRedis.

@taylorotwell
Copy link
Member

Looks like a decent start and the performance gains would probably be nice for people who use Redis heavily.

@GrahamCampbell
Copy link
Member

Why not contribute this driver to the other project so everyone benefits?

@tillkruss
Copy link
Contributor Author

@taylorotwell Great, I'll test, improve and finish this up today.

@GrahamCampbell: Other project?

@GrahamCampbell
Copy link
Member

The PHP library we're using which already supports one of the native redis extensions.

@tillkruss
Copy link
Contributor Author

@GrahamCampbell Do you have a link for more information on that?

@GrahamCampbell
Copy link
Member

@GrahamCampbell
Copy link
Member

Also, right at the start of their readme:

Predis does not require any additional C extension by default, but it can be optionally paired with phpiredis to lower the overhead of the serialization and parsing of the Redis RESP Protocol.

:P

@tillkruss
Copy link
Contributor Author

tillkruss commented Aug 18, 2016

@taylorotwell

I've added separate subscribe() and psubscribe() methods for Predis and PhpRedis.

The client key is now pulled from the config in the RedisServiceProvider.

I've pointed the core alias for redis to the abstract Illuminate\Redis\Database class. I'm not sure if that's correct, or if we need a Redis Database Manager or similar.

@tillkruss
Copy link
Contributor Author

To test/use this:

  • Rename (or uncomment) Redis facade in config/app.php.
  • Add 'client' => 'phpredis', to redis array in config/database.php.

@GrahamCampbell
Copy link
Member

Please rebase. :)

@GrahamCampbell
Copy link
Member

Without merge commits please.

@tillkruss tillkruss force-pushed the phpredis branch 2 times, most recently from 911f8fb to 4359ffe Compare August 30, 2016 15:25
eriktisme and others added 23 commits August 30, 2016 08:28
* Add missing methods

* fix docblock
Export phpunit.xml.dist instead of phpunit.xml.
Update the author email address to taylor@laravel.com instead of gmail.
* Add withoutTrashed method to SoftDeletingScope

* Apply StyleCI patch
* Change read to timestamp from boolean

* Use timestamps not datetime

* Dont use carbon, instead use freshTimstamp()

* Use read_at not read
@tillkruss tillkruss force-pushed the phpredis branch 2 times, most recently from 8ee8052 to 6cdef4b Compare August 30, 2016 15:32
@tillkruss tillkruss closed this Aug 30, 2016
@tillkruss tillkruss deleted the phpredis branch August 30, 2016 15:55
@tillkruss
Copy link
Contributor Author

@GrahamCampbell I messed up the branch and reopened another PR.

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.