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

Fix Memcached::setOption for OPT_LIBKETAMA_COMPATIBLE #4364

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@adsr
Contributor

adsr commented Dec 1, 2014

The current implementation is missing an if-statement which causes
the option to be unset even when a truthy value is passed in. The comment
directly above this block describes the correct behavior. See the original PHP
implementation for reference: http://goo.gl/7p7WqP. A new constant
Memcached::DISTRIBUTION_CONSISTENT_KETAMA is added for the sake of testing.

Fix Memcached::setOption for OPT_LIBKETAMA_COMPATIBLE
Summary: The current implementation is missing an if-statement which causes
the option to be unset even when a truthy value is passed in. The comment
directly above this block describes the correct behavior. See the original PHP
implementation for reference: http://goo.gl/7p7WqP. A new constant
Memcached::DISTRIBUTION_CONSISTENT_KETAMA is added for the sake of testing.
@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Dec 1, 2014

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D29703

adsr added some commits Dec 2, 2014

Add option Memcached::DISTRIBUTION_CONSISTENT_KETAMA and fix test
Summary: The test in the previous commit was checking for the wrong constant.
I think I was testing against an old libmemcached. Also the addServer line is
removed because the test does not actually make any network calls.
Wrap MEMCACHED_DISTRIBUTION_CONSISTENT_WEIGHTED in ifdef and skipif f…
…or test

in case we're building against an old version of libmemcached.

@hhvm-bot hhvm-bot closed this in 7eb91d4 Dec 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment