-
Notifications
You must be signed in to change notification settings - Fork 326
fix: compression edge case #255
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: compression edge case #255
Conversation
could you help to review the PR and accept it if it looks good to you? it fixes some problem due to incorrect compression flag set |
@dictcp Could I ask you to rebase this against master, as the php7 branch has been merged? |
2eaee8b
to
2b860f2
Compare
2b860f2
to
1b8d4f7
Compare
4a79ba6
to
3a2e9a1
Compare
@sodabrew rebased and target to merge to master branch. |
Thanks for the rebase! The doc link says
But the code is: One of those two expressions has the comparison operator flipped!? |
it should be
https://secure.php.net/manual/en/memcached.configuration.php#ini.memcached.compression-factor
https://github.com/php-memcached-dev/php-memcached/blob/master/memcached.ini#L93 hence, the condition (to early exit, without compression) should be |
Nit: should your change be Could I bug you to update the docs in this PR as you've described? |
@sodabrew push commit and update the description of the PR. sounds good to you? |
Thank you! |
the condition to do compression is incorrent since previous refactoring. according to docs
https://secure.php.net/manual/en/memcached.configuration.php#ini.memcached.compression-factor
https://github.com/php-memcached-dev/php-memcached/blob/master/memcached.ini#L93
hence, the condition (to early exit, without compression) should be
if (ZSTR_LEN(payload) < (compressed_size * compression_factor))
PR includes