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.8] Add support for custom redis driver #29275

Merged
merged 6 commits into from
Jul 30, 2019
Merged

[5.8] Add support for custom redis driver #29275

merged 6 commits into from
Jul 30, 2019

Conversation

paulhenri-l
Copy link
Contributor

@paulhenri-l paulhenri-l commented Jul 23, 2019

So as the title says, this PR adds support for custom Redis drivers.

Why though?

At the place where I work we have a very specific way of connecting to and using Redis. Unfortunately the Laravel provided drivers (phpredis and predis) do not allow us to configure the connection the way we want.

Adding in the framework a way to swap Laravel's default Connector classes with a custom one will allow anyone to connect to Redis the way they want.

As this is supported by the CacheManager I also think it could be a welcomed addition to RedisManager.

How does it work?

I've taken inspiration from the CacheManager extension system so it works pretty much the same way.

If someone wants to extend Redis with their custom Connector all they have to do is call the extend() method from within the boot() method of a service provider.

public function boot()
{
    $this->app->make('redis')->extend('my_very_specific_connnector', function () {
        return new MyVerySpecificConnector()
    });
}

Now they simply have to use it in their database.php configuration file.

<?php

    // ...

    'redis' => [
        'client' => 'my_very_specific_connnector',

        // ...
    ],
];

Contracts

I've seen that both the PredisConnector and PhpRedisConnector had the same interface so I've extracted a Connector contract to make it easier for people to write their own Connector.

Tests

Tests have also been added for this feature.

Improvements

I feel there is too many names for the same thing:

  • in RedisManager the Connector name is stored in the $this->driver property.
  • in database.php the Connector name is stored in the client key

That is: client, driver and Conector are all used to refer to the same thing. Maybe we could use this PR to change that as-well. If you feel it is needed.

We'll have to look at the best way to make it backward compatible though.

Final notes

I hope this will help anyone in the same situation :)
If you need changes in the PR do not hesitate I will do it.

Copy link
Contributor

@seriquynh seriquynh left a comment

Choose a reason for hiding this comment

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

Bringing your idea to your own package first may be a better choice.

@paulhenri-l
Copy link
Contributor Author

paulhenri-l commented Jul 24, 2019

@XuanQuynh Well that’s the goal, it is currently impossible to use custom connectors.

If this get merged people will have the possibility to create custom packages.

The only way of doing approximately the same thing right now is by completely swapping out the default RedisManager for a new one supporting extension.

I feel that’s a lot of work for something that should probably be supported in the framework. As it already is supported by the CacheManager.

One of the things that I really love about Laravel is how it never gets in my way and allow me to do whatever I want with it.

That being said when I’ve been confronted to this issue I felt that the framework did got in my way by not allowing me to swap out one of its default implementation.

I sincerely think this should be implemented. We cannot lock people on a certain implementation of a class. Just as rails, we should provide “good defaults” and make it possible to use custom code.

*
* @param array $config
* @param array $options
* @return \Illuminate\Redis\Connections\Connection
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've realized that \Illuminate\Contracts\Redis\Connection is not sufficient.

RedisManager has a method type hinted on \Illuminate\Redis\Connections\Connection.

It also appears that \Illuminate\Contracts\Redis\Connection does not have all of the methods defined in \Illuminate\Redis\Connections\Connection.

So extensions will need to subclass \Illuminate\Redis\Connections\Connection. That does not seems to be a problem as there are no private methods or properties.

The ideal solution would be to change the interface but that would break backwards compatibility.

We can also add a new interface inheriting from this one \Illuminate\Contracts\Redis\Connection. But having two interface for the same thing does not sounds ideal.

@taylorotwell taylorotwell merged commit 5e3ebc5 into laravel:5.8 Jul 30, 2019
@paulhenri-l paulhenri-l deleted the add-support-for-custom-redis-driver branch July 30, 2019 14:13
josiasmontag added a commit to josiasmontag/laravel-redis-mock that referenced this pull request Aug 2, 2019
Since Laravel 5.8.30 it is possible to register a custom Redis driver. No need to overwrite RedisManager any more.

laravel/framework#29275
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.

3 participants