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

[6.x] Adds missing options for PhpRedis #31182

Merged
merged 12 commits into from
Jan 21, 2020

Conversation

Wizofgoz
Copy link
Contributor

This PR adds support for options on PhpRedis connections that were not allowed before. This includes specifying a serializer, scan option, and failover behavior for cluster connections.

The serializer option allows for using igbinary which can use significantly less storage space as documented here.

The scan option allows for customizing the behavior of scan commands as described here.

The failover option allows for customizing the behavior of a cluster connection as described here.

Replaces #31021

@omega123456
Copy link

The serializer option doesnt seem to be working. The $option variable you reference in vendor/laravel/framework/src/Illuminate/Redis/Connectors/PhpRedisConnector.php:101 is never set, so it will never get into the if block

@@ -97,6 +97,14 @@ protected function createClient(array $config)
if (! empty($config['read_timeout'])) {
$client->setOption(Redis::OPT_READ_TIMEOUT, $config['read_timeout']);
}

if (! empty($options['serializer'])) {
Copy link
Contributor

@danijelk danijelk Jan 24, 2020

Choose a reason for hiding this comment

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

I just tested and confirm that $options isn't in this scope, as it is within createRedisClusterInstance which is probably what @Wizofgoz uses.

@Wizofgoz Wizofgoz restored the add-missing-options-for-phpredis branch January 24, 2020 23:28
@danijelk
Copy link
Contributor

Any status on this bug?

Would be a good idea to add OPT_COMPRESSION_LEVEL & OPT_COMPRESSION while we're on this missing options issue. I should be able to do a new PR tomorrow if no one else has the time @Wizofgoz @driesvints

@driesvints
Copy link
Member

@danijelk feel free to send in a pr. Make sure you add a thorough description it.

@Wizofgoz
Copy link
Contributor Author

I briefly looked into it and it looks like I copy/pasted between createRedisClusterInstance() and createClient() not realizing that the variable names were different and my IDE didn't warn me. After correcting the variable name, a few tests were failing and I ran out of time looking into it so feel free to take a crack at it if you have time.

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.

6 participants