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

[8.x] Make it possible to use prefixes on Predis per Connection #40083

Merged
merged 4 commits into from
Dec 17, 2021
Merged

[8.x] Make it possible to use prefixes on Predis per Connection #40083

merged 4 commits into from
Dec 17, 2021

Conversation

tben
Copy link
Contributor

@tben tben commented Dec 17, 2021

This PR allows the ability to use prefixes on a per connection basis for Predis.

Basically the only way to setup a prefix is by doing the following ['redis' => ['options' => ['prefix' => 'prefix:']]] in the /config/database.php file.

So copying a similar setup in PHPRedis. Each connection can be given a unique prefix by defining them within the individual connection config like so ['redis' => ['connection1' => ['prefix' => 'prefix:'], 'connection2' => ['prefix' => 'prefix2:']]].

@@ -42,6 +42,10 @@ public function connectToCluster(array $config, array $clusterOptions, array $op
{
$clusterSpecificOptions = Arr::pull($config, 'options', []);

if (! empty($config['prefix'])) {
Copy link
Member

Choose a reason for hiding this comment

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

empty is not the right function here. it will not permit using 0 as a prefix.

Copy link
Contributor Author

@tben tben Dec 17, 2021

Choose a reason for hiding this comment

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

I simply copied the code from

if (! empty($config['prefix'])) {

Do we want to allow zeros within PHPRedis Connector as well?

To allow the use of zero. Change the function from empty to isset
@taylorotwell
Copy link
Member

Please share what the actual formatted Redis configuration would look like using these new options. Please mark as ready for review when you have done so.

@taylorotwell taylorotwell marked this pull request as draft December 17, 2021 16:41
@tben
Copy link
Contributor Author

tben commented Dec 17, 2021

Assuming I'm understanding your question correct. The formatted Redis configuration would look like:

/config/database.php

'redis' => [
    'client' => env('REDIS_CLIENT', 'predis'),

    'default' => [
        'host' => env('REDIS_HOST', '127.0.0.1'),
        'password' => env('REDIS_PASSWORD', null),
        'port' => env('REDIS_PORT', 6379),
        'database' => env('REDIS_DB', 0),
        'prefix' => env('REDIS_PREFIX', 'prefix:'),
    ],

    'cache' => [
        'host' => env('REDIS_HOST', '127.0.0.1'),
        'password' => env('REDIS_PASSWORD', null),
        'port' => env('REDIS_PORT', 6379),
        'database' => env('REDIS_CACHE_DB', 1),
        'prefix' => env('REDIS_PREFIX', 'prefix2:'),
    ],
],

@tben tben marked this pull request as ready for review December 17, 2021 16:59
@taylorotwell taylorotwell merged commit 33fb408 into laravel:8.x Dec 17, 2021
@tben tben deleted the patch-1 branch December 18, 2021 18:52
@jasonvarga
Copy link
Contributor

jasonvarga commented Apr 6, 2022

We were just looking into this and noticed it was already possible to put a prefix on a specific store. You just had to nest it under an options array. This works for both phpredis and predis clients.

'redis' => [
    'cache' => [
        'host' => env('REDIS_HOST', '127.0.0.1'),
        'password' => env('REDIS_PASSWORD', null),
        'port' => env('REDIS_PORT', 6379),
        'database' => env('REDIS_CACHE_DB', 1),
        
        'options' => [ // 👈 you needed this
            'prefix' => env('REDIS_PREFIX', 'prefix2:'),
        ]
    ],
],

So now this PR introduces an inconsistency. You can now add un-nested prefix keys to predis stores, but not phpredis.

Although my first instinct was to plop prefix right onto the store, too. Maybe we should just apply this to the phpredis connector.

@tben
Copy link
Contributor Author

tben commented Apr 6, 2022

It's been a while since I've look at the code but the change was centered around how predis client code expected the prefix to be presented. Atleast when I committed the code there was no way to pass this information to the predis code in a way that would allow it to function. It's possible that code changes now allow for it.

@jasonvarga
Copy link
Contributor

In the few lines above what was added in this PR, you can see that the options were already being merged in:

$formattedOptions = array_merge(
    ['timeout' => 10.0], $options, Arr::pull($config, 'options', [])
);

@tben
Copy link
Contributor Author

tben commented Apr 6, 2022

$options = $this->config['options'] ?? [];
if (isset($this->config[$name])) {
return $this->connector()->connect(
$this->parseConnectionConfiguration($this->config[$name]),
array_merge(Arr::except($options, 'parameters'), ['parameters' => Arr::get($options, 'parameters.'.$name, Arr::get($options, 'parameters', []))])
);
}

If you take a look at the code above. That's the parent code that calls the function that I modified. As you can see originally it would only send the global redis options not the individual options through the $config variable. This behaviour has since changed which is why it now works for you.

But I did want to point out that this code does seem pointless now but it does actually keep the setting the same between the two redis drivers settings. As you can setup a prefix in the exact same way for phpredis.

if (! empty($config['prefix'])) {
$client->setOption(Redis::OPT_PREFIX, $config['prefix']);
}

@jasonvarga
Copy link
Contributor

The first argument passed into connect() would be the store's config, which could have options in it.

@tben
Copy link
Contributor Author

tben commented Apr 6, 2022

@jasonvarga Okay, I just checked on my local machine and you were correct. Yeah, that's really is my bad. :( I swear down that options didn't work for me when I wrote this fix.

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.

4 participants