-
Notifications
You must be signed in to change notification settings - Fork 328
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
#460 conditionally enable lazy loading for phpredis
#481
Conversation
@rvanlaak Could you target branch |
sprintf('Lazy loading Redis is not supported on PhpRedis %s. Please update to Phpredis %s or higher.', $phpRedisVersion, $minimumVersionForLazyLoading), | ||
E_USER_WARNING | ||
); | ||
} else { |
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.
There's really no reason for an else
here, this is basically saying
else if ($var === true) {
set(true);
}
which is equivalent to just
set($var);
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.
@B-Galati up to you 👍 I'm neutral in this one
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.
@dkarlovi You mean we could do $phpredisDef->setLazy($supportsLazyServices);
instead of the else block right?
If yes it sounds better 👍
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.
@B-Galati yes.
LGTM |
For your information. Using ubuntu 7.2, standard distribution of PHP 7.2 (which will be maintained for like; 10 years haha) and its php-redis package, the issue #460 occurs. I assume this is a must have. |
#487 targets |
Closing in favor of #487 |
Replaces #479
Fixes #460