Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix problem with floatvalue-scores in PHP 5.4 #189

Merged
merged 5 commits into from Aug 16, 2012

Conversation

Projects
None yet
4 participants
Contributor

maxbeutel commented May 23, 2012

Due to a bug/weird behaviour in PHP 5.4 with _php_math_number_format float values were transformed to invalid values.

Test script:

$redis = new Redis();
$redis->connect('redis01');
$redis->zAdd('foo', 1337706476.8469, 'VAL');

Redis command sent with rev. f8f552e

"ZADD" "foo" "1" "VAL"

Expected command (at least within a certain precision range)

"ZADD" "foo" "1337706476.84689999" "VAL"

This patch circumvents the problem by not using _php_math_number_format at all and using spprintf which is also used internally in _php_math_number_format for getting the string length of the input.

See https://bugs.php.net/bug.php?id=62112
PHP Version: PHP 5.4.1 on Debian Squeeze

Contributor

maxbeutel commented May 23, 2012

Other functions like incrbyfloat might be affected as well because _php_math_number_format is also used in library.c which I didnt notice before, we only use zAdd.

I think it would make sense to replace all occurences of _php_math_number_format with spprintf, if you are ok with this solution proposed?

Contributor

nicolasff commented May 23, 2012

Hello Max,

I think this is how I had implemented it in the first place, but there were issues with several locales, notably French which would use a comma instead of a period as a decimal separator.
Have you tried your change with these?

Nicolas

Contributor

maxbeutel commented May 23, 2012

Hi, I think you are right, that is an issue with sprintf and the like... I updated the pull request, we can use _php_math_number_format_ex in PHP 5.4 and give the thousand separator a zero length, then the function returns a correct value.

Contributor

maxbeutel commented May 23, 2012

This changes breaks the tests, didn´t check it again unter PHP 5.4, will fix it soon.

Contributor

nicolasff commented May 23, 2012

That looks pretty good, I'll try it tonight.

Contributor

maxbeutel commented May 23, 2012

This should work now, I used the API version for comparision, thats also used in library.c. Thanks a lot to @lstrojny for help!

Can we get some feedback here?

Owner

michael-grunder commented Jun 12, 2012

Sorry guys,

It looks fine to me. I'll clone your fork and do some testing to see if anything breaks on various versions of PHP.

Thanks,
Mike

Thanks!

michael-grunder added a commit that referenced this pull request Jun 16, 2012

Use a custom method to encode float values, depending on our PHP vers…
…ion.

Many thanks to @maxbeutel and @lstrojny for reporting this issue and
coming up with a resolution!

This is related to fix/pull #189
Owner

michael-grunder commented Jun 16, 2012

Hey guys,

I have incorporated your changes into a new branch php-numencode that I will merge to the master branch after a bit more testing (over the weekend). So far it is working great for me. @nicolasff had some concerns about different locales, so I will test that specifically.

Apologies for not simply merging your changes, but I wasn't exactly sure of the process to manually merge changes within git, so I just typed them in.

Cheers!
Mike

The patch uses PHP's number_format() which is not locale aware.

Owner

michael-grunder commented Jun 16, 2012

@lstrojny I think this is OK though, because it's using \0 as the thousands separator, which (for the different versions of PHP I've tried this on) works.

Yep, it may not be locale aware. Using \0 as thousand separator no longer works with 5.4 though, that’s all the patch is about.

Owner

michael-grunder commented Jun 18, 2012

Man, i'm sorry. I must be confused about something. I tested with 5.4 and 5.3.10 and both work properly for me, whereas the described bug occures for me without the patch.

What am I missing?

Nothing. The patch fixes the issue with PHP 5.4 and we use it in production for a few weeks now.

Owner

michael-grunder commented Jun 30, 2012

Cool. I have since figured out how to manually merge in git, so I'll delete the branch I did and import the changes from the pull request so proper credit is given. :)

Cheers guys,
Mike

lstrojny commented Jul 2, 2012

Any news?

Owner

michael-grunder commented Jul 2, 2012

I added the InterNations upstream and manually merged into a new branch. If it looks good to @nicolasff we'll merge into the master.

Any news? This pull requests has been sitting waiting for a while now, could we get it merged. If you need anything from us, please let us know.

@michael-grunder michael-grunder merged commit c12a873 into phpredis:master Aug 16, 2012

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